v1 of this patch series available here:
Link: https://lore.kernel.org/linuxppc-dev/20230508020120.218494-1-rmclure@linux.ibm.com/
The KCSAN sanitiser notifies programmers of instances where unmarked
accesses to shared state has lead to a data race, or when the compiler
has liberty to reorder an unmarked access and so generate a data race.
This patch series deals with benign data races, which nonetheless need
annotation in order to ensure the correctness of the emitted code.
In keeping with the principles given in
tools/memory-model/Documentation/access-marking.txt, racing reads of
shared state for purely diagnostic/debug purposes are annotated with
data_race, while reads/writes that are examples of intention polling of
shared variables are performed with READ_ONCE, WRITE_ONCE.
These changes remove the majority of warnings observable on pseries and
powernv, where for development, I was able to narrow down to only power
relevant bugs by temporarily disabling sanitisation for all other files.
Future patch series will deal with the subtler bugs which persist under
this configuration.
KCSAN races addressed:
- qspinlock: assignign of qnode->locked and polling
- check_return_regs_valid [h]srr_valid
- arch_cpu_idle idle callback
- powernv idle_state paca entry (polling the bit-lock is viewed by
KCSAN as asynchronous access to the fields it protects)
- Asynchronous access to irq_data->hwirq
- Opal asynchronous event handling
- IPIs
Miscellaneous other changes:
- Annotate the asm-generic/mmiowb code, which riscv and powerpc each
consume
- Update usages of qnode->locked in powerpc's qspinlock interpretation
to reflect the comment beside this field
v2:
- Match READ_ONCE with WRITE_ONCE and vice versa where required
- In arch/powerpc/lib/qspinlock.c, use kcsan_release() to notify KCSAN
of locked being assigned prior to publish, and remove extraneous
compiler barrier (publish_tail_cpu features memory clobber).
- Keep polarity for locked variable in qspinlock
- Remove extraneous READ_ONCE in mmiowb()
- Use data_race() for power_save callback to remove instrumentation, as
there is no real data race
Rohan McLure (11):
powerpc: qspinlock: Mark accesses to qnode lock checks
powerpc: qspinlock: Enforce qnode writes prior to publishing to queue
asm-generic/mmiowb: Mark accesses to fix KCSAN warnings
powerpc: Mark [h]ssr_valid accesses in check_return_regs_valid
powerpc: Mark accesses to power_save callback in arch_cpu_idle
powerpc: powernv: Fix KCSAN datarace warnings on idle_state contention
powerpc: Annotate accesses to ipi message flags
powerpc: Mark writes registering ipi to host cpu through kvm and
polling
powerpc: powernv: Annotate data races in opal events
powerpc: powernv: Annotate asynchronous access to opal tokens
powerpc: Mark asynchronous accesses to irq_data
arch/powerpc/include/asm/kvm_ppc.h | 4 ++--
arch/powerpc/include/asm/paca.h | 1 +
arch/powerpc/include/asm/ptrace.h | 4 ++--
arch/powerpc/kernel/idle.c | 9 ++++++---
arch/powerpc/kernel/interrupt.c | 14 ++++++--------
arch/powerpc/kernel/irq.c | 2 +-
arch/powerpc/kernel/smp.c | 4 ++--
arch/powerpc/kvm/book3s_hv_builtin.c | 4 ++--
arch/powerpc/lib/qspinlock.c | 11 +++++++++--
arch/powerpc/platforms/powernv/idle.c | 16 +++++++++-------
arch/powerpc/platforms/powernv/opal-async.c | 6 +++---
arch/powerpc/platforms/powernv/opal-irqchip.c | 6 +++---
arch/powerpc/platforms/powernv/pci-ioda.c | 12 ++++++------
include/asm-generic/mmiowb.h | 14 +++++++++-----
include/linux/irq.h | 2 +-
kernel/irq/irqdomain.c | 4 ++--
16 files changed, 64 insertions(+), 49 deletions(-)
--
2.37.2
Annotate the release barrier and memory clobber (in effect, producing a
compiler barrier) in the publish_tail_cpu call. These barriers have the
effect of ensuring that qnode attributes are all written to prior to
publish the node to the waitqueue.
Even while the initial write to the 'locked' attribute is guaranteed to
terminate prior to the node being visible, KCSAN still complains that
the write is reorderable by the compiler. Issue a kcsan_release() to
inform KCSAN of the release barrier contained in publish_tail_cpu().
Signed-off-by: Rohan McLure <redacted>
---
v2: Remove extraneous compiler barrier, but annotate release-barrier
contained in call publish_tail_cpu(), and include kcsan_release().
---
arch/powerpc/lib/qspinlock.c | 7 +++++++
1 file changed, 7 insertions(+)
The opal-async.c unit contains code for polling event sources, which
implies intentional data races. Ensure that the compiler will atomically
access such variables by means of {READ,WRITE}_ONCE calls, which in turn
inform KCSAN that polling behaviour is intended.
Signed-off-by: Rohan McLure <redacted>
---
arch/powerpc/platforms/powernv/opal-async.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
The powerpc implemenation of qspinlocks will both poll and spin on the
bitlock guarding a qnode. Mark these accesses with READ_ONCE to convey
to KCSAN that polling is intentional here.
Signed-off-by: Rohan McLure <redacted>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/lib/qspinlock.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -584,7 +584,7 @@ static __always_inline void queued_spin_lock_mcs_queue(struct qspinlock *lock, b/* Wait for mcs node lock to be released */spin_begin();-while(!node->locked){+while(!READ_ONCE(node->locked)){spec_barrier();if(yield_to_prev(lock,node,old,paravirt))
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 <redacted>
---
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(-)
@@ -43,10 +43,13 @@ __setup("powersave=off", powersave_off);voidarch_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();/**Somepower_savefunctionsreturnwith*interruptsenabled,somedon't.
The idle_state entry in the PACA on PowerNV features a bit which is
atomically tested and set through ldarx/stdcx. to be used as a spinlock.
This lock then guards access to other bit fields of idle_state. KCSAN
cannot differentiate between any of these bitfield accesses as they all
are implemented by 8-byte store/load instructions, thus cores contending
on the bit-lock appear to data race with modifications to idle_state.
Separate the bit-lock entry from the data guarded by the lock to avoid
the possibility of data races being detected by KCSAN.
Suggested-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Rohan McLure <redacted>
---
v2: Remove extraneous WRITE_ONCE on paca thread_idle_state, which are
only read diagnostically.
---
arch/powerpc/include/asm/paca.h | 1 +
arch/powerpc/platforms/powernv/idle.c | 16 +++++++++-------
2 files changed, 10 insertions(+), 7 deletions(-)
@@ -191,6 +191,7 @@ struct paca_struct {#ifdef CONFIG_PPC_POWERNV/* PowerNV idle fields *//* PNV_CORE_IDLE_* bits, all siblings work on thread 0 paca */+unsignedlongidle_lock;/* A value of 1 means acquired */unsignedlongidle_state;union{/* P7/P8 specific fields */
IPI message flags are observed and consequently consumed in the
smp_ipi_demux_relaxed function, which handles these message sources
until it observes none more arriving. Mark the checked loop guard with
READ_ONCE, to signal to KCSAN that the read is known to be volatile, and
that non-determinism is expected. Mark write for message source in
smp_muxed_ipi_set_message().
Signed-off-by: Rohan McLure <redacted>
---
v2: Add missing WRITE_ONCE() in smp_muxed_ipi_set_message().
---
arch/powerpc/kernel/smp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Prior to this patch, data races are detectable by KCSAN of the following
forms:
[1] Asynchronous calls to mmiowb_set_pending() from an interrupt context
or otherwise outside of a critical section
[2] Interrupted critical sections, where the interrupt will itself
acquire a lock
In case [1], calling context does not need an mmiowb() call to be
issued, otherwise it would do so itself. Such calls to
mmiowb_set_pending() are either idempotent or no-ops.
In case [2], irrespective of when the interrupt occurs, the interrupt
will acquire and release its locks prior to its return, nesting_count
will continue balanced. In the worst case, the interrupted critical
section during a mmiowb_spin_unlock() call observes an mmiowb to be
pending and afterward is interrupted, leading to an extraneous call to
mmiowb(). This data race is clearly innocuous.
Mark all potentially asynchronous memory accesses with READ_ONCE or
WRITE_ONCE, including increments and decrements to nesting_count. This
has the effect of removing KCSAN warnings at consumer's callsites.
Signed-off-by: Rohan McLure <redacted>
Reported-by: Michael Ellerman <mpe@ellerman.id.au>
Reported-by: Gautam Menghani <redacted>
Tested-by: Gautam Menghani <redacted>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
v2: Remove extraneous READ_ONCE in mmiowb_set_pending for nesting_count
---
include/asm-generic/mmiowb.h | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
@@ -37,25 +37,29 @@ static inline void mmiowb_set_pending(void)structmmiowb_state*ms=__mmiowb_state();if(likely(ms->nesting_count))-ms->mmiowb_pending=ms->nesting_count;+WRITE_ONCE(ms->mmiowb_pending,ms->nesting_count);}staticinlinevoidmmiowb_spin_lock(void){structmmiowb_state*ms=__mmiowb_state();-ms->nesting_count++;++/* Increment need not be atomic. Nestedness is balanced over interrupts. */+WRITE_ONCE(ms->nesting_count,READ_ONCE(ms->nesting_count)+1);}staticinlinevoidmmiowb_spin_unlock(void){structmmiowb_state*ms=__mmiowb_state();+u16pending=READ_ONCE(ms->mmiowb_pending);-if(unlikely(ms->mmiowb_pending)){-ms->mmiowb_pending=0;+WRITE_ONCE(ms->mmiowb_pending,0);+if(unlikely(pending)){mmiowb();}-ms->nesting_count--;+/* Decrement need not be atomic. Nestedness is balanced over interrupts. */+WRITE_ONCE(ms->nesting_count,READ_ONCE(ms->nesting_count)-1);}#else#define mmiowb_set_pending() do { } while (0)
Mark writes to hypervisor ipi state so that KCSAN recognises these
asynchronous issue of kvmppc_{set,clear}_host_ipi to be intended, with
atomic writes. Mark asynchronous polls to this variable in
kvm_ppc_read_one_intr().
Signed-off-by: Rohan McLure <redacted>
---
v2: Add read-side annotations to both polling locations in
kvm_ppc_read_one_intr().
---
arch/powerpc/include/asm/kvm_ppc.h | 4 ++--
arch/powerpc/kvm/book3s_hv_builtin.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
@@ -406,7 +406,7 @@ static long kvmppc_read_one_intr(bool *again)return1;/* see if a host IPI is pending */-host_ipi=local_paca->kvm_hstate.host_ipi;+host_ipi=READ_ONCE(local_paca->kvm_hstate.host_ipi);if(host_ipi)return1;
@@ -466,7 +466,7 @@ static long kvmppc_read_one_intr(bool *again)*meantime.Ifit'sclear,webouncetheinterrupttothe*guest*/-host_ipi=local_paca->kvm_hstate.host_ipi;+host_ipi=READ_ONCE(local_paca->kvm_hstate.host_ipi);if(unlikely(host_ipi!=0)){/* We raced with the host,*weneedtoresendthatIPI,bummer
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 <redacted>
Reported-by: Michael Ellerman <mpe@ellerman.id.au>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/ptrace.h | 4 ++--
arch/powerpc/kernel/interrupt.c | 14 ++++++--------
2 files changed, 8 insertions(+), 10 deletions(-)
KCSAN revealed that while irq_data entries are written to either from
behind a mutex, or otherwise atomically, accesses to irq_data->hwirq can
occur asynchronously, without volatile annotation. Mark these accesses
with READ_ONCE to avoid unfortunate compiler reorderings and remove
KCSAN warnings.
Signed-off-by: Rohan McLure <redacted>
---
arch/powerpc/kernel/irq.c | 2 +-
arch/powerpc/platforms/powernv/pci-ioda.c | 12 ++++++------
include/linux/irq.h | 2 +-
kernel/irq/irqdomain.c | 4 ++--
4 files changed, 10 insertions(+), 10 deletions(-)
The kopald thread handles opal events as they appear, but by polling a
static bit-vector in last_outstanding_events. Annotate these data races
accordingly. We are not at risk of missing events, but use of READ_ONCE,
WRITE_ONCE will assist readers in seeing that kopald only consumes the
events it is aware of when it is scheduled. Also removes extraneous
KCSAN warnings.
Signed-off-by: Rohan McLure <redacted>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/platforms/powernv/opal-irqchip.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2023-05-12 02:21:23
Rohan McLure [off-list ref] writes:
Prior to this patch, data races are detectable by KCSAN of the following
forms:
[1] Asynchronous calls to mmiowb_set_pending() from an interrupt context
or otherwise outside of a critical section
[2] Interrupted critical sections, where the interrupt will itself
acquire a lock
In case [1], calling context does not need an mmiowb() call to be
issued, otherwise it would do so itself. Such calls to
mmiowb_set_pending() are either idempotent or no-ops.
In case [2], irrespective of when the interrupt occurs, the interrupt
will acquire and release its locks prior to its return, nesting_count
will continue balanced. In the worst case, the interrupted critical
section during a mmiowb_spin_unlock() call observes an mmiowb to be
pending and afterward is interrupted, leading to an extraneous call to
mmiowb(). This data race is clearly innocuous.
Mark all potentially asynchronous memory accesses with READ_ONCE or
WRITE_ONCE, including increments and decrements to nesting_count. This
has the effect of removing KCSAN warnings at consumer's callsites.
Signed-off-by: Rohan McLure <redacted>
Reported-by: Michael Ellerman <mpe@ellerman.id.au>
Reported-by: Gautam Menghani <redacted>
Tested-by: Gautam Menghani <redacted>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
v2: Remove extraneous READ_ONCE in mmiowb_set_pending for nesting_count
---
include/asm-generic/mmiowb.h | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
This will need wider review, it's used by a lot of other architectures.
Probably best to pull this one out of the series and post it separately.
Add at least linux-kernel and linux-arch to Cc, and probably Will as
he's the original author.
cheers
On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
quoted hunk
Annotate the release barrier and memory clobber (in effect, producing a
compiler barrier) in the publish_tail_cpu call. These barriers have the
effect of ensuring that qnode attributes are all written to prior to
publish the node to the waitqueue.
Even while the initial write to the 'locked' attribute is guaranteed to
terminate prior to the node being visible, KCSAN still complains that
the write is reorderable by the compiler. Issue a kcsan_release() to
inform KCSAN of the release barrier contained in publish_tail_cpu().
Signed-off-by: Rohan McLure <redacted>
---
v2: Remove extraneous compiler barrier, but annotate release-barrier
contained in call publish_tail_cpu(), and include kcsan_release().
---
arch/powerpc/lib/qspinlock.c | 7 +++++++
1 file changed, 7 insertions(+)
Possibly better to be with the publish function, hopefully the name
gives away it has a release barrier then store that makes it visible.
But that's nitpicking.
Thanks for the qspinlock fixes.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
quoted hunk
Prior to this patch, data races are detectable by KCSAN of the following
forms:
[1] Asynchronous calls to mmiowb_set_pending() from an interrupt context
or otherwise outside of a critical section
[2] Interrupted critical sections, where the interrupt will itself
acquire a lock
In case [1], calling context does not need an mmiowb() call to be
issued, otherwise it would do so itself. Such calls to
mmiowb_set_pending() are either idempotent or no-ops.
In case [2], irrespective of when the interrupt occurs, the interrupt
will acquire and release its locks prior to its return, nesting_count
will continue balanced. In the worst case, the interrupted critical
section during a mmiowb_spin_unlock() call observes an mmiowb to be
pending and afterward is interrupted, leading to an extraneous call to
mmiowb(). This data race is clearly innocuous.
Mark all potentially asynchronous memory accesses with READ_ONCE or
WRITE_ONCE, including increments and decrements to nesting_count. This
has the effect of removing KCSAN warnings at consumer's callsites.
Signed-off-by: Rohan McLure <redacted>
Reported-by: Michael Ellerman <mpe@ellerman.id.au>
Reported-by: Gautam Menghani <redacted>
Tested-by: Gautam Menghani <redacted>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
v2: Remove extraneous READ_ONCE in mmiowb_set_pending for nesting_count
---
include/asm-generic/mmiowb.h | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
@@ -37,25 +37,29 @@ static inline void mmiowb_set_pending(void)structmmiowb_state*ms=__mmiowb_state();if(likely(ms->nesting_count))-ms->mmiowb_pending=ms->nesting_count;+WRITE_ONCE(ms->mmiowb_pending,ms->nesting_count);}staticinlinevoidmmiowb_spin_lock(void){structmmiowb_state*ms=__mmiowb_state();-ms->nesting_count++;++/* Increment need not be atomic. Nestedness is balanced over interrupts. */+WRITE_ONCE(ms->nesting_count,READ_ONCE(ms->nesting_count)+1);}staticinlinevoidmmiowb_spin_unlock(void){structmmiowb_state*ms=__mmiowb_state();+u16pending=READ_ONCE(ms->mmiowb_pending);-if(unlikely(ms->mmiowb_pending)){-ms->mmiowb_pending=0;+WRITE_ONCE(ms->mmiowb_pending,0);+if(unlikely(pending)){mmiowb();}-ms->nesting_count--;+/* Decrement need not be atomic. Nestedness is balanced over interrupts. */+WRITE_ONCE(ms->nesting_count,READ_ONCE(ms->nesting_count)-1);
Still think the nesting_counts don't need WRITE_ONCE/READ_ONCE.
data_race() maybe but I don't know if it's even classed as a data
race. How does KCSAN handle/annotate preempt_count, for example?
Thanks,
Nick
On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
quoted hunk
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 <redacted>
---
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(-)
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
quoted hunk
@@ -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.
On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
The idle_state entry in the PACA on PowerNV features a bit which is
atomically tested and set through ldarx/stdcx. to be used as a spinlock.
This lock then guards access to other bit fields of idle_state. KCSAN
cannot differentiate between any of these bitfield accesses as they all
are implemented by 8-byte store/load instructions, thus cores contending
on the bit-lock appear to data race with modifications to idle_state.
Separate the bit-lock entry from the data guarded by the lock to avoid
the possibility of data races being detected by KCSAN.
Suggested-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Rohan McLure <redacted>
On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
IPI message flags are observed and consequently consumed in the
smp_ipi_demux_relaxed function, which handles these message sources
until it observes none more arriving. Mark the checked loop guard with
READ_ONCE, to signal to KCSAN that the read is known to be volatile, and
that non-determinism is expected. Mark write for message source in
smp_muxed_ipi_set_message().
Signed-off-by: Rohan McLure <redacted>
On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
Mark writes to hypervisor ipi state so that KCSAN recognises these
asynchronous issue of kvmppc_{set,clear}_host_ipi to be intended, with
atomic writes. Mark asynchronous polls to this variable in
kvm_ppc_read_one_intr().
Signed-off-by: Rohan McLure <redacted>
What's the go with accesses in asm? Does it just assume you know
what you're doing?
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
quoted hunk
---
v2: Add read-side annotations to both polling locations in
kvm_ppc_read_one_intr().
---
arch/powerpc/include/asm/kvm_ppc.h | 4 ++--
arch/powerpc/kvm/book3s_hv_builtin.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
@@ -406,7 +406,7 @@ static long kvmppc_read_one_intr(bool *again)return1;/* see if a host IPI is pending */-host_ipi=local_paca->kvm_hstate.host_ipi;+host_ipi=READ_ONCE(local_paca->kvm_hstate.host_ipi);if(host_ipi)return1;
@@ -466,7 +466,7 @@ static long kvmppc_read_one_intr(bool *again)*meantime.Ifit'sclear,webouncetheinterrupttothe*guest*/-host_ipi=local_paca->kvm_hstate.host_ipi;+host_ipi=READ_ONCE(local_paca->kvm_hstate.host_ipi);if(unlikely(host_ipi!=0)){/* We raced with the host,*weneedtoresendthatIPI,bummer
On 15 May 2023, at 3:53 pm, Nicholas Piggin [off-list ref] wrote:
On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
quoted
Mark writes to hypervisor ipi state so that KCSAN recognises these
asynchronous issue of kvmppc_{set,clear}_host_ipi to be intended, with
atomic writes. Mark asynchronous polls to this variable in
kvm_ppc_read_one_intr().
Signed-off-by: Rohan McLure <redacted>
What's the go with accesses in asm? Does it just assume you know
what you're doing?
Exactly, KCSAN only emits instrumentation calls to around load/store
instructions that the compiler itself generated. So by default, asm
accesses are not instrumented.
Thanks
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
quoted
---
v2: Add read-side annotations to both polling locations in
kvm_ppc_read_one_intr().
---
arch/powerpc/include/asm/kvm_ppc.h | 4 ++--
arch/powerpc/kvm/book3s_hv_builtin.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
@@ -406,7 +406,7 @@ static long kvmppc_read_one_intr(bool *again)
return 1;
/* see if a host IPI is pending */
- host_ipi = local_paca->kvm_hstate.host_ipi;
+ host_ipi = READ_ONCE(local_paca->kvm_hstate.host_ipi);
if (host_ipi)
return 1;
@@ -466,7 +466,7 @@ static long kvmppc_read_one_intr(bool *again)
* meantime. If it's clear, we bounce the interrupt to the
* guest
*/
- host_ipi = local_paca->kvm_hstate.host_ipi;
+ host_ipi = READ_ONCE(local_paca->kvm_hstate.host_ipi);
if (unlikely(host_ipi != 0)) {
/* We raced with the host,
* we need to resend that IPI, bummer
--
2.37.2
On 15 May 2023, at 3:50 pm, Nicholas Piggin [off-list ref] wrote:
On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
quoted
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 <redacted>
---
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(-)
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.
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 23 May 2023, at 10:28 am, Rohan McLure [off-list ref] wrote:
On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
quoted
Prior to this patch, data races are detectable by KCSAN of the following
forms:
[1] Asynchronous calls to mmiowb_set_pending() from an interrupt context
or otherwise outside of a critical section
[2] Interrupted critical sections, where the interrupt will itself
acquire a lock
In case [1], calling context does not need an mmiowb() call to be
issued, otherwise it would do so itself. Such calls to
mmiowb_set_pending() are either idempotent or no-ops.
In case [2], irrespective of when the interrupt occurs, the interrupt
will acquire and release its locks prior to its return, nesting_count
will continue balanced. In the worst case, the interrupted critical
section during a mmiowb_spin_unlock() call observes an mmiowb to be
pending and afterward is interrupted, leading to an extraneous call to
mmiowb(). This data race is clearly innocuous.
Mark all potentially asynchronous memory accesses with READ_ONCE or
WRITE_ONCE, including increments and decrements to nesting_count. This
has the effect of removing KCSAN warnings at consumer's callsites.
Signed-off-by: Rohan McLure <redacted>
Reported-by: Michael Ellerman <mpe@ellerman.id.au>
Reported-by: Gautam Menghani <redacted>
Tested-by: Gautam Menghani <redacted>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
v2: Remove extraneous READ_ONCE in mmiowb_set_pending for nesting_count
---
include/asm-generic/mmiowb.h | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
struct mmiowb_state *ms = __mmiowb_state();
if (likely(ms->nesting_count))
- ms->mmiowb_pending = ms->nesting_count;
+ WRITE_ONCE(ms->mmiowb_pending, ms->nesting_count);
}
static inline void mmiowb_spin_lock(void)
{
struct mmiowb_state *ms = __mmiowb_state();
- ms->nesting_count++;
+
+ /* Increment need not be atomic. Nestedness is balanced over interrupts. */
+ WRITE_ONCE(ms->nesting_count, READ_ONCE(ms->nesting_count) + 1);
}
static inline void mmiowb_spin_unlock(void)
{
struct mmiowb_state *ms = __mmiowb_state();
+ u16 pending = READ_ONCE(ms->mmiowb_pending);
- if (unlikely(ms->mmiowb_pending)) {
- ms->mmiowb_pending = 0;
+ WRITE_ONCE(ms->mmiowb_pending, 0);
+ if (unlikely(pending)) {
mmiowb();
}
- ms->nesting_count--;
+ /* Decrement need not be atomic. Nestedness is balanced over interrupts. */
+ WRITE_ONCE(ms->nesting_count, READ_ONCE(ms->nesting_count) - 1);
Still think the nesting_counts don't need WRITE_ONCE/READ_ONCE.
data_race() maybe but I don't know if it's even classed as a data
race. How does KCSAN handle/annotate preempt_count, for example?
Wow sorry my mail client has some unhelpful keybindings - I don’t know why it
thought I’d want to resend your last item!
Yeah I agree, we don’t need the compiler guarantees of WRITE_ONCE/READ_ONCE, and
yet it’s also not a real data-race. I think I’ll apply data_race() and comment as
I’m still seeing KCSAN warnings here.
Just from inspection, it appears as if __preempt_count_{add,sub} are unmarked and
so likely to generate KCSAN warnings also, but also asm-generic/preempt.h I think
hasn’t been updated to address any such warnings.
On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
quoted hunk
Prior to this patch, data races are detectable by KCSAN of the following
forms:
[1] Asynchronous calls to mmiowb_set_pending() from an interrupt context
or otherwise outside of a critical section
[2] Interrupted critical sections, where the interrupt will itself
acquire a lock
In case [1], calling context does not need an mmiowb() call to be
issued, otherwise it would do so itself. Such calls to
mmiowb_set_pending() are either idempotent or no-ops.
In case [2], irrespective of when the interrupt occurs, the interrupt
will acquire and release its locks prior to its return, nesting_count
will continue balanced. In the worst case, the interrupted critical
section during a mmiowb_spin_unlock() call observes an mmiowb to be
pending and afterward is interrupted, leading to an extraneous call to
mmiowb(). This data race is clearly innocuous.
Mark all potentially asynchronous memory accesses with READ_ONCE or
WRITE_ONCE, including increments and decrements to nesting_count. This
has the effect of removing KCSAN warnings at consumer's callsites.
Signed-off-by: Rohan McLure <redacted>
Reported-by: Michael Ellerman <mpe@ellerman.id.au>
Reported-by: Gautam Menghani <redacted>
Tested-by: Gautam Menghani <redacted>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
v2: Remove extraneous READ_ONCE in mmiowb_set_pending for nesting_count
---
include/asm-generic/mmiowb.h | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
struct mmiowb_state *ms = __mmiowb_state();
if (likely(ms->nesting_count))
- ms->mmiowb_pending = ms->nesting_count;
+ WRITE_ONCE(ms->mmiowb_pending, ms->nesting_count);
}
static inline void mmiowb_spin_lock(void)
{
struct mmiowb_state *ms = __mmiowb_state();
- ms->nesting_count++;
+
+ /* Increment need not be atomic. Nestedness is balanced over interrupts. */
+ WRITE_ONCE(ms->nesting_count, READ_ONCE(ms->nesting_count) + 1);
}
static inline void mmiowb_spin_unlock(void)
{
struct mmiowb_state *ms = __mmiowb_state();
+ u16 pending = READ_ONCE(ms->mmiowb_pending);
- if (unlikely(ms->mmiowb_pending)) {
- ms->mmiowb_pending = 0;
+ WRITE_ONCE(ms->mmiowb_pending, 0);
+ if (unlikely(pending)) {
mmiowb();
}
- ms->nesting_count--;
+ /* Decrement need not be atomic. Nestedness is balanced over interrupts. */
+ WRITE_ONCE(ms->nesting_count, READ_ONCE(ms->nesting_count) - 1);
Still think the nesting_counts don't need WRITE_ONCE/READ_ONCE.
data_race() maybe but I don't know if it's even classed as a data
race. How does KCSAN handle/annotate preempt_count, for example?
Thanks,
Nick
From: Michael Ellerman <hidden> Date: 2023-07-03 05:58:11
On Wed, 10 May 2023 13:31:06 +1000, Rohan McLure wrote:
v1 of this patch series available here:
Link: https://lore.kernel.org/linuxppc-dev/20230508020120.218494-1-rmclure@linux.ibm.com/
The KCSAN sanitiser notifies programmers of instances where unmarked
accesses to shared state has lead to a data race, or when the compiler
has liberty to reorder an unmarked access and so generate a data race.
This patch series deals with benign data races, which nonetheless need
annotation in order to ensure the correctness of the emitted code.
[...]