| Message ID | 20230508020120.218494-6-rmclure@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc: KCSAN fix warnings and mark accesses | expand |
On Mon May 8, 2023 at 12:01 PM AEST, Rohan McLure wrote: > Checks to see if the [H]SRR registers have been clobbered by (soft) > NMI interrupts imply the possibility for a data race on the > [h]srr_valid entries in the PACA. Annotate accesses to these fields with > READ_ONCE, removing the need for the barrier. > > The diagnostic can use plain-access reads and writes, but annotate with > data_race. Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > > Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> > Reported-by: Michael Ellerman <mpe@ellerman.id.au> > --- > arch/powerpc/include/asm/ptrace.h | 4 ++-- > arch/powerpc/kernel/interrupt.c | 14 ++++++-------- > 2 files changed, 8 insertions(+), 10 deletions(-) > > diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h > index 0eb90a013346..9db8b16567e2 100644 > --- a/arch/powerpc/include/asm/ptrace.h > +++ b/arch/powerpc/include/asm/ptrace.h > @@ -180,8 +180,8 @@ void do_syscall_trace_leave(struct pt_regs *regs); > static inline void set_return_regs_changed(void) > { > #ifdef CONFIG_PPC_BOOK3S_64 > - local_paca->hsrr_valid = 0; > - local_paca->srr_valid = 0; > + WRITE_ONCE(local_paca->hsrr_valid, 0); > + WRITE_ONCE(local_paca->srr_valid, 0); > #endif > } > > diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c > index e34c72285b4e..1f033f11b871 100644 > --- a/arch/powerpc/kernel/interrupt.c > +++ b/arch/powerpc/kernel/interrupt.c > @@ -125,7 +125,7 @@ static notrace void check_return_regs_valid(struct pt_regs *regs) > case 0x1600: > case 0x1800: > validp = &local_paca->hsrr_valid; > - if (!*validp) > + if (!READ_ONCE(*validp)) > return; > > srr0 = mfspr(SPRN_HSRR0); > @@ -135,7 +135,7 @@ static notrace void check_return_regs_valid(struct pt_regs *regs) > break; > default: > validp = &local_paca->srr_valid; > - if (!*validp) > + if (!READ_ONCE(*validp)) > return; > > srr0 = mfspr(SPRN_SRR0); > @@ -161,19 +161,17 @@ static notrace void check_return_regs_valid(struct pt_regs *regs) > * such things will get caught most of the time, statistically > * enough to be able to get a warning out. > */ > - barrier(); > - > - if (!*validp) > + if (!READ_ONCE(*validp)) > return; > > - if (!warned) { > - warned = true; > + if (!data_race(warned)) { > + data_race(warned = true); > printk("%sSRR0 was: %lx should be: %lx\n", h, srr0, regs->nip); > printk("%sSRR1 was: %lx should be: %lx\n", h, srr1, regs->msr); > show_regs(regs); > } > > - *validp = 0; /* fixup */ > + WRITE_ONCE(*validp, 0); /* fixup */ > #endif > } > > -- > 2.37.2
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h index 0eb90a013346..9db8b16567e2 100644 --- a/arch/powerpc/include/asm/ptrace.h +++ b/arch/powerpc/include/asm/ptrace.h @@ -180,8 +180,8 @@ void do_syscall_trace_leave(struct pt_regs *regs); static inline void set_return_regs_changed(void) { #ifdef CONFIG_PPC_BOOK3S_64 - local_paca->hsrr_valid = 0; - local_paca->srr_valid = 0; + WRITE_ONCE(local_paca->hsrr_valid, 0); + WRITE_ONCE(local_paca->srr_valid, 0); #endif } diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c index e34c72285b4e..1f033f11b871 100644 --- a/arch/powerpc/kernel/interrupt.c +++ b/arch/powerpc/kernel/interrupt.c @@ -125,7 +125,7 @@ static notrace void check_return_regs_valid(struct pt_regs *regs) case 0x1600: case 0x1800: validp = &local_paca->hsrr_valid; - if (!*validp) + if (!READ_ONCE(*validp)) return; srr0 = mfspr(SPRN_HSRR0); @@ -135,7 +135,7 @@ static notrace void check_return_regs_valid(struct pt_regs *regs) break; default: validp = &local_paca->srr_valid; - if (!*validp) + if (!READ_ONCE(*validp)) return; srr0 = mfspr(SPRN_SRR0); @@ -161,19 +161,17 @@ static notrace void check_return_regs_valid(struct pt_regs *regs) * such things will get caught most of the time, statistically * enough to be able to get a warning out. */ - barrier(); - - if (!*validp) + if (!READ_ONCE(*validp)) return; - if (!warned) { - warned = true; + if (!data_race(warned)) { + data_race(warned = true); printk("%sSRR0 was: %lx should be: %lx\n", h, srr0, regs->nip); printk("%sSRR1 was: %lx should be: %lx\n", h, srr1, regs->msr); show_regs(regs); } - *validp = 0; /* fixup */ + WRITE_ONCE(*validp, 0); /* fixup */ #endif }
Checks to see if the [H]SRR registers have been clobbered by (soft) NMI interrupts imply the possibility for a data race on the [h]srr_valid entries in the PACA. Annotate accesses to these fields with READ_ONCE, removing the need for the barrier. The diagnostic can use plain-access reads and writes, but annotate with data_race. Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> Reported-by: Michael Ellerman <mpe@ellerman.id.au> --- arch/powerpc/include/asm/ptrace.h | 4 ++-- arch/powerpc/kernel/interrupt.c | 14 ++++++-------- 2 files changed, 8 insertions(+), 10 deletions(-)