lkml.org 
[lkml]   [2022]   [Nov]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: 答复: [External Mail]Re: [PATCH 1/1] sched: fix user mask double free
From
On 11/23/22 21:37, David Wang 王标 wrote:
>
> Dear Waiman,
>
> Yes, we have read
> https://lore.kernel.org/lkml/20221123131612.914906-1-longman@redhat.com/
>  and checked it  carefully.
>
> We mean dup_user_cpus_ptr should not judge if the src->user_cpus_ptr
> is null at the entry of the
> Function dup_user_cpus_ptr.
> If do this when user_cpus_ptr is freed by othe thread, but the parent
> task copy the user_cpus_ptr
> data for new task through dup_task_struct
> <https://opengrok.qualcomm.com/source/s?refs=dup_task_struct&project=KERNEL.PLATFORM.2.0>
> -> arch_dup_task_struct
> <https://opengrok.qualcomm.com/source/xref/KERNEL.PLATFORM.2.0/kernel_platform/common/kernel/fork.c#arch_dup_task_struct>(tsk
> <https://opengrok.qualcomm.com/source/s?defs=tsk&project=KERNEL.PLATFORM.2.0>,
> orig
> <https://opengrok.qualcomm.com/source/s?defs=orig&project=KERNEL.PLATFORM.2.0>)
> before the user_cpus_ptr
> ,
> is freed, ,next , when dup_task_struct
> <https://opengrok.qualcomm.com/source/s?refs=dup_task_struct&project=KERNEL.PLATFORM.2.0>
> call dup_user_cpus_ptr
> <https://opengrok.qualcomm.com/source/s?defs=dup_user_cpus_ptr&project=KERNEL.PLATFORM.2.0>(tsk
> <https://opengrok.qualcomm.com/source/s?defs=tsk&project=KERNEL.PLATFORM.2.0>,
> orig
> <https://opengrok.qualcomm.com/source/s?defs=orig&project=KERNEL.PLATFORM.2.0>,
> node
> <https://opengrok.qualcomm.com/source/s?defs=node&project=KERNEL.PLATFORM.2.0>),it
> will return directly
> without doing nothing.  When wake up new task , then call
> select_fallback_rqàdo_set_cpus_allowed,
> it will meet slub double free issue.  Then new path can not fix issue
> in this scenario.
> void do_set_cpus_allowed(struct task_struct *p, const struct cpumask
> *new_mask)
> {
>         struct affinity_context ac = {
>                .new_mask  = new_mask,
>                .user_mask = NULL,
>                .flags     = SCA_USER,      /* clear the user requested
> mask */
>         };
>         __do_set_cpus_allowed(p, &ac);
>         kfree(ac.user_mask);
> }
> Kfree kfree(ac.user_mask) cause double free issue. New patch just
> cover the user_cpus_ptr is freed
> after  code running into raw_spin_lock_irqsave, if it can not enter
> into pi_lock critical section,
> what will happen.
>
> Maybe should delte following code at the entry of fuction . Please
> help check it.
>
> -       if (!src->user_cpus_ptr)     //delete this
>
> -              return 0;            //delete  this
>
> We think maybe path needs a little more modification like following :
>
> kernel/sched/core.c
> <https://lore.kernel.org/lkml/20221123131612.914906-1-longman@redhat.com/#Z31kernel:sched:core.c>
> | 23 +++++++++++++++++++++--
>
> 1 file changed
> <https://lore.kernel.org/lkml/20221123131612.914906-1-longman@redhat.com/#related>,
> 21 insertions(+), 2 deletions(-)
>
> diff
> <https://lore.kernel.org/lkml/20221123131612.914906-1-longman@redhat.com/#iZ31kernel:sched:core.c>
> --git a/kernel/sched/core.c b/kernel/sched/core.c
>
> index 8df51b08bb38..6b259d9e127a 100644
>
> --- a/kernel/sched/core.c
>
> +++ b/kernel/sched/core.c
>
> @@ -2624,8 +2624,14 @@ void do_set_cpus_allowed(struct task_struct *p,
> const struct cpumask *new_mask)
>
> int dup_user_cpus_ptr(struct task_struct *dst, struct task_struct *src,
>
>       int node)
>
> {
>
> + cpumask_t *user_mask = NULL;
>
>        unsigned long flags;
>
> + /*
>
> + * This check is racy and losing the race is a valid situation.
>
> + * It is not worth the extra overhead of taking the pi_lock on
>
> + * every fork/clone.
>
> + */
>
> -       if (!src->user_cpus_ptr) //delete this
>
> -              return 0;            //delete this
>
The clearing of user_cpus_ptr is protected by pi_lock. IOW, racing
between dup_user_cpus_ptr() and do_set_cpus_allowed is not possible and
double free like what you have suggested should not happen. Yes, the
user_cpus_ptr check here is racy. The worst case result is that a
user_cpus_ptr has just been set in the task to be cloned, but it fail to
copy over the user mask. With or without the check, the race can happen.
The check is an optimization. Its effect is just make one outcome more
likely than the other.

Cheers,
Longman

\
 
 \ /
  Last update: 2022-11-24 05:01    [W:0.077 / U:0.480 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site