[PATCH] percpu_counter: fix a data race at vm_committed_as

[PATCH] percpu_counter: fix a data race at vm_committed_as

[Qian Cai]

"vm_committed_as.count" could be accessed concurrently as reported by
KCSAN,

 BUG: KCSAN: data-race in __vm_enough_memory / percpu_counter_add_batch

 write to 0xffffffff9451c538 of 8 bytes by task 65879 on cpu 35:
  percpu_counter_add_batch+0x83/0xd0
  percpu_counter_add_batch at lib/percpu_counter.c:91
  __vm_enough_memory+0xb9/0x260
  dup_mm+0x3a4/0x8f0
  copy_process+0x2458/0x3240
  _do_fork+0xaa/0x9f0
  __do_sys_clone+0x125/0x160
  __x64_sys_clone+0x70/0x90
  do_syscall_64+0x91/0xb05
  entry_SYSCALL_64_after_hwframe+0x49/0xbe

 read to 0xffffffff9451c538 of 8 bytes by task 66773 on cpu 19:
  __vm_enough_memory+0x199/0x260
  percpu_counter_read_positive at include/linux/percpu_counter.h:81
  (inlined by) __vm_enough_memory at mm/util.c:839
  mmap_region+0x1b2/0xa10
  do_mmap+0x45c/0x700
  vm_mmap_pgoff+0xc0/0x130
  ksys_mmap_pgoff+0x6e/0x300
  __x64_sys_mmap+0x33/0x40
  do_syscall_64+0x91/0xb05
  entry_SYSCALL_64_after_hwframe+0x49/0xbe

The read is outside percpu_counter::lock critical section which results
in a data race. Fix it by adding a READ_ONCE() in
percpu_counter_read_positive() which could also service as the existing
compiler memory barrier.

Signed-off-by: Qian Cai <cai@lca.pw>
---
 include/linux/percpu_counter.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h
index 4f052496cdfd..0a4f54dd4737 100644
--- a/include/linux/percpu_counter.h
+++ b/include/linux/percpu_counter.h
@@ -78,9 +78,9 @@ static inline s64 percpu_counter_read(struct percpu_counter *fbc)
  */
 static inline s64 percpu_counter_read_positive(struct percpu_counter *fbc)
 {
-	s64 ret = fbc->count;
+	/* Prevent reloads of fbc->count */
+	s64 ret = READ_ONCE(fbc->count);
 
-	barrier();		/* Prevent reloads of fbc->count */
 	if (ret >= 0)
 		return ret;
 	return 0;
-- 
1.8.3.1

Re: [PATCH] percpu_counter: fix a data race at vm_committed_as

[Marco Elver]

On Fri, 21 Feb 2020, Qian Cai wrote:

> "vm_committed_as.count" could be accessed concurrently as reported by
> KCSAN,
> 
>  BUG: KCSAN: data-race in __vm_enough_memory / percpu_counter_add_batch
> 
>  write to 0xffffffff9451c538 of 8 bytes by task 65879 on cpu 35:
>   percpu_counter_add_batch+0x83/0xd0
>   percpu_counter_add_batch at lib/percpu_counter.c:91
>   __vm_enough_memory+0xb9/0x260
>   dup_mm+0x3a4/0x8f0
>   copy_process+0x2458/0x3240
>   _do_fork+0xaa/0x9f0
>   __do_sys_clone+0x125/0x160
>   __x64_sys_clone+0x70/0x90
>   do_syscall_64+0x91/0xb05
>   entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
>  read to 0xffffffff9451c538 of 8 bytes by task 66773 on cpu 19:
>   __vm_enough_memory+0x199/0x260
>   percpu_counter_read_positive at include/linux/percpu_counter.h:81
>   (inlined by) __vm_enough_memory at mm/util.c:839
>   mmap_region+0x1b2/0xa10
>   do_mmap+0x45c/0x700
>   vm_mmap_pgoff+0xc0/0x130
>   ksys_mmap_pgoff+0x6e/0x300
>   __x64_sys_mmap+0x33/0x40
>   do_syscall_64+0x91/0xb05
>   entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
> The read is outside percpu_counter::lock critical section which results
> in a data race. Fix it by adding a READ_ONCE() in
> percpu_counter_read_positive() which could also service as the existing
> compiler memory barrier.
> 
> Signed-off-by: Qian Cai <cai@lca.pw>

Acked-by: Marco Elver <elver@google.com>

FWIW in the function where this was inlined here, the generated code (on
x86 at least) is identical.

Thanks,
-- Marco

> ---
>  include/linux/percpu_counter.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h
> index 4f052496cdfd..0a4f54dd4737 100644
> --- a/include/linux/percpu_counter.h
> +++ b/include/linux/percpu_counter.h
> @@ -78,9 +78,9 @@ static inline s64 percpu_counter_read(struct percpu_counter *fbc)
>   */
>  static inline s64 percpu_counter_read_positive(struct percpu_counter *fbc)
>  {
> -	s64 ret = fbc->count;
> +	/* Prevent reloads of fbc->count */
> +	s64 ret = READ_ONCE(fbc->count);
>  
> -	barrier();		/* Prevent reloads of fbc->count */
>  	if (ret >= 0)
>  		return ret;
>  	return 0;
> -- 
> 1.8.3.1
>