| Message ID | 20230510033117.1395895-6-rmclure@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc: KCSAN fix warnings and mark accesses | expand |
On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote: > The power_save callback can be overwritten by another core at boot time. > Specifically, null values will be replaced exactly once with the callback > suitable for the particular platform (PowerNV / pseries lpars), making > this value a good candidate for __ro_after_init. > > Even with this the case, KCSAN sees unmarked reads to the callback > variable, and notices that unfortunate compiler reorderings could lead > to distinct function pointers being read. In reality this is impossible, > so don't instrument at this read. > > Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> > --- > v2: Mark instances at init where the callback is written to, and > data_race() read as there is no capacity for the value to change > underneath. > --- > arch/powerpc/kernel/idle.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c > index b1c0418b25c8..43d96c0e3b96 100644 > --- a/arch/powerpc/kernel/idle.c > +++ b/arch/powerpc/kernel/idle.c > @@ -35,7 +35,7 @@ EXPORT_SYMBOL(cpuidle_disable); > > static int __init powersave_off(char *arg) > { > - ppc_md.power_save = NULL; > + WRITE_ONCE(ppc_md.power_save, NULL); > cpuidle_disable = IDLE_POWERSAVE_OFF; > return 1; > } Shouldn't need the WRITE_ONCE if you don't need a READ_ONCE. Does data_race work here too? What about the other writers? Does KCSAN know it's single threaded in early boot so skips marking, but perhaps this comes later? Would be good to have a little comment if so. Thanks, Nick > @@ -43,10 +43,13 @@ __setup("powersave=off", powersave_off); > > void arch_cpu_idle(void) > { > + /* power_save callback assigned only at init so no data race */ > + void (*power_save)(void) = data_race(ppc_md.power_save); > + > ppc64_runlatch_off(); > > - if (ppc_md.power_save) { > - ppc_md.power_save(); > + if (power_save) { > + power_save(); > /* > * Some power_save functions return with > * interrupts enabled, some don't. > -- > 2.37.2
> On 15 May 2023, at 3:50 pm, Nicholas Piggin <npiggin@gmail.com> wrote: > > On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote: >> The power_save callback can be overwritten by another core at boot time. >> Specifically, null values will be replaced exactly once with the callback >> suitable for the particular platform (PowerNV / pseries lpars), making >> this value a good candidate for __ro_after_init. >> >> Even with this the case, KCSAN sees unmarked reads to the callback >> variable, and notices that unfortunate compiler reorderings could lead >> to distinct function pointers being read. In reality this is impossible, >> so don't instrument at this read. >> >> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> >> --- >> v2: Mark instances at init where the callback is written to, and >> data_race() read as there is no capacity for the value to change >> underneath. >> --- >> arch/powerpc/kernel/idle.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c >> index b1c0418b25c8..43d96c0e3b96 100644 >> --- a/arch/powerpc/kernel/idle.c >> +++ b/arch/powerpc/kernel/idle.c >> @@ -35,7 +35,7 @@ EXPORT_SYMBOL(cpuidle_disable); >> >> static int __init powersave_off(char *arg) >> { >> - ppc_md.power_save = NULL; >> + WRITE_ONCE(ppc_md.power_save, NULL); >> cpuidle_disable = IDLE_POWERSAVE_OFF; >> return 1; >> } > > Shouldn't need the WRITE_ONCE if you don't need a READ_ONCE. Does > data_race work here too? What about the other writers? Does > KCSAN know it's single threaded in early boot so skips marking, > but perhaps this comes later? Would be good to have a little > comment if so. Apologies, yep I was meant to remove this WRITE_ONCE now that the read-side has data_race. Sorry for the confusion. > > Thanks, > Nick > >> @@ -43,10 +43,13 @@ __setup("powersave=off", powersave_off); >> >> void arch_cpu_idle(void) >> { >> + /* power_save callback assigned only at init so no data race */ >> + void (*power_save)(void) = data_race(ppc_md.power_save); >> + >> ppc64_runlatch_off(); >> >> - if (ppc_md.power_save) { >> - ppc_md.power_save(); >> + if (power_save) { >> + power_save(); >> /* >> * Some power_save functions return with >> * interrupts enabled, some don't. >> -- >> 2.37.2 >
diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c index b1c0418b25c8..43d96c0e3b96 100644 --- a/arch/powerpc/kernel/idle.c +++ b/arch/powerpc/kernel/idle.c @@ -35,7 +35,7 @@ EXPORT_SYMBOL(cpuidle_disable); static int __init powersave_off(char *arg) { - ppc_md.power_save = NULL; + WRITE_ONCE(ppc_md.power_save, NULL); cpuidle_disable = IDLE_POWERSAVE_OFF; return 1; } @@ -43,10 +43,13 @@ __setup("powersave=off", powersave_off); void arch_cpu_idle(void) { + /* power_save callback assigned only at init so no data race */ + void (*power_save)(void) = data_race(ppc_md.power_save); + ppc64_runlatch_off(); - if (ppc_md.power_save) { - ppc_md.power_save(); + if (power_save) { + power_save(); /* * Some power_save functions return with * interrupts enabled, some don't.
The power_save callback can be overwritten by another core at boot time. Specifically, null values will be replaced exactly once with the callback suitable for the particular platform (PowerNV / pseries lpars), making this value a good candidate for __ro_after_init. Even with this the case, KCSAN sees unmarked reads to the callback variable, and notices that unfortunate compiler reorderings could lead to distinct function pointers being read. In reality this is impossible, so don't instrument at this read. Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> --- v2: Mark instances at init where the callback is written to, and data_race() read as there is no capacity for the value to change underneath. --- arch/powerpc/kernel/idle.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)