Re: [PATCH] lib/percpu_counter: fix data race in __percpu_counter_limited_add()

[Mateusz Guzik <mjguzik@gmail.com>]

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.