linux-kernel - Re: [PATCH] sched/rt: Fix race in push_rt

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: <9C390C10-8741-4992-8E29-303C907C8C00@nutanix.com>
Date: Tue, 11 Feb 2025 22:24:56 +0000
From: Harshit Agarwal <harshit@...anix.com>
To: Steven Rostedt <rostedt@...dmis.org>
CC: Ingo Molnar <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot
	<vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        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>,
        Jon Kohler <jon@...anix.com>,
        Gauri
 Patwardhan <gauri.patwardhan@...anix.com>,
        Rahul Chunduru
	<rahul.chunduru@...anix.com>,
        Will Ton <william.ton@...anix.com>
Subject: Re: [PATCH] sched/rt: Fix race in push_rt_task



> On Feb 11, 2025, at 1:39 PM, Steven Rostedt <rostedt@...dmis.org> wrote:
> 
> On Tue, 11 Feb 2025 21:08:22 +0000
> Harshit Agarwal <harshit@...anix.com> wrote:
> 
>> Thanks Steve for taking a look. Yes we should ideally remove any conditions that are 
>> subsumed by task != pick_next_pushable_task condition, however I am not sure if anyone (say ttwu())
>> will try to modify p’s state without removing it from the pushable tasks list first. In such a case
>> pick_next_pushable_task will still pick p again and then it will match and will proceed to do the migration
>> while ttwu() is trying to wake it up. Most likely, some asserts like !task_on_rq_queued etc will be hit in
>> pick_next_pushable_task as soon as p is picked. If we can be sure that p’s state is modified by others
>> only after it being removed from pushable tasks list of this CPU then it should be safe to remove it
>> else not.
> 
> Note that all tasks on the pick_next_pushable list is in the running state.
> Nothing should be trying to wake it up while its on that list. That is, if
> p is still on the pushable tasks then everything should be still fine.
> Especially now that the rq locks are still held again.
> 
> I think that is the only check we need. The is_migration_disabled() check
> should probably be checked earlier, as the only way it could be set is if
> the current task preempted it. If it had migrated, and migrated back, it
> couldn't have that set on this current CPU otherwise it would not have
> migrated.
> 
> Here's the current checks again:
> 
> if (unlikely(task_rq(task) != rq ||
>     !cpumask_test_cpu(lowest_rq->cpu, &task->cpus_mask) ||
>     task_on_cpu(rq, task) ||
>     !rt_task(task) ||
>     is_migration_disabled(task) ||
>     !task_on_rq_queued(task))) {
> 
> 
> Let's look at pick_next_pushable_task():
> 
> p = plist_first_entry(&rq->rt.pushable_tasks,
>      struct task_struct, pushable_tasks);
> 
> BUG_ON(rq->cpu != task_cpu(p));
> BUG_ON(task_current(rq, p));
> BUG_ON(task_current_donor(rq, p));
> BUG_ON(p->nr_cpus_allowed <= 1);
> 
> BUG_ON(!task_on_rq_queued(p));
> BUG_ON(!rt_task(p));
> 
> If task_rq(task) != rq is true, then BUG_ON(rq->cpu != task_cpu(p)) would trigger.
> 
>  !cpumask_test_cpu(lowest_rq->cpu, &task->cpus_mask
> 
> We still need that check, to make sure the CPU we picked is in the task's affinity.
> 
> If task_on_cpu(rq, task) is true, then BUG_ON(task_current(rq, p)) would trigger.
> 
> If !rt_task(task) is true then BUG_ON(!rt_task(p)) would trigger.
> 
>   is_migration_disabled(task)
> 
> Is still needed, but could possibly be moved up? (unrelated change though)
> 
> If !task_on_rq_queued(task) is true then BUG_ON(!task_on_rq_queued(p)) would trigger.
> 
> Thus, I think we can change that condition to just:
> 
> if (is_migration_disabled(task) ||
>    !cpumask_test_cpu(lowest_rq->cpu, &task->cpus_mask) ||
>    task != pick_next_pushable_task(rq)) {
> 
> I moved the is_migration_disabled() up as that's the quickest check. If
> that's true, no need to test the other conditions.
> 
> -- Steve

Makes sense and thanks for all the details. This simplifies it. 
I will make this change as per your suggestion and send the
updated patch after testing it.

Regards,
Harshit