Re: [PATCH 03/12] sched/deadline: Do not BUG() on a deadline task with no active CPU allowed

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] sched/deadline: stale root domain in dl_task_root_domain_cpu() fallback
--

commit 1ac76c29bedf1013e2c606590748a52f51b1170b
Author: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>

sched/deadline: Do not BUG() on a deadline task with no active CPU allowed

This commit replaces the BUG_ON(cpu >= nr_cpu_ids) in
dl_task_root_domain_cpu() with a fallback to task_cpu(p) when a deadline
task's cpus_mask contains no active CPUs.

> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index dadda1a528ec6..30a52e42c26d8 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -3338,13 +3338,19 @@ static void task_woken_dl(struct rq *rq, struct task_struct *p)
>   * current cpus_mask (not yet updated to the new mask) to find the
>   * actual root domain. Use cpus_mask rather than cpus_ptr, which
>   * may be temporarily narrowed by migrate_disable().
> + *
> + * The mask has no active CPU left when the CPUs it allows went offline,
> + * which the affinity of a deadline task allows with admission control
> + * off, and that of a task boosted into the deadline class by priority
> + * inheritance always: fall back to the CPU the task last ran on.
>   */
>  static unsigned int dl_task_root_domain_cpu(struct task_struct *p)
>  {
>  	unsigned int cpu;
>  
>  	cpu = cpumask_first_and(&p->cpus_mask, cpu_active_mask);
> -	BUG_ON(cpu >= nr_cpu_ids);
> +	if (cpu >= nr_cpu_ids)
> +		cpu = task_cpu(p);
>  	return cpu;
>  }

[Severity: High]
When a deadline task's cpus_mask has no active CPUs left, can falling back
to task_cpu(p) in dl_task_root_domain_cpu() return an offline or stale CPU
whose root domain no longer holds p->dl.dl_bw?

For example, with admission control disabled (sched_rt_runtime_us == -1),
suppose a SCHED_DEADLINE task p in top_cpuset is pinned via
sched_setaffinity() to CPU 1 (so p->cpus_mask is {1}), runs on CPU 1
(task_cpu(p) == 1), and blocks.

When CPU 1 goes offline via sched_cpu_deactivate(), cpuset_handle_hotplug()
does not rewrite p->cpus_mask for tasks in top_cpuset, so p->cpus_mask
remains {1} and task_cpu(p) remains 1. During the domain rebuild:

  partition_sched_domains_locked()
    detach_destroy_domains() -> attaches offline CPU 1 to def_root_domain
    build_sched_domains() -> attaches remaining active CPUs to rd_active
    dl_rebuild_rd_accounting()
      dl_clear_root_domain() -> sets def_root_domain.dl_bw.total_bw = 0
      dl_add_task_root_domain(p)

In kernel/sched/deadline.c:dl_add_task_root_domain():

	msk = this_cpu_cpumask_var_ptr(local_cpu_mask_dl);
	dl_get_task_effective_cpus(p, msk);
	cpu = cpumask_first_and(cpu_active_mask, msk);
	BUG_ON(cpu >= nr_cpu_ids);
	rq = cpu_rq(cpu);
	dl_b = &rq->rd->dl_bw;

	raw_spin_lock(&dl_b->lock);
	__dl_add(dl_b, p->dl.dl_bw, cpumask_weight(rq->rd->span));

dl_get_task_effective_cpus() queries cpuset_cpus_allowed_locked(), so p's
bandwidth is accounted into rd_active->dl_bw.total_bw, whereas
cpu_rq(task_cpu(p))->rd is def_root_domain. (Similarly, if a sleeping
deadline task last ran on CPU A in partition A, was moved to a 1-CPU cpuset
on CPU B without waking up, and CPU B goes offline, task_cpu(p) still
points to CPU A in partition A.)

Because dl_task_root_domain_cpu(p) returns task_cpu(p) (CPU 1) while
p->cpus_mask has no active CPUs:

1. In switched_from_dl(), if p changes policy away from SCHED_DEADLINE
   while sleeping, __dl_sub() subtracts p->dl.dl_bw from
   def_root_domain.dl_bw.total_bw (underflowing it from 0) instead of
   rd_active->dl_bw.total_bw, leaking p->dl.dl_bw in rd_active.

2. In dl_task_needs_bw_move() and set_cpus_allowed_dl():

	cpu = dl_task_root_domain_cpu(p);
	return !cpumask_intersects(cpu_rq(cpu)->rd->span, new_mask);

   cpu_rq(1)->rd->span is def_root_domain.span, which contains only
   offline CPUs and does not intersect any new_mask of active CPUs.
   Updating p's affinity to active CPUs in rd_active via
   sched_setaffinity() or cpuset_cpus_allowed_fallback() causes
   dl_task_needs_bw_move() to return true and set_cpus_allowed_dl() to
   subtract p->dl.dl_bw from def_root_domain.dl_bw.total_bw. Moving p via
   cpuset_can_attach() and cpuset_attach_task() to a child cpuset in the
   same root domain rd_active causes cpuset_reserve_dl_bw() to add
   p->dl.dl_bw a second time to rd_active->dl_bw.total_bw while
   set_cpus_allowed_dl() subtracts from def_root_domain; moving p to a
   different partition subtracts from def_root_domain instead of rd_active,
   leaking p->dl.dl_bw in rd_active.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793804635.git.mathieu.desnoyers@xxxxxxxxxxxx?part=3