* [PATCH 1/2] powerpc: Apply __always_inline to interrupt_{enter,exit}_prepare()
@ 2024-04-04 4:45 Rohan McLure
2024-04-04 4:45 ` [PATCH 2/2] powerpc/64: Only warn for kuap locked when KCSAN not present Rohan McLure
2024-04-04 6:14 ` [PATCH 1/2] powerpc: Apply __always_inline to interrupt_{enter,exit}_prepare() Christophe Leroy
0 siblings, 2 replies; 5+ messages in thread
From: Rohan McLure @ 2024-04-04 4:45 UTC (permalink / raw
To: linuxppc-dev; +Cc: Rohan McLure
In keeping with the advice given by Documentation/core-api/entry.rst,
entry and exit handlers for interrupts should not be instrumented.
Guarantee that the interrupt_{enter,exit}_prepare() routines are inlined
so that they will inheret instrumentation from their caller.
KCSAN kernels were observed to compile without inlining these routines,
which would lead to grief on NMI handlers.
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
arch/powerpc/include/asm/interrupt.h | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
index 7b610864b364..f4343e0bfb13 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -150,7 +150,7 @@ static inline void booke_restore_dbcr0(void)
#endif
}
-static inline void interrupt_enter_prepare(struct pt_regs *regs)
+static __always_inline void interrupt_enter_prepare(struct pt_regs *regs)
{
#ifdef CONFIG_PPC64
irq_soft_mask_set(IRQS_ALL_DISABLED);
@@ -215,11 +215,11 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs)
* However interrupt_nmi_exit_prepare does return directly to regs, because
* NMIs do not do "exit work" or replay soft-masked interrupts.
*/
-static inline void interrupt_exit_prepare(struct pt_regs *regs)
+static __always_inline void interrupt_exit_prepare(struct pt_regs *regs)
{
}
-static inline void interrupt_async_enter_prepare(struct pt_regs *regs)
+static __always_inline void interrupt_async_enter_prepare(struct pt_regs *regs)
{
#ifdef CONFIG_PPC64
/* Ensure interrupt_enter_prepare does not enable MSR[EE] */
@@ -238,7 +238,7 @@ static inline void interrupt_async_enter_prepare(struct pt_regs *regs)
irq_enter();
}
-static inline void interrupt_async_exit_prepare(struct pt_regs *regs)
+static __always_inline void interrupt_async_exit_prepare(struct pt_regs *regs)
{
/*
* Adjust at exit so the main handler sees the true NIA. This must
@@ -278,7 +278,7 @@ static inline bool nmi_disables_ftrace(struct pt_regs *regs)
return true;
}
-static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct interrupt_nmi_state *state)
+static __always_inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct interrupt_nmi_state *state)
{
#ifdef CONFIG_PPC64
state->irq_soft_mask = local_paca->irq_soft_mask;
@@ -340,7 +340,7 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
nmi_enter();
}
-static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct interrupt_nmi_state *state)
+static __always_inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct interrupt_nmi_state *state)
{
if (mfmsr() & MSR_DR) {
// nmi_exit if relocations are on
--
2.44.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] powerpc/64: Only warn for kuap locked when KCSAN not present
2024-04-04 4:45 [PATCH 1/2] powerpc: Apply __always_inline to interrupt_{enter,exit}_prepare() Rohan McLure
@ 2024-04-04 4:45 ` Rohan McLure
2024-04-04 6:14 ` Christophe Leroy
2024-04-04 6:14 ` [PATCH 1/2] powerpc: Apply __always_inline to interrupt_{enter,exit}_prepare() Christophe Leroy
1 sibling, 1 reply; 5+ messages in thread
From: Rohan McLure @ 2024-04-04 4:45 UTC (permalink / raw
To: linuxppc-dev; +Cc: Rohan McLure, Nicholas Piggin
Arbitrary instrumented locations, including syscall handlers, can call
arch_local_irq_restore() transitively when KCSAN is enabled, and in turn
also replay_soft_interrupts_irqrestore(). The precondition on entry to
this routine that is checked is that KUAP is enabled (user access
prohibited). Failure to meet this condition only triggers a warning
however, and afterwards KUAP is enabled anyway. That is, KUAP being
disabled on entry is in fact permissable, but not possible on an
uninstrumented kernel.
Disable this assertion only when KCSAN is enabled.
Suggested-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
arch/powerpc/kernel/irq_64.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/irq_64.c b/arch/powerpc/kernel/irq_64.c
index d5c48d1b0a31..18b2048389a2 100644
--- a/arch/powerpc/kernel/irq_64.c
+++ b/arch/powerpc/kernel/irq_64.c
@@ -189,7 +189,8 @@ static inline __no_kcsan void replay_soft_interrupts_irqrestore(void)
* and re-locking AMR but we shouldn't get here in the first place,
* hence the warning.
*/
- kuap_assert_locked();
+ if (!IS_ENABLED(CONFIG_KCSAN))
+ kuap_assert_locked();
if (kuap_state != AMR_KUAP_BLOCKED)
set_kuap(AMR_KUAP_BLOCKED);
--
2.44.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] powerpc/64: Only warn for kuap locked when KCSAN not present
2024-04-04 4:45 ` [PATCH 2/2] powerpc/64: Only warn for kuap locked when KCSAN not present Rohan McLure
@ 2024-04-04 6:14 ` Christophe Leroy
2024-04-11 1:53 ` Rohan McLure
0 siblings, 1 reply; 5+ messages in thread
From: Christophe Leroy @ 2024-04-04 6:14 UTC (permalink / raw
To: Rohan McLure, linuxppc-dev@lists.ozlabs.org; +Cc: Nicholas Piggin
Le 04/04/2024 à 06:45, Rohan McLure a écrit :
> Arbitrary instrumented locations, including syscall handlers, can call
> arch_local_irq_restore() transitively when KCSAN is enabled, and in turn
> also replay_soft_interrupts_irqrestore(). The precondition on entry to
> this routine that is checked is that KUAP is enabled (user access
> prohibited). Failure to meet this condition only triggers a warning
> however, and afterwards KUAP is enabled anyway. That is, KUAP being
> disabled on entry is in fact permissable, but not possible on an
> uninstrumented kernel.
>
> Disable this assertion only when KCSAN is enabled.
Please elaborate on that arbitrary call to arch_local_irq_restore()
transitively, when does it happen and why, and why only when KCSAN is
enabled.
I don't understand the reasoning, if it is permissible as you say, just
drop the warning. If the warning is there, it should stay also with
KCSAN. You should fix the root cause instead.
>
> Suggested-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> arch/powerpc/kernel/irq_64.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/irq_64.c b/arch/powerpc/kernel/irq_64.c
> index d5c48d1b0a31..18b2048389a2 100644
> --- a/arch/powerpc/kernel/irq_64.c
> +++ b/arch/powerpc/kernel/irq_64.c
> @@ -189,7 +189,8 @@ static inline __no_kcsan void replay_soft_interrupts_irqrestore(void)
> * and re-locking AMR but we shouldn't get here in the first place,
> * hence the warning.
> */
> - kuap_assert_locked();
> + if (!IS_ENABLED(CONFIG_KCSAN))
> + kuap_assert_locked();
>
> if (kuap_state != AMR_KUAP_BLOCKED)
> set_kuap(AMR_KUAP_BLOCKED);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] powerpc: Apply __always_inline to interrupt_{enter,exit}_prepare()
2024-04-04 4:45 [PATCH 1/2] powerpc: Apply __always_inline to interrupt_{enter,exit}_prepare() Rohan McLure
2024-04-04 4:45 ` [PATCH 2/2] powerpc/64: Only warn for kuap locked when KCSAN not present Rohan McLure
@ 2024-04-04 6:14 ` Christophe Leroy
1 sibling, 0 replies; 5+ messages in thread
From: Christophe Leroy @ 2024-04-04 6:14 UTC (permalink / raw
To: Rohan McLure, linuxppc-dev@lists.ozlabs.org
Le 04/04/2024 à 06:45, Rohan McLure a écrit :
> In keeping with the advice given by Documentation/core-api/entry.rst,
> entry and exit handlers for interrupts should not be instrumented.
> Guarantee that the interrupt_{enter,exit}_prepare() routines are inlined
> so that they will inheret instrumentation from their caller.
>
> KCSAN kernels were observed to compile without inlining these routines,
> which would lead to grief on NMI handlers.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/include/asm/interrupt.h | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
> index 7b610864b364..f4343e0bfb13 100644
> --- a/arch/powerpc/include/asm/interrupt.h
> +++ b/arch/powerpc/include/asm/interrupt.h
> @@ -150,7 +150,7 @@ static inline void booke_restore_dbcr0(void)
> #endif
> }
>
> -static inline void interrupt_enter_prepare(struct pt_regs *regs)
> +static __always_inline void interrupt_enter_prepare(struct pt_regs *regs)
> {
> #ifdef CONFIG_PPC64
> irq_soft_mask_set(IRQS_ALL_DISABLED);
> @@ -215,11 +215,11 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs)
> * However interrupt_nmi_exit_prepare does return directly to regs, because
> * NMIs do not do "exit work" or replay soft-masked interrupts.
> */
> -static inline void interrupt_exit_prepare(struct pt_regs *regs)
> +static __always_inline void interrupt_exit_prepare(struct pt_regs *regs)
> {
> }
>
> -static inline void interrupt_async_enter_prepare(struct pt_regs *regs)
> +static __always_inline void interrupt_async_enter_prepare(struct pt_regs *regs)
> {
> #ifdef CONFIG_PPC64
> /* Ensure interrupt_enter_prepare does not enable MSR[EE] */
> @@ -238,7 +238,7 @@ static inline void interrupt_async_enter_prepare(struct pt_regs *regs)
> irq_enter();
> }
>
> -static inline void interrupt_async_exit_prepare(struct pt_regs *regs)
> +static __always_inline void interrupt_async_exit_prepare(struct pt_regs *regs)
> {
> /*
> * Adjust at exit so the main handler sees the true NIA. This must
> @@ -278,7 +278,7 @@ static inline bool nmi_disables_ftrace(struct pt_regs *regs)
> return true;
> }
>
> -static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct interrupt_nmi_state *state)
> +static __always_inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct interrupt_nmi_state *state)
> {
> #ifdef CONFIG_PPC64
> state->irq_soft_mask = local_paca->irq_soft_mask;
> @@ -340,7 +340,7 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
> nmi_enter();
> }
>
> -static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct interrupt_nmi_state *state)
> +static __always_inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct interrupt_nmi_state *state)
> {
> if (mfmsr() & MSR_DR) {
> // nmi_exit if relocations are on
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] powerpc/64: Only warn for kuap locked when KCSAN not present
2024-04-04 6:14 ` Christophe Leroy
@ 2024-04-11 1:53 ` Rohan McLure
0 siblings, 0 replies; 5+ messages in thread
From: Rohan McLure @ 2024-04-11 1:53 UTC (permalink / raw
To: Christophe Leroy, linuxppc-dev@lists.ozlabs.org; +Cc: Nicholas Piggin
On Thu, 2024-04-04 at 06:14 +0000, Christophe Leroy wrote:
>
>
> Le 04/04/2024 à 06:45, Rohan McLure a écrit :
> > Arbitrary instrumented locations, including syscall handlers, can
> > call
> > arch_local_irq_restore() transitively when KCSAN is enabled, and in
> > turn
> > also replay_soft_interrupts_irqrestore(). The precondition on entry
> > to
> > this routine that is checked is that KUAP is enabled (user access
> > prohibited). Failure to meet this condition only triggers a warning
> > however, and afterwards KUAP is enabled anyway. That is, KUAP being
> > disabled on entry is in fact permissable, but not possible on an
> > uninstrumented kernel.
> >
> > Disable this assertion only when KCSAN is enabled.
>
> Please elaborate on that arbitrary call to arch_local_irq_restore()
> transitively, when does it happen and why, and why only when KCSAN is
> enabled.
The implementation of kcsan depends on this_cpu_* routines, which in
turn need to manage irqs for correctness. This means that the presence
of KCSAN in a uaccess enabled epoch can introduce calls to
arch_local_irq_restore().
For this reason, the warning really only applies in the instance of
uninstrumented code, and so to prevent KCSAN from issuing a false-
positive here, it makes sense to issue it only when KCSAN is not
present.
>
> I don't understand the reasoning, if it is permissible as you say,
> just
> drop the warning. If the warning is there, it should stay also with
> KCSAN. You should fix the root cause instead.
By dropping this assertion when KCSAN is enabled, we open up the
opportunity for KCSAN to warn only when data races are actually
observed, rather than just instances where an unblocked AMR state is
inherited into an IRQ context.
>
> >
> > Suggested-by: Nicholas Piggin <npiggin@gmail.com>
> > Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> > ---
> > arch/powerpc/kernel/irq_64.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/kernel/irq_64.c
> > b/arch/powerpc/kernel/irq_64.c
> > index d5c48d1b0a31..18b2048389a2 100644
> > --- a/arch/powerpc/kernel/irq_64.c
> > +++ b/arch/powerpc/kernel/irq_64.c
> > @@ -189,7 +189,8 @@ static inline __no_kcsan void
> > replay_soft_interrupts_irqrestore(void)
> > * and re-locking AMR but we shouldn't get here in the
> > first place,
> > * hence the warning.
> > */
> > - kuap_assert_locked();
> > + if (!IS_ENABLED(CONFIG_KCSAN))
> > + kuap_assert_locked();
> >
> > if (kuap_state != AMR_KUAP_BLOCKED)
> > set_kuap(AMR_KUAP_BLOCKED);
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-04-11 1:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-04 4:45 [PATCH 1/2] powerpc: Apply __always_inline to interrupt_{enter,exit}_prepare() Rohan McLure
2024-04-04 4:45 ` [PATCH 2/2] powerpc/64: Only warn for kuap locked when KCSAN not present Rohan McLure
2024-04-04 6:14 ` Christophe Leroy
2024-04-11 1:53 ` Rohan McLure
2024-04-04 6:14 ` [PATCH 1/2] powerpc: Apply __always_inline to interrupt_{enter,exit}_prepare() Christophe Leroy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).