From: wangbiao3 <[email protected]>
Clone/Fork a new task,call dup_task_struct->arch_dup_task_struct(tsk,orig)
which copy the data of parent/sibling task inclding p->user_cpus_ptr,so
the user_cpus_ptr of newtask is the same with orig task's.When
dup_task_struct call dup_user_cpus_ptr(tsk, orig, node),it return 0
dircetly if src->user_cpus_ptris free by other task,in this case ,
the newtask's address of user_cpus_ptr is not changed. Finally,
wakup newtask to execute, call task_cpu_possible_mask-->
do_set_cpus_allowed to set new task's user_cpus_ptr(user_mask) which
call kfree user_mask at the end. So cause a slub double free panic.
Use pi_lock to protect content of user_cpus_ptr in dup_user_cpus_ptr and
clear dst->user_cpus_ptr when found src->user_cpus_ptr is null
kernel BUG at mm/slub.c:363!
Call trace:
__slab_free+0x230/0x28c
kfree+0x220/0x2cc
do_set_cpus_allowed+0x74/0xa4
select_fallback_rq+0x12c/0x200
wake_up_new_task+0x26c/0x304
kernel_clone+0x2c0/0x470
__arm64_sys_clone+0x5c/0x8c
invoke_syscall+0x60/0x150
el0_svc_common.llvm.13030543509303927816+0x98/0x114
do_el0_svc_compat+0x20/0x30
el0_svc_compat+0x28/0x90
el0t_32_sync_handler+0x7c/0xbc
el0t_32_sync+0x1b8/0x1bc
wangbiao3 (1):
sched: fix user_mask double free
kernel/sched/core.c | 31 ++++++++++++++++++++-----------
1 file changed, 20 insertions(+), 11 deletions(-)
--
2.38.1
#/******???ʼ????丽??????С??˾?ı?????Ϣ???????ڷ?????????ַ???г??ĸ??˻?Ⱥ?顣??ֹ?κ??????????κ???ʽʹ?ã?????????????ȫ???????ֵ?й¶?????ơ???ɢ???????ʼ??е???Ϣ?????????????˱??ʼ????????????绰???ʼ?֪ͨ?????˲?ɾ?????ʼ??? This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#
From: wangbiao3 <[email protected]>
Clone/Fork a new task,call dup_task_struct->arch_dup_task_struct(tsk,orig)
which copy the data of parent/sibling task inclding p->user_cpus_ptr,so
the user_cpus_ptr of newtask is the same with orig task's.When
dup_task_struct call dup_user_cpus_ptr(tsk, orig, node),it return 0
dircetly if src->user_cpus_ptris free by other task,in this case ,
the newtask's address of user_cpus_ptr is not changed. Finally,
wakup newtask to execute, call task_cpu_possible_mask-->
do_set_cpus_allowed to set new task's user_cpus_ptr(user_mask) which
call kfree user_mask at the end. So cause a slub double free panic.
Use pi_lock to protect content of user_cpus_ptr in dup_user_cpus_ptr and
clear dst->user_cpus_ptr when found src->user_cpus_ptr is null
kernel BUG at mm/slub.c:363!
Call trace:
__slab_free+0x230/0x28c
kfree+0x220/0x2cc
do_set_cpus_allowed+0x74/0xa4
select_fallback_rq+0x12c/0x200
wake_up_new_task+0x26c/0x304
kernel_clone+0x2c0/0x470
__arm64_sys_clone+0x5c/0x8c
invoke_syscall+0x60/0x150
el0_svc_common.llvm.13030543509303927816+0x98/0x114
do_el0_svc_compat+0x20/0x30
el0_svc_compat+0x28/0x90
el0t_32_sync_handler+0x7c/0xbc
el0t_32_sync+0x1b8/0x1bc
Signed-off-by: wangbiao3 <[email protected]>
---
kernel/sched/core.c | 31 ++++++++++++++++++++-----------
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index daff72f00385..b013d8b777b4 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2584,29 +2584,38 @@ void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new_mask)
__do_set_cpus_allowed(p, new_mask, 0);
}
+static inline struct cpumask *clear_user_cpus_ptr(struct task_struct *p)
+{
+ struct cpumask *user_mask = NULL;
+
+ swap(p->user_cpus_ptr, user_mask);
+
+ return user_mask;
+}
+
int dup_user_cpus_ptr(struct task_struct *dst, struct task_struct *src,
int node)
{
- if (!src->user_cpus_ptr)
- return 0;
+ unsigned long flags;
+ struct cpumask *user_mask = NULL;
dst->user_cpus_ptr = kmalloc_node(cpumask_size(), GFP_KERNEL, node);
if (!dst->user_cpus_ptr)
return -ENOMEM;
+ /* Use pi_lock to protect content of user_cpus_ptr */
+ raw_spin_lock_irqsave(&src->pi_lock, flags);
+ if (!src->user_cpus_ptr) {
+ user_mask = clear_user_cpus_ptr(dst);
+ raw_spin_unlock_irqrestore(&src->pi_lock, flags);
+ kfree(user_mask);
+ return 0;
+ }
cpumask_copy(dst->user_cpus_ptr, src->user_cpus_ptr);
+ raw_spin_unlock_irqrestore(&src->pi_lock, flags);
return 0;
}
-static inline struct cpumask *clear_user_cpus_ptr(struct task_struct *p)
-{
- struct cpumask *user_mask = NULL;
-
- swap(p->user_cpus_ptr, user_mask);
-
- return user_mask;
-}
-
void release_user_cpus_ptr(struct task_struct *p)
{
kfree(clear_user_cpus_ptr(p));
--
2.38.1
#/******???ʼ????丽??????С??˾?ı?????Ϣ???????ڷ?????????ַ???г??ĸ??˻?Ⱥ?顣??ֹ?κ??????????κ???ʽʹ?ã?????????????ȫ???????ֵ?й¶?????ơ???ɢ???????ʼ??е???Ϣ?????????????˱??ʼ????????????绰???ʼ?֪ͨ?????˲?ɾ?????ʼ??? This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#
+CC the missing people from get_maintainers.pl
On Monday 21 Nov 2022 at 18:04:20 (+0800), [email protected] wrote:
> From: wangbiao3 <[email protected]>
>
> Clone/Fork a new task,call dup_task_struct->arch_dup_task_struct(tsk,orig)
> which copy the data of parent/sibling task inclding p->user_cpus_ptr,so
> the user_cpus_ptr of newtask is the same with orig task's.When
> dup_task_struct call dup_user_cpus_ptr(tsk, orig, node),it return 0
> dircetly if src->user_cpus_ptris free by other task,in this case ,
> the newtask's address of user_cpus_ptr is not changed. Finally,
> wakup newtask to execute, call task_cpu_possible_mask-->
> do_set_cpus_allowed to set new task's user_cpus_ptr(user_mask) which
> call kfree user_mask at the end. So cause a slub double free panic.
>
> Use pi_lock to protect content of user_cpus_ptr in dup_user_cpus_ptr and
> clear dst->user_cpus_ptr when found src->user_cpus_ptr is null
>
> kernel BUG at mm/slub.c:363!
> Call trace:
> __slab_free+0x230/0x28c
> kfree+0x220/0x2cc
> do_set_cpus_allowed+0x74/0xa4
> select_fallback_rq+0x12c/0x200
> wake_up_new_task+0x26c/0x304
> kernel_clone+0x2c0/0x470
> __arm64_sys_clone+0x5c/0x8c
> invoke_syscall+0x60/0x150
> el0_svc_common.llvm.13030543509303927816+0x98/0x114
> do_el0_svc_compat+0x20/0x30
> el0_svc_compat+0x28/0x90
> el0t_32_sync_handler+0x7c/0xbc
> el0t_32_sync+0x1b8/0x1bc
>
> Signed-off-by: wangbiao3 <[email protected]>
> ---
> kernel/sched/core.c | 31 ++++++++++++++++++++-----------
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index daff72f00385..b013d8b777b4 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2584,29 +2584,38 @@ void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new_mask)
> __do_set_cpus_allowed(p, new_mask, 0);
> }
>
> +static inline struct cpumask *clear_user_cpus_ptr(struct task_struct *p)
> +{
> + struct cpumask *user_mask = NULL;
> +
> + swap(p->user_cpus_ptr, user_mask);
> +
> + return user_mask;
> +}
> +
> int dup_user_cpus_ptr(struct task_struct *dst, struct task_struct *src,
> int node)
> {
> - if (!src->user_cpus_ptr)
> - return 0;
Removing this puts the kmalloc_node() (and kfree()) below on the fast
path for everyone. It wouldn't be surprising if this causes regressions
for some people ...
Can we optimize that?
> + unsigned long flags;
> + struct cpumask *user_mask = NULL;
>
> dst->user_cpus_ptr = kmalloc_node(cpumask_size(), GFP_KERNEL, node);
> if (!dst->user_cpus_ptr)
> return -ENOMEM;
>
> + /* Use pi_lock to protect content of user_cpus_ptr */
> + raw_spin_lock_irqsave(&src->pi_lock, flags);
> + if (!src->user_cpus_ptr) {
> + user_mask = clear_user_cpus_ptr(dst);
> + raw_spin_unlock_irqrestore(&src->pi_lock, flags);
> + kfree(user_mask);
> + return 0;
> + }
> cpumask_copy(dst->user_cpus_ptr, src->user_cpus_ptr);
> + raw_spin_unlock_irqrestore(&src->pi_lock, flags);
> return 0;
> }
>
> -static inline struct cpumask *clear_user_cpus_ptr(struct task_struct *p)
> -{
> - struct cpumask *user_mask = NULL;
> -
> - swap(p->user_cpus_ptr, user_mask);
> -
> - return user_mask;
> -}
> -
> void release_user_cpus_ptr(struct task_struct *p)
> {
> kfree(clear_user_cpus_ptr(p));
> --
> 2.38.1
>
> #/******?????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????? This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#
Note that this legalese must be removed from your email before anybody
can apply this patch, please see
Documentation/process/submitting-patches.rst.
Thanks,
Quentin
On Tue, Nov 22, 2022 at 03:05:01PM +0100, Peter Zijlstra wrote:
>
> So you failed:
>
> > #/******?????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????? This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#
Also, that ^ is super fail, you're sending this to a public list. Please
tell your (IT) manager it makes your corporation look like an idiot.
So you failed:
- to Cc the original author of this code (Will Deacon)
- to report what version this is against (apparently Linus' tree)
- to check if this still applies to the latest tree (it doesn't)
- to Cc the author of the code it now conflicts with (Waiman)
- write something coherent in the changelog.
- to include a Fixes tag.
Still, let me try and make sense of things...
On Mon, Nov 21, 2022 at 06:04:20PM +0800, [email protected] wrote:
> From: wangbiao3 <[email protected]>
>
> Clone/Fork a new task,call dup_task_struct->arch_dup_task_struct(tsk,orig)
> which copy the data of parent/sibling task inclding p->user_cpus_ptr,so
> the user_cpus_ptr of newtask is the same with orig task's.When
> dup_task_struct call dup_user_cpus_ptr(tsk, orig, node),it return 0
> dircetly if src->user_cpus_ptris free by other task,in this case ,
> the newtask's address of user_cpus_ptr is not changed.
(even just inserting some whitespace would've made it so much easier to
read)
But, the only way that would be possible is if
force_compatible_cpus_allowed_ptr() were to be called on !current, and
that just doesn't happen, the only callsite is:
arch/arm64/kernel/process.c: force_compatible_cpus_allowed_ptr(current);
And you can't be in fork() and exec() at the same time.
If it were possible to call restrict_cpus_allowed_ptr() on a non-current
task then yes, absolutely, which is why:
8f9ea86fdf99 ("sched: Always preserve the user requested cpumask")
also wraps the thing in pi_lock, but looking at it now, perhaps it needs
to do the alloc/copy first and swap under pi_lock instead.
(leaving the rest for the newly Cc'ed)
> Finally,
> wakup newtask to execute, call task_cpu_possible_mask-->
> do_set_cpus_allowed to set new task's user_cpus_ptr(user_mask) which
> call kfree user_mask at the end. So cause a slub double free panic.
>
> Use pi_lock to protect content of user_cpus_ptr in dup_user_cpus_ptr and
> clear dst->user_cpus_ptr when found src->user_cpus_ptr is null
>
> kernel BUG at mm/slub.c:363!
> Call trace:
> __slab_free+0x230/0x28c
> kfree+0x220/0x2cc
> do_set_cpus_allowed+0x74/0xa4
> select_fallback_rq+0x12c/0x200
> wake_up_new_task+0x26c/0x304
> kernel_clone+0x2c0/0x470
> __arm64_sys_clone+0x5c/0x8c
> invoke_syscall+0x60/0x150
> el0_svc_common.llvm.13030543509303927816+0x98/0x114
> do_el0_svc_compat+0x20/0x30
> el0_svc_compat+0x28/0x90
> el0t_32_sync_handler+0x7c/0xbc
> el0t_32_sync+0x1b8/0x1bc
>
> Signed-off-by: wangbiao3 <[email protected]>
> ---
> kernel/sched/core.c | 31 ++++++++++++++++++++-----------
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index daff72f00385..b013d8b777b4 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2584,29 +2584,38 @@ void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new_mask)
> __do_set_cpus_allowed(p, new_mask, 0);
> }
>
> +static inline struct cpumask *clear_user_cpus_ptr(struct task_struct *p)
> +{
> + struct cpumask *user_mask = NULL;
> +
> + swap(p->user_cpus_ptr, user_mask);
> +
> + return user_mask;
> +}
> +
> int dup_user_cpus_ptr(struct task_struct *dst, struct task_struct *src,
> int node)
> {
> - if (!src->user_cpus_ptr)
> - return 0;
> + unsigned long flags;
> + struct cpumask *user_mask = NULL;
>
> dst->user_cpus_ptr = kmalloc_node(cpumask_size(), GFP_KERNEL, node);
> if (!dst->user_cpus_ptr)
> return -ENOMEM;
>
> + /* Use pi_lock to protect content of user_cpus_ptr */
> + raw_spin_lock_irqsave(&src->pi_lock, flags);
> + if (!src->user_cpus_ptr) {
> + user_mask = clear_user_cpus_ptr(dst);
> + raw_spin_unlock_irqrestore(&src->pi_lock, flags);
> + kfree(user_mask);
> + return 0;
> + }
> cpumask_copy(dst->user_cpus_ptr, src->user_cpus_ptr);
> + raw_spin_unlock_irqrestore(&src->pi_lock, flags);
> return 0;
> }
>
> -static inline struct cpumask *clear_user_cpus_ptr(struct task_struct *p)
> -{
> - struct cpumask *user_mask = NULL;
> -
> - swap(p->user_cpus_ptr, user_mask);
> -
> - return user_mask;
> -}
> -
> void release_user_cpus_ptr(struct task_struct *p)
> {
> kfree(clear_user_cpus_ptr(p));
> --
> 2.38.1
>
> #/******?????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????? This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#
On 11/22/22 09:05, Peter Zijlstra wrote:
> So you failed:
>
> - to Cc the original author of this code (Will Deacon)
> - to report what version this is against (apparently Linus' tree)
> - to check if this still applies to the latest tree (it doesn't)
> - to Cc the author of the code it now conflicts with (Waiman)
> - write something coherent in the changelog.
> - to include a Fixes tag.
>
> Still, let me try and make sense of things...
>
> On Mon, Nov 21, 2022 at 06:04:20PM +0800, [email protected] wrote:
>> From: wangbiao3 <[email protected]>
>>
>> Clone/Fork a new task,call dup_task_struct->arch_dup_task_struct(tsk,orig)
>> which copy the data of parent/sibling task inclding p->user_cpus_ptr,so
>> the user_cpus_ptr of newtask is the same with orig task's.When
>> dup_task_struct call dup_user_cpus_ptr(tsk, orig, node),it return 0
>> dircetly if src->user_cpus_ptris free by other task,in this case ,
>> the newtask's address of user_cpus_ptr is not changed.
> (even just inserting some whitespace would've made it so much easier to
> read)
>
> But, the only way that would be possible is if
> force_compatible_cpus_allowed_ptr() were to be called on !current, and
> that just doesn't happen, the only callsite is:
>
> arch/arm64/kernel/process.c: force_compatible_cpus_allowed_ptr(current);
>
> And you can't be in fork() and exec() at the same time.
>
> If it were possible to call restrict_cpus_allowed_ptr() on a non-current
> task then yes, absolutely, which is why:
>
> 8f9ea86fdf99 ("sched: Always preserve the user requested cpumask")
>
> also wraps the thing in pi_lock, but looking at it now, perhaps it needs
> to do the alloc/copy first and swap under pi_lock instead.
With the latest change, user_cpus_ptr, once set, will not be cleared
until when the task dies. That is why I don't recheck if user_cpus_ptr
is NULL under pi_lock. The user_cpus_ptr value can certainly changes
during its lifetime, but it will be stable under pi_lock.
clear_user_cpus_ptr() is called by release_user_cpus_ptr() only. As said
before, it is only call when the task dies at free_task() and so there
shouldn't be any other racing conditions that can happen at the same time.
David, can you try the latest tip tree to see if the problem is still
reproducible ?
Thanks,
Longman
On 11/22/22 10:39, Waiman Long wrote:
>
> On 11/22/22 09:05, Peter Zijlstra wrote:
>> So you failed:
>>
>> - to Cc the original author of this code (Will Deacon)
>> - to report what version this is against (apparently Linus' tree)
>> - to check if this still applies to the latest tree (it doesn't)
>> - to Cc the author of the code it now conflicts with (Waiman)
>> - write something coherent in the changelog.
>> - to include a Fixes tag.
>>
>> Still, let me try and make sense of things...
>>
>> On Mon, Nov 21, 2022 at 06:04:20PM +0800, [email protected] wrote:
>>> From: wangbiao3 <[email protected]>
>>>
>>> Clone/Fork a new task,call
>>> dup_task_struct->arch_dup_task_struct(tsk,orig)
>>> which copy the data of parent/sibling task inclding p->user_cpus_ptr,so
>>> the user_cpus_ptr of newtask is the same with orig task's.When
>>> dup_task_struct call dup_user_cpus_ptr(tsk, orig, node),it return 0
>>> dircetly if src->user_cpus_ptris free by other task,in this case ,
>>> the newtask's address of user_cpus_ptr is not changed.
>> (even just inserting some whitespace would've made it so much easier to
>> read)
>>
>> But, the only way that would be possible is if
>> force_compatible_cpus_allowed_ptr() were to be called on !current, and
>> that just doesn't happen, the only callsite is:
>>
>> arch/arm64/kernel/process.c: force_compatible_cpus_allowed_ptr(current);
>>
>> And you can't be in fork() and exec() at the same time.
>>
>> If it were possible to call restrict_cpus_allowed_ptr() on a non-current
>> task then yes, absolutely, which is why:
>>
>> 8f9ea86fdf99 ("sched: Always preserve the user requested cpumask")
>>
>> also wraps the thing in pi_lock, but looking at it now, perhaps it needs
>> to do the alloc/copy first and swap under pi_lock instead.
>
> With the latest change, user_cpus_ptr, once set, will not be cleared
> until when the task dies. That is why I don't recheck if user_cpus_ptr
> is NULL under pi_lock. The user_cpus_ptr value can certainly changes
> during its lifetime, but it will be stable under pi_lock.
> clear_user_cpus_ptr() is called by release_user_cpus_ptr() only. As
> said before, it is only call when the task dies at free_task() and so
> there shouldn't be any other racing conditions that can happen at the
> same time.
On second thought, do_set_cpus_allowed() can put NULL into
user_cpus_ptr. So I think we should do null check in dup_user_cpus_ptr()
inside the pi_lock. Will send a patch to do that.
Cheers,
Longman