Re: [PATCH v4 sched_ext/for-7.3 21/40] sched_ext: Add per-shard cap delegation for sub-schedulers

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing validation for unallocated `pshard` arrays and missing arena maps in `scx_bpf_sub_grant()`, `scx_bpf_sub_revoke()`, and `scx_bpf_sub_caps()` leads to NULL pointer dereferences and invalid memory access Oops.
--

--- Patch [21]: [PATCH v4 sched_ext/for-7.3 21/40] sched_ext: Add per-shard cap delegation for sub-schedulers ---
commit 809108a1ad8d6e1bc475090c3e5ff6fba86f6bad
Author: Tejun Heo <[email protected]>
sched_ext: Add per-shard cap delegation for sub-schedulers

Caps are per-cid permissions parents delegate to direct children via
scx_bpf_sub_grant() / scx_bpf_sub_revoke(). A child's cap set is always a
subset of its parent's. Sub-scheds check their caps locally, and cross-sched
communication is needed only when the delegation set itself changes.

> diff --git a/kernel/sched/ext/sub.c b/kernel/sched/ext/sub.c
> index 8d98250936497..d0ffc3a8550cf 100644
> --- a/kernel/sched/ext/sub.c
> +++ b/kernel/sched/ext/sub.c
[ ... ]
> @@ -760,6 +808,284 @@ __bpf_kfunc bool scx_bpf_sub_dispatch(u64 cgroup_id, const struct bpf_prog_aux *
>  				  true);
>  }
>  
> +/* Validate common inputs. On success, *parent_out and *child_out are set. */
> +static s32 sub_cap_preamble(u64 cgroup_id, u64 caps, const struct bpf_prog_aux *aux,
> +			    struct scx_sched **parent_out, struct scx_sched **child_out)
> +{
[ ... ]
> +	*parent_out = parent;
> +	*child_out = child;
> +	return 0;
> +}
> +
> +/**
> + * scx_bpf_sub_grant - Grant @caps on @cmask__ign's cids to a direct child
[ ... ]
> +__bpf_kfunc s32 scx_bpf_sub_grant(u64 cgroup_id, u64 caps,
> +				  const struct scx_cmask *cmask__ign,
> +				  struct scx_cmask *denied_out__ign,
> +				  const struct bpf_prog_aux *aux)
> +{
[ ... ]
> +	/* apply the grant one shard at a time */
> +	for (si = ref.shard_first; si < ref.shard_end; si++) {
> +		SCX_CMASK_DEFINE_SHARD(slice, 0, SCX_CID_SHARD_MAX_CPUS);
> +		struct scx_pshard *pps = parent->pshard[si];
> +		struct scx_pshard *cps = child->pshard[si];

[Severity: High]
Does this code dereference a NULL pointer if the child scheduler was created
without a BPF arena?

If the child scheduler has no arena pool, scx_alloc_pshards() skips allocating
the pshard array and leaves it as NULL. When scx_bpf_sub_grant() accesses
child->pshard[si], it would attempt to dereference that NULL pointer.

> +		u32 cap_bit;
> +
> +		scx_cmask_ref_shard(&ref, si, slice);
> +		if (scx_cmask_empty(slice))
> +			continue;
[ ... ]
> +__bpf_kfunc void scx_bpf_sub_revoke(u64 cgroup_id, u64 caps,
> +				    const struct scx_cmask *cmask__ign,
> +				    const struct bpf_prog_aux *aux)
> +{
[ ... ]
> +		guard(raw_spinlock)(&parent->pshard[si]->lock);
> +		pos = scx_next_descendant_pre(NULL, child);
> +		while (pos) {
> +			struct scx_pshard *ps = pos->pshard[si];

[Severity: High]
Can pos->pshard be NULL here if one of the descendant schedulers was created
without an arena map?

If a descendant scheduler lacks an arena, pos->pshard is NULL, leading to a
NULL pointer dereference when accessing pos->pshard[si].

> +			u64 revoked_caps = 0;
> +			u32 cap_bit;
> +
> +			scoped_guard (raw_spinlock_nested, &ps->lock) {
> +				scx_for_each_cap_bit(cap_bit, caps) {
[ ... ]
> +__bpf_kfunc s32 scx_bpf_sub_caps(u64 cgroup_id, u64 caps, struct scx_cmask *out__ign,
> +				 const struct bpf_prog_aux *aux)
> +{
[ ... ]
> +	pshard = READ_ONCE(target->pshard);
> +	if (unlikely(!pshard)) {
> +		scx_error(sch, "scx_bpf_sub_caps() called before caps storage is initialized");
> +		return -ENODEV;
> +	}
> +
> +	ret = scx_cmask_ref_init(sch, out__ign, &ref);

[Severity: High]
Can this lead to an invalid memory access oops if the calling scheduler (sch)
has no arena?

If sch does not have an arena map, its arena_kern_base is 0. 
scx_cmask_ref_init() uses this base when resolving the out__ign arena pointer,
resulting in a read from a user-space address and causing a kernel page fault.

> +	if (ret) {
> +		scx_error(sch, "invalid out (%d)", ret);
> +		return ret;
> +	}

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