| Message ID | 20240404044535.642122-4-rmclure@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] powerpc: Apply __always_inline to interrupt_{enter,exit}_prepare() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
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);
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);
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);
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(-)