linux-kernel - Re: [PATCH linux-next] kernel/fork.c: annotate data races for copy

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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 17 Jun 2020 09:08:38 +0000
From:   Chenweilong <chenweilong@...wei.com>
To:     Christian Brauner <christian.brauner@...ntu.com>
CC:     "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        Lizefan <lizefan@...wei.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "dvyukov@...gle.com" <dvyukov@...gle.com>
Subject: Re: [PATCH linux-next] kernel/fork.c: annotate data races for
 copy_process

>On Tue, Jun 09, 2020 at 11:08:01AM +0800, Weilong Chen wrote:
>> The check is only there to stop root fork bombs.
>>
>> BUG: KCSAN: data-race in copy_process / copy_process
>>
>> write to 0xffffffff86f87d20 of 4 bytes by task 7121 on cpu 5:
>>  copy_process+0x2e1a/0x3af0 kernel/fork.c:2285
>>  _do_fork+0xf7/0x790 kernel/fork.c:2430
>>  __do_sys_clone+0xf9/0x130 kernel/fork.c:2585  __se_sys_clone
>> kernel/fork.c:2566 [inline]
>>  __x64_sys_clone+0x6c/0x80 kernel/fork.c:2566
>>  do_syscall_64+0xc7/0x3b0 arch/x86/entry/common.c:295
>>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> read to 0xffffffff86f87d20 of 4 bytes by task 7125 on cpu 3:
>>  copy_process+0x9eb/0x3af0 kernel/fork.c:1967
>>  _do_fork+0xf7/0x790 kernel/fork.c:2430
>>  __do_sys_clone+0xf9/0x130 kernel/fork.c:2585  __se_sys_clone
>> kernel/fork.c:2566 [inline]
>>  __x64_sys_clone+0x6c/0x80 kernel/fork.c:2566
>>  do_syscall_64+0xc7/0x3b0 arch/x86/entry/common.c:295
>>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> Signed-off-by: Weilong Chen <chenweilong@...wei.com>
>
>Plumbing data_race() in there just to taper over this seems ugly.
>Before we do that we should probably simply make nr_threads atomic_t.
Will using atomic_t cause performance degradation ? I don’t know why atomic was not used in the beginning.

>Also, where's the link to the syzbot/kcsan report? Or did you get this report from somewhere else?
I got this from local test.
>
>diff --git a/kernel/exit.c b/kernel/exit.c index c300253a7b8e..42e1cf640b20 100644
>--- a/kernel/exit.c
>+++ b/kernel/exit.c
>@@ -71,7 +71,7 @@
>
> static void __unhash_process(struct task_struct *p, bool group_dead)  {
>-       nr_threads--;
>+       atomic_dec(&nr_threads);
>        detach_pid(p, PIDTYPE_PID);
>        if (group_dead) {
>                detach_pid(p, PIDTYPE_TGID); diff --git a/kernel/fork.c b/kernel/fork.c index cefe8745c46e..c8355448d7c6 100644
>--- a/kernel/fork.c
>+++ b/kernel/fork.c
>@@ -122,7 +122,7 @@
>  * Protected counters by write_lock_irq(&tasklist_lock)
>  */
> unsigned long total_forks;     /* Handle normal Linux uptimes. */
>-int nr_threads;                        /* The idle threads do not count.. */
>+atomic_t nr_threads;           /* The idle threads do not count.. */
>
> static int max_threads;                /* tunable limit on nr_threads */
>
>@@ -1978,7 +1978,7 @@ static __latent_entropy struct task_struct *copy_process(
>         * to stop root fork bombs.
>         */
>        retval = -EAGAIN;
>-       if (nr_threads >= max_threads)
>+       if (atomic_read(&nr_threads) >= max_threads)
>                goto bad_fork_cleanup_count;
>
>        delayacct_tsk_init(p);  /* Must remain after dup_task_struct() */ @@ -2296,7 +2296,7 @@ static __latent_entropy struct task_struct *copy_process(
>                                          &p->signal->thread_head);
>                }
>                attach_pid(p, PIDTYPE_PID);
>-               nr_threads++;
>+               atomic_inc(&nr_threads);
>        }
>        total_forks++;
>        hlist_del_init(&delayed.node);
>
>> ---
>>  kernel/fork.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/fork.c b/kernel/fork.c index
>> 142b23645d82..efc5493203ae 100644
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -1977,7 +1977,7 @@ static __latent_entropy struct task_struct *copy_process(
>>       * to stop root fork bombs.
>>       */
>>      retval = -EAGAIN;
>> -    if (nr_threads >= max_threads)
>> +    if (data_race(nr_threads >= max_threads))
>>              goto bad_fork_cleanup_count;
>>
>>      delayacct_tsk_init(p);  /* Must remain after dup_task_struct() */
>> --
>> 2.17.1
>>
>