Messages in this thread | | | Date | Wed, 23 Nov 2022 07:50:31 -0500 | Subject | Re: [PATCH] sched: Fix NULL user_cpus_ptr check in dup_user_cpus_ptr() | From | Waiman Long <> |
| |
On 11/23/22 06:20, Peter Zijlstra wrote: > On Tue, Nov 22, 2022 at 02:06:53PM -0500, Waiman Long wrote: >> In general, a non-null user_cpus_ptr will remain set until the task dies. >> A possible exception to this is the fact that do_set_cpus_allowed() >> will clear a non-null user_cpus_ptr. To allow this possible racing >> condition, we need to check for NULL user_cpus_ptr under the pi_lock >> before duping the user mask. >> >> Fixes: 851a723e45d1 ("sched: Always clear user_cpus_ptr in do_set_cpus_allowed()") >> Signed-off-by: Waiman Long <longman@redhat.com> >> --- >> kernel/sched/core.c | 23 +++++++++++++++++++++-- >> 1 file changed, 21 insertions(+), 2 deletions(-) >> >> diff --git a/kernel/sched/core.c b/kernel/sched/core.c >> index 8df51b08bb38..f447a6285ea2 100644 >> --- a/kernel/sched/core.c >> +++ b/kernel/sched/core.c >> @@ -2625,7 +2625,14 @@ int dup_user_cpus_ptr(struct task_struct *dst, struct task_struct *src, >> int node) >> { >> unsigned long flags; >> + cpumask_t *user_mask = NULL; > The inverse xmas tree is sad :-( Right. The inverse xmas tree rule. Will fix that. > >> >> + /* >> + * If there is a concurrent sched_setaffinity(), we may miss the >> + * newly updated user_cpus_ptr. However, a non-NULL user_cpus_ptr >> + * is relatively unlikely and it is not worth the extra overhead >> + * of taking the pi_lock on every fork/clone. >> + */ > I think the correct argument is saying the thing is racy and loosing the > race is a valid situation. IOW, this is the same as if the concurrent > sched_setaffinity() happens after fork(). Good point, will update the comment. >> if (!src->user_cpus_ptr) >> return 0; >> >> @@ -2633,10 +2640,22 @@ int dup_user_cpus_ptr(struct task_struct *dst, struct task_struct *src, >> if (!dst->user_cpus_ptr) >> return -ENOMEM; >> >> - /* Use pi_lock to protect content of user_cpus_ptr */ >> + /* >> + * Use pi_lock to protect content of user_cpus_ptr >> + * >> + * Though unlikely, user_cpus_ptr can be reset to NULL by a concurrent >> + * do_set_cpus_allowed(). >> + */ >> raw_spin_lock_irqsave(&src->pi_lock, flags); >> + if (src->user_cpus_ptr) >> + cpumask_copy(dst->user_cpus_ptr, src->user_cpus_ptr); >> + else >> + swap(dst->user_cpus_ptr, user_mask); > Uhhhh, did you mean to write: > > if (src->user_cpus_ptr) { > cpumask_copy(user_mask, src->user_cpus_ptr); > swap(dst->user_cpus_ptr, user_mask); > } > > ?
Not really. The point is that dst->user_cpus_ptr has been allocated. If src->user_cpus_ptr turns out to be NULL, we need to clear dst->user_cpus_ptr which is what the swap() does and then free that memory after unlock. Will add a comment to make this point clear.
Cheers, Longman
| |