Re: [PATCH 1/1] sched: fix user_mask double free

[Waiman Long]

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, wangbiao3@xxxxxxxxxx wrote:
> > > From: wangbiao3 <wangbiao3@xxxxxxxxxx>
> > >
> > > 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