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] [thread-next>] [day] [month] [year] [list]

Message-ID: <59E10428-6359-4E0A-BBB2-C98DF01F79BA@nutanix.com>
Date: Thu, 13 Mar 2025 19:38:40 +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



>>> 
>>> 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 :)
> 
> What you mean with "handle the race for offline migration case"?

By offline migration I meant dl_task_offline_migration path which
calls find_lock_later_rq. So unless we think the same race that this
fix is trying to address for push_dl_task can happen for
dl_task_offline_migration, there is one less reason to encapsulate
this in find_lock_later_rq.

> 
> And I am honestly conflicted. I think I like the encapsulation better if
> we can find a solution inside find_lock_later_rq(), as it also aligns
> better with rt.c, but you fear it's more fragile?
> 

Yes I agree that encapsulation in find_lock_later_rq will be ideal
but by keeping it limited to push_dl_task I wanted to keep the change
more targeted to avoid any possible side effect on
dl_task_offline_migration call path.

Let’s say if we go ahead with making the change in find_lock_later_rq
itself then we will have to fallback to current checks for throttled case
and for remaining we will use the task != pick_next_pushable_dl_task(rq)
check. Below is the diff of how it will be:

                /* Retry if something changed. */
                if (double_lock_balance(rq, later_rq)) {
-                       if (unlikely(task_rq(task) != rq ||
+                       if (unlikely(is_migration_disabled(task) ||
                                     !cpumask_test_cpu(later_rq->cpu, &task->cpus_mask) ||
-                                    task_on_cpu(rq, task) ||
-                                    !dl_task(task) ||
-                                    is_migration_disabled(task) ||
-                                    !task_on_rq_queued(task))) {
+                                    (task->dl.dl_throttled &&
+                                      (task_rq(task) != rq ||
+                                       task_on_cpu(rq, task) ||
+                                       !dl_task(task)
+                                       !task_on_rq_queued(task))) ||
+                                    (!task->dl.dl_throttled &&
+                                      task != pick_next_pushable_dl_task(rq)))) {
                                double_unlock_balance(rq, later_rq);
                                later_rq = NULL;
                                break;
 
Let me know your thoughts and I can send v2 patch accordingly.

Thanks,
Harshit