linux-kernel - Re: [PATCH] percpu_counter: fix a data race at vm_committed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200221170713.GA227918@google.com>
Date:   Fri, 21 Feb 2020 18:07:13 +0100
From:   Marco Elver <elver@...gle.com>
To:     Qian Cai <cai@....pw>
Cc:     akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] percpu_counter: fix a data race at vm_committed_as



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@....pw>

Acked-by: Marco Elver <elver@...gle.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
>