Re: [PATCH v23 2/9] sched: Fix modifying donor->blocked on without proper locking

[John Stultz]

On Wed, Oct 29, 2025 at 9:51 PM K Prateek Nayak <kprateek.nayak@xxxxxxx> wrote:
> On 10/30/2025 5:48 AM, John Stultz wrote:
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 517b26c515bc5..0533a14ce5935 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -6591,7 +6591,7 @@ static struct task_struct *proxy_deactivate(struct rq *rq, struct task_struct *d
> >                * as unblocked, as we aren't doing proxy-migrations
> >                * yet (more logic will be needed then).
> >                */
> > -             donor->blocked_on = NULL;
> > +             clear_task_blocked_on(donor, NULL);
>
> nit. You can probably switch this to use __clear_task_blocked_on() in
> the previous patch and then to the clear_task_blocked_on() variant here.
> It makes it more clear that proxy_deactivate() is now out of the
> "donor->blocked_lock" critical section.

The gotcha there is the __clear_task_blocked_on() will assert if we
don't hold the right lock. So this patch is sort of fixing it up to
allow for proper locking without cheating here.


> Either way, no strong feelings.
>
> >       }
> >       return NULL;
> >  }
> > @@ -6619,6 +6619,7 @@ find_proxy_task(struct rq *rq, struct task_struct *donor, struct rq_flags *rf)
> >       int this_cpu = cpu_of(rq);
> >       struct task_struct *p;
> >       struct mutex *mutex;
> > +     enum { FOUND, DEACTIVATE_DONOR } action = FOUND;
>
> nit. If you move that declaration to the top, you can preserve the nice
> reverse xmas arrangement ;)

Yeah, I meant to do that, but just overlooked it. Thanks for pointing it out.


> Apart from those couple of nits, feel free to include:
>
> Reviewed-by: K Prateek Nayak <kprateek.nayak@xxxxxxx>

As always, greatly appreciate your time for the review and feedback here!
Thank you!
-john