Messages in this thread | | | Date | Tue, 22 Nov 2022 13:13:35 -0500 | Subject | Re: [PATCH 1/1] sched: fix user_mask double free | From | 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@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
| |