Re: [PATCH 2/3] sched_ext: Add dispatch transaction API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Double Fetch / TOCTOU vulnerability in `scx_bpf_dsq_insert_commit()` when accessing `args` fields.
- [High] Unprotected state modification (`p->scx.slice`) causing a data race on actively running or migrated tasks.
--

commit c413431466d2cd2f8ef609dabb82ff0b92ea46d2
Author: Cheng-Yang Chou <yphbchou0911@gmail.com>

sched_ext: Add dispatch transaction API

This commit introduces scx_bpf_dsq_insert_begin() and
scx_bpf_dsq_insert_commit() kfuncs to allow a dispatch transaction. This
mechanism extends the race detection window for schedulers that queue tasks
in ops.enqueue() but do not implement properly synchronized ops.dequeue().

Link: https://lore.kernel.org/r/20260203230639.1259869-1-arighi@nvidia.com/

> diff --git a/kernel/sched/ext.c b/kernel/sched/ext.c
> index 0dadda97ef57c..2dfcfc3c885bb 100644
> --- a/kernel/sched/ext.c
> +++ b/kernel/sched/ext.c

[ ... ]

> @@ -8518,6 +8530,92 @@ __bpf_kfunc void scx_bpf_dsq_insert_vtime(struct task_struct *p, u64 dsq_id,

[ ... ]

> +__bpf_kfunc bool scx_bpf_dsq_insert_commit(struct task_struct *p,
> +					    struct scx_bpf_dsq_insert_commit_args *args,
> +					    u64 token,
> +					    const struct bpf_prog_aux *aux)
> +{
> +	struct scx_sched *sch;
> +	u64 enq_flags = args->enq_flags;
> +
> +	guard(rcu)();
> +	sch = scx_prog_sched(aux);
> +	if (unlikely(!sch))
> +		return false;
> +
> +	if (args->dsq_id != SCX_DSQ_LOCAL &&
> +	    (args->dsq_id & SCX_DSQ_LOCAL_ON) != SCX_DSQ_LOCAL_ON) {

Since pointer arguments to BPF kfuncs like args can point to shared memory
such as BPF map values, which can be concurrently modified by userspace or
other BPF programs, is it possible for args->dsq_id to change between reads?

If args->dsq_id changes to a global DSQ ID after this local-only check passes,
could it bypass the constraint during the subsequent scx_dsq_insert_preamble()
and scx_dsq_insert_stage() calls?

> +		scx_error(sch, "scx_bpf_dsq_insert_commit() may only target local DSQs");
> +		return false;
> +	}
> +
> +	if (!scx_dsq_insert_preamble(sch, p, args->dsq_id, &enq_flags))
> +		return false;
> +
> +	if (args->slice)
> +		p->scx.slice = args->slice;
> +	else
> +		p->scx.slice = p->scx.slice ?: 1;

Could a concurrent modification to args->slice between the condition check
and the assignment result in setting the task's slice to 0, bypassing the
fallback logic?

Would it be safer to fetch args->dsq_id and args->slice exactly once into
local variables at the beginning of the function, similar to how
args->enq_flags is handled?

Additionally, since scx_bpf_dsq_insert_commit() is intended for schedulers
where p may have been dequeued or migrated to another CPU after the dispatch
token was captured, the current CPU might not hold p's runqueue lock here.

Could modifying p->scx.slice without synchronization cause a data race if
the task is actively running or queued on another CPU?

Even if the stale token is later detected and discarded by finish_dispatch(),
it appears the corruption to p->scx.slice would already have occurred. Is
there a way to defer this write or synchronize it so the active state is not
modified?

> +
> +	scx_dsq_insert_stage(sch, p, args->dsq_id, enq_flags, token);
> +
> +	return true;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515172807.254804-1-yphbchou0911@gmail.com?part=2