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

[Waiman Long <longman@redhat.com>]

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@xiaomi.com wrote:
>>> From: wangbiao3 <wangbiao3@xiaomi.com>
>>>
>>> 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