linux-kernel - Re: [PATCH] sched/deadline: Fix race in push_dl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <8B627F86-EF5F-4EA2-96F4-E47B0B3CAD38@nutanix.com>
Date: Wed, 12 Mar 2025 18:46:18 +0000
From: Harshit Agarwal <harshit@...anix.com>
To: Juri Lelli <juri.lelli@...hat.com>
CC: Ingo Molnar <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann
	<dietmar.eggemann@....com>,
        Steven Rostedt <rostedt@...dmis.org>, Ben Segall
	<bsegall@...gle.com>,
        Mel Gorman <mgorman@...e.de>,
        Valentin Schneider
	<vschneid@...hat.com>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org"
	<stable@...r.kernel.org>
Subject: Re: [PATCH] sched/deadline: Fix race in push_dl_task

Thanks Juri, for taking a look.

> On Mar 12, 2025, at 2:42 AM, Juri Lelli <juri.lelli@...hat.com> wrote:
> 
> Hi Harshit,
> 
> Thanks for this!
> 
> I don't think we want this kind of URLs in the changelog, as URL might
> disappear while the history remains (at least usually a little longer
> :). Maybe you could add a very condensed version of the description of
> the problem you have on the other fix?

Sorry about this and thanks for pointing it out. I will fix it in the next version
of the patch.

> 
>> In this fix we bail out or retry in the push_dl_task, if the task is no
>> longer at the head of pushable tasks list because this list changed
>> while trying to lock the runqueue of the other CPU.
>> 
>> Signed-off-by: Harshit Agarwal <harshit@...anix.com>
>> Cc: stable@...r.kernel.org
>> ---
>> kernel/sched/deadline.c | 25 +++++++++++++++++++++----
>> 1 file changed, 21 insertions(+), 4 deletions(-)
>> 
>> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
>> index 38e4537790af..c5048969c640 100644
>> --- a/kernel/sched/deadline.c
>> +++ b/kernel/sched/deadline.c
>> @@ -2704,6 +2704,7 @@ static int push_dl_task(struct rq *rq)
>> {
>> struct task_struct *next_task;
>> struct rq *later_rq;
>> + struct task_struct *task;
>> int ret = 0;
>> 
>> next_task = pick_next_pushable_dl_task(rq);
>> @@ -2734,15 +2735,30 @@ static int push_dl_task(struct rq *rq)
>> 
>> /* Will lock the rq it'll find */
>> later_rq = find_lock_later_rq(next_task, rq);
>> - if (!later_rq) {
>> - struct task_struct *task;
>> + task = pick_next_pushable_dl_task(rq);
>> + if (later_rq && (!task || task != next_task)) {
>> + /*
>> + * We must check all this again, since
>> + * find_lock_later_rq releases rq->lock and it is
>> + * then possible that next_task has migrated and
>> + * is no longer at the head of the pushable list.
>> + */
>> + double_unlock_balance(rq, later_rq);
>> + if (!task) {
>> + /* No more tasks */
>> + goto out;
>> + }
>> 
>> + put_task_struct(next_task);
>> + next_task = task;
>> + goto retry;
> 
> I fear we might hit a pathological condition that can lead us into a
> never ending (or very long) loop. find_lock_later_rq() tries to find a
> later_rq for at most DL_MAX_TRIES and it bails out if it can't.

This pathological case exists today as well and will be there even
if we move this check inside find_lock_later_rq. This check is just
broadening the scenarios where we would retry, where we would
have panicked otherwise (the bug).
If this check is moved inside find_lock_later_rq then function will
return null and then the caller here will do the same which is retry
or bail out if no tasks are available. Specifically, tt will execute
the if (!later_rq) block here.
The number of retries will be bound by the number of tasks in 
the pushable tasks list.

> 
> Maybe to discern between find_lock_later_rq() callers we can use
> dl_throttled flag in dl_se and still implement the fix in find_lock_
> later_rq()? I.e., fix similar to the rt.c patch in case the task is not
> throttled (so caller is push_dl_task()) and not rely on pick_next_
> pushable_dl_task() if the task is throttled.
> 

Sure I can do this as well but like I mentioned above I don’t think
it will be any different than this patch unless we want to
handle the race for offline migration case or if you prefer
this in find_lock_later_rq just to keep it more inline with the rt
patch. I just found the current approach to be less risky :)

Let me know your thoughts.

Regards,
Harshit