Re: [PATCH 2/2] sched_ext: Sync common and compat headers from the scx repo

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The default value of `__scx_prolog_disables_migration` contradicts its safety documentation and causes dangerous under-reporting.
- [High] Missing ksym guard for `scx_bpf_cpu_rq` causes BPF program load failure on newer kernels.
--

--- Patch [2]: [PATCH 2/2] sched_ext: Sync common and compat headers from the scx repo ---
commit d5556fe1c132d0cef0267d7ea8593fa9b725e9e6
Author: Tejun Heo <[email protected]>

sched_ext: Sync common and compat headers from the scx repo

Sync common.bpf.h, compat.bpf.h and compat.h with the scx repo. This 
includes updates to 64-bit enum recovery, is_migration_disabled() logic, 
and backwards compatibility for scx_bpf_cpu_curr and scx_bpf_cpu_rq.

Link: https://lore.kernel.org/all/[email protected]

> diff --git a/tools/sched_ext/include/scx/common.bpf.h b/tools/sched_ext/include/scx/common.bpf.h
> index 979d4cabfaf98..76f5e025e1076 100644
> --- a/tools/sched_ext/include/scx/common.bpf.h
> +++ b/tools/sched_ext/include/scx/common.bpf.h

[ ... ]

> @@ -527,32 +529,103 @@ static __always_inline const struct cpumask *cast_mask(struct bpf_cpumask *mask)
>  	return (const struct cpumask *)mask;
>  }
>  
> +/*
> + * True if the non-sleepable BPF trampoline prolog (__bpf_prog_enter) calls
> + * migrate_disable() for the current task. Recorded once by
> + * scx_lib_init_probe, an fentry program on bpf_scx_reg() that fires during
> + * the natural scheduler-attach call chain (auto-attached by scx_ops_attach!).
> + *
> + * Defaults to true (conservative). Over-reporting in is_migration_disabled()
> + * causes local-only dispatch, which is safe. Under-reporting can crash the
> + * scheduler, so we err high if the probe somehow fails to run.
> + */
> +bool __scx_prolog_disables_migration __weak = true;

[Severity: High]
Does this default value actually under-report if the probe fails to run?

The documentation indicates that the fallback should err high (over-report)
to avoid crashing the scheduler. However, looking at the slow path in
is_migration_disabled() below, when __scx_prolog_disables_migration is true,
the macro evaluates bpf_get_current_task_btf() != p. This causes it to
return false for the current task, which means it incorrectly assumes
migration is not disabled (under-reporting).

Should the default value of __scx_prolog_disables_migration be false
instead, so that it falls back to returning true?

> +
> +/*
> + * scx_lib_init_probe - non-sleepable prolog probe.

[ ... ]

>  static inline bool is_migration_disabled(const struct task_struct *p)
>  {

[ ... ]

> +	if (bpf_core_field_exists(p->migration_disabled)) {
> -		if (p->migration_disabled == 1)
> -			return bpf_get_current_task_btf() != p;
> -		else
> -			return p->migration_disabled;
> +		if (p->migration_disabled == 1) {
> +			/* Fast path: prolog always disables migration */
> +			if (CONFIG_PREEMPT_RCU)
> +				return bpf_get_current_task_btf() != p;
> +			/* Fast path: prolog never disables migration */
> +			if (LINUX_KERNEL_VERSION >= KERNEL_VERSION(6, 18, 0))
> +				return true;
> +			/* Slow path: pre-v6.18, !PREEMPT_RCU - use runtime flag */
> +			return __scx_prolog_disables_migration ?
> +			       bpf_get_current_task_btf() != p : true;
> +		}
> +		return p->migration_disabled;
>  	}
>  	return false;
>  }

[ ... ]

> diff --git a/tools/sched_ext/include/scx/compat.bpf.h b/tools/sched_ext/include/scx/compat.bpf.h
> index 3ab642f92c8a5..6944221f96cc0 100644
> --- a/tools/sched_ext/include/scx/compat.bpf.h
> +++ b/tools/sched_ext/include/scx/compat.bpf.h

[ ... ]

> @@ -238,6 +243,26 @@ static inline bool __COMPAT_is_enq_cpu_selected(u64 enq_flags)
>  	 scx_bpf_pick_any_cpu_node(cpus_allowed, node, flags) :			\
>  	 scx_bpf_pick_any_cpu(cpus_allowed, flags))
>  
> +/*
> + * v6.18: Add a helper to retrieve the current task running on a CPU.
> + *
> + * The kernel tree dropped this helper and scx_bpf_cpu_rq(), but schedulers in
> + * this tree still support pre-v6.18 kernels where scx_bpf_cpu_curr() doesn't
> + * resolve and the scx_bpf_cpu_rq() fallback still exists. Keep it until
> + * pre-v6.18 kernels fall out of the support window.
> + */
> +static inline struct task_struct *__COMPAT_scx_bpf_cpu_curr(int cpu)
> +{
> +	struct rq *rq;
> +
> +	if (bpf_ksym_exists(scx_bpf_cpu_curr))
> +		return scx_bpf_cpu_curr(cpu);
> +
> +	rq = scx_bpf_cpu_rq(cpu);

[Severity: High]
Will this unconditionally call scx_bpf_cpu_rq() on newer kernels?

If both scx_bpf_cpu_curr and scx_bpf_cpu_rq have been dropped from the kernel
tree, on modern kernels bpf_ksym_exists(scx_bpf_cpu_curr) evaluates to false
and the fallback path is taken.

Because the call to scx_bpf_cpu_rq() is not guarded by its own
bpf_ksym_exists() check, won't libbpf poison the missing call and cause the
BPF verifier to reject the program on newer kernels? Should we guard the
fallback call as well?

> +
> +	return rq ? rq->curr : NULL;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2