Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a7fb3cac-176f-3928-f3a3-23c25ab9677e@redhat.com>
Date:   Tue, 22 Nov 2022 10:39:12 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.0
Subject: Re: [PATCH 1/1] sched: fix user_mask double free
Content-Language: en-US
To:     Peter Zijlstra <peterz@infradead.org>, wangbiao3@xiaomi.com
Cc:     mingo@redhat.com, juri.lelli@redhat.com,
        vincent.guittot@linaro.org, brauner@kernel.org, bsegall@google.com,
        linux-kernel@vger.kernel.org, wenjieli@qti.qualcomm.com,
        chenguanyou@xiaomi.com, Will Deacon <will@kernel.org>
References: <cover.1669020858.git.wangbiao3@xiaomi.com>
 <b8970a530d420109ee9fe0b268e097fb839211b0.1669020858.git.wangbiao3@xiaomi.com>
 <Y3zXDZeTUgDz2hJ8@hirez.programming.kicks-ass.net>
From:   Waiman Long <longman@redhat.com>
In-Reply-To: <Y3zXDZeTUgDz2hJ8@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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.

David, can you try the latest tip tree to see if the problem is still 
reproducible ?

Thanks,
Longman