Messages in this thread | | | From | Chenweilong <> | Subject | Re: [PATCH linux-next] kernel/fork.c: annotate data races for copy_process | Date | Wed, 17 Jun 2020 09:08:38 +0000 |
| |
>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@huawei.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 >> >
| |