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 <[email protected]>
---
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
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 <[email protected]>
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.
Also, where's the link to the syzbot/kcsan report? Or did you get this
report from somewhere else?
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
>
>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 <[email protected]>
>
>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
>>
>
On 2020/6/17 17:08, Chenweilong wrote:
>> 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 <[email protected]>
>>
>> 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.
There is a comment just above the if statement to explain this race:
/*
* If multiple threads are within copy_process(), then this check
* triggers too late. This doesn't hurt, the check is only there
* to stop root fork bombs.
*/
This race won't go away by making nr_threads atomic, because I think it is
tasklist_lock that protects nr_thread.
Adding data_race() here I think makes the code more readable, as a supplementary
to the code comment.