* [PATCH] lib/percpu_counter: fix data race in __percpu_counter_limited_add()
@ 2025-05-06 10:24 Jeongjun Park
2025-05-06 11:56 ` Mateusz Guzik
0 siblings, 1 reply; 3+ messages in thread
From: Jeongjun Park @ 2025-05-06 10:24 UTC (permalink / raw)
To: dennis, tj, cl, akpm; +Cc: jack, hughd, linux-mm, linux-kernel, Jeongjun Park
The following data-race was found in __percpu_counter_limited_add():
==================================================================
BUG: KCSAN: data-race in __percpu_counter_limited_add / __percpu_counter_limited_add
write to 0xffff88801f417e50 of 8 bytes by task 6663 on cpu 0:
__percpu_counter_limited_add+0x388/0x4a0 lib/percpu_counter.c:386
percpu_counter_limited_add include/linux/percpu_counter.h:77 [inline]
shmem_inode_acct_blocks+0x10e/0x230 mm/shmem.c:233
shmem_alloc_and_add_folio mm/shmem.c:1923 [inline]
shmem_get_folio_gfp.constprop.0+0x87f/0xc90 mm/shmem.c:2533
shmem_get_folio mm/shmem.c:2639 [inline]
....
read to 0xffff88801f417e50 of 8 bytes by task 6659 on cpu 1:
__percpu_counter_limited_add+0xc8/0x4a0 lib/percpu_counter.c:344
percpu_counter_limited_add include/linux/percpu_counter.h:77 [inline]
shmem_inode_acct_blocks+0x10e/0x230 mm/shmem.c:233
shmem_alloc_and_add_folio mm/shmem.c:1923 [inline]
shmem_get_folio_gfp.constprop.0+0x87f/0xc90 mm/shmem.c:2533
shmem_get_folio mm/shmem.c:2639 [inline]
....
value changed: 0x000000000000396d -> 0x000000000000398e
==================================================================
__percpu_counter_limited_add() should protect fbc via raw_spin_lock(),
but it calls spinlock in the wrong place. This causes a data-race,
so we need to fix it to call raw_spin_lock() a bit earlier.
Fixes: beb986862844 ("shmem,percpu_counter: add _limited_add(fbc, limit, amount)")
Signed-off-by: Jeongjun Park <aha310510@gmail.com>
---
lib/percpu_counter.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
index 2891f94a11c6..17f9fc12b409 100644
--- a/lib/percpu_counter.c
+++ b/lib/percpu_counter.c
@@ -336,6 +336,7 @@ bool __percpu_counter_limited_add(struct percpu_counter *fbc,
return true;
local_irq_save(flags);
+ raw_spin_lock(&fbc->lock);
unknown = batch * num_online_cpus();
count = __this_cpu_read(*fbc->counters);
@@ -344,11 +345,10 @@ bool __percpu_counter_limited_add(struct percpu_counter *fbc,
((amount > 0 && fbc->count + unknown <= limit) ||
(amount < 0 && fbc->count - unknown >= limit))) {
this_cpu_add(*fbc->counters, amount);
- local_irq_restore(flags);
- return true;
+ good = true;
+ goto out;
}
- raw_spin_lock(&fbc->lock);
count = fbc->count + amount;
/* Skip percpu_counter_sum() when safe */
--
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] lib/percpu_counter: fix data race in __percpu_counter_limited_add()
2025-05-06 10:24 [PATCH] lib/percpu_counter: fix data race in __percpu_counter_limited_add() Jeongjun Park
@ 2025-05-06 11:56 ` Mateusz Guzik
2025-05-07 0:10 ` Andrew Morton
0 siblings, 1 reply; 3+ messages in thread
From: Mateusz Guzik @ 2025-05-06 11:56 UTC (permalink / raw)
To: Jeongjun Park; +Cc: dennis, tj, cl, akpm, jack, hughd, linux-mm, linux-kernel
On Tue, May 06, 2025 at 07:24:02PM +0900, Jeongjun Park wrote:
> The following data-race was found in __percpu_counter_limited_add():
>
> ==================================================================
> BUG: KCSAN: data-race in __percpu_counter_limited_add / __percpu_counter_limited_add
>
> write to 0xffff88801f417e50 of 8 bytes by task 6663 on cpu 0:
> __percpu_counter_limited_add+0x388/0x4a0 lib/percpu_counter.c:386
> percpu_counter_limited_add include/linux/percpu_counter.h:77 [inline]
> shmem_inode_acct_blocks+0x10e/0x230 mm/shmem.c:233
> shmem_alloc_and_add_folio mm/shmem.c:1923 [inline]
> shmem_get_folio_gfp.constprop.0+0x87f/0xc90 mm/shmem.c:2533
> shmem_get_folio mm/shmem.c:2639 [inline]
> ....
>
> read to 0xffff88801f417e50 of 8 bytes by task 6659 on cpu 1:
> __percpu_counter_limited_add+0xc8/0x4a0 lib/percpu_counter.c:344
> percpu_counter_limited_add include/linux/percpu_counter.h:77 [inline]
> shmem_inode_acct_blocks+0x10e/0x230 mm/shmem.c:233
> shmem_alloc_and_add_folio mm/shmem.c:1923 [inline]
> shmem_get_folio_gfp.constprop.0+0x87f/0xc90 mm/shmem.c:2533
> shmem_get_folio mm/shmem.c:2639 [inline]
> ....
>
> value changed: 0x000000000000396d -> 0x000000000000398e
> ==================================================================
>
> __percpu_counter_limited_add() should protect fbc via raw_spin_lock(),
> but it calls spinlock in the wrong place. This causes a data-race,
> so we need to fix it to call raw_spin_lock() a bit earlier.
>
> Fixes: beb986862844 ("shmem,percpu_counter: add _limited_add(fbc, limit, amount)")
> Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> ---
> lib/percpu_counter.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
> index 2891f94a11c6..17f9fc12b409 100644
> --- a/lib/percpu_counter.c
> +++ b/lib/percpu_counter.c
> @@ -336,6 +336,7 @@ bool __percpu_counter_limited_add(struct percpu_counter *fbc,
> return true;
>
> local_irq_save(flags);
> + raw_spin_lock(&fbc->lock);
> unknown = batch * num_online_cpus();
> count = __this_cpu_read(*fbc->counters);
>
> @@ -344,11 +345,10 @@ bool __percpu_counter_limited_add(struct percpu_counter *fbc,
> ((amount > 0 && fbc->count + unknown <= limit) ||
> (amount < 0 && fbc->count - unknown >= limit))) {
> this_cpu_add(*fbc->counters, amount);
> - local_irq_restore(flags);
> - return true;
> + good = true;
> + goto out;
> }
>
> - raw_spin_lock(&fbc->lock);
> count = fbc->count + amount;
>
> /* Skip percpu_counter_sum() when safe */
> --
>
As this always takes the centralized lock in the fast path this defeats
the point of using a per-cpu counter in the first place.
I noted this thing is buggy almost a year ago:
https://lore.kernel.org/linux-mm/5eemkb4lo5eefp7ijgncgogwmadyzmvjfjmmmvfiki6cwdskfs@hi2z4drqeuz6/
per the e-mail I don't believe existence of this routine is warranted.
shmem is still the only consumer.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] lib/percpu_counter: fix data race in __percpu_counter_limited_add()
2025-05-06 11:56 ` Mateusz Guzik
@ 2025-05-07 0:10 ` Andrew Morton
0 siblings, 0 replies; 3+ messages in thread
From: Andrew Morton @ 2025-05-07 0:10 UTC (permalink / raw)
To: Mateusz Guzik
Cc: Jeongjun Park, dennis, tj, cl, jack, hughd, linux-mm, linux-kernel
On Tue, 6 May 2025 13:56:13 +0200 Mateusz Guzik <mjguzik@gmail.com> wrote:
> > unknown = batch * num_online_cpus();
> > count = __this_cpu_read(*fbc->counters);
> >
> > @@ -344,11 +345,10 @@ bool __percpu_counter_limited_add(struct percpu_counter *fbc,
> > ((amount > 0 && fbc->count + unknown <= limit) ||
> > (amount < 0 && fbc->count - unknown >= limit))) {
> > this_cpu_add(*fbc->counters, amount);
> > - local_irq_restore(flags);
> > - return true;
> > + good = true;
> > + goto out;
> > }
> >
> > - raw_spin_lock(&fbc->lock);
> > count = fbc->count + amount;
> >
> > /* Skip percpu_counter_sum() when safe */
> > --
> >
>
> As this always takes the centralized lock in the fast path this defeats
> the point of using a per-cpu counter in the first place.
Well. It partially "defeats the point" if the client code actually
uses percpu_counter_limited_add(). Only shmem.c does that.
> I noted this thing is buggy almost a year ago:
> https://lore.kernel.org/linux-mm/5eemkb4lo5eefp7ijgncgogwmadyzmvjfjmmmvfiki6cwdskfs@hi2z4drqeuz6/
>
> per the e-mail I don't believe existence of this routine is warranted.
>
> shmem is still the only consumer.
Totally. It would be better to remove percpu_counter_limited_add() and
to implement its intended effect within shmem.c
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-05-07 0:10 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-05-06 10:24 [PATCH] lib/percpu_counter: fix data race in __percpu_counter_limited_add() Jeongjun Park
2025-05-06 11:56 ` Mateusz Guzik
2025-05-07 0:10 ` Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox