Re: [PATCH v2 2/3] sched_ext: Track bits[] storage size in struct scx_cmask

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The `__SCX_CMASK_DEFINE` macro leaves the `bits[]` flexible array uninitialized, causing subsequent operations to operate on stack garbage.
- [Medium] The BPF helper `__cmask_init()` returns early on failure without initializing the structure, exposing uninitialized memory to the executing BPF program.
- [Medium] BPF `__cmask_init()` does not validate `alloc_cids` against `CMASK_MAX_WORDS`, leading to inconsistent state and truncated operations.
- [Medium] Integer wrap-around in `cmask_reframe()` and `scx_cmask_reframe()` leaves tail padding uncleared when `nr_cids` is near `U32_MAX`.
--

commit af0adccc546dd475652f096375ea97f8c951b0f6
Author: Tejun Heo <tj@kernel.org>

sched_ext: Track bits[] storage size in struct scx_cmask

This commit adds tracking of the allocated word count to scx_cmask, splits
the bit-range API into explicit-capacity and tight-capacity helpers, and
provides equivalent BPF mirror functions. It also adds clear and fill
helpers to manage the active-range bits safely.

diff --git a/kernel/sched/ext_cid.h b/kernel/sched/ext_cid.h
--- a/kernel/sched/ext_cid.h
+++ b/kernel/sched/ext_cid.h
[ ... ]
> +static inline void scx_cmask_reframe(struct scx_cmask *m, u32 base, u32 nr_cids)
> +{
> +	if (WARN_ON_ONCE(SCX_CMASK_NR_WORDS(nr_cids) > m->alloc_words))
> +		return;
> +
> +	if (nr_cids) {
> +		u32 last_word = ((base & 63) + nr_cids - 1) / 64;

The commit message states:
"Widen SCX_CMASK_NR_WORDS()/CMASK_NR_WORDS() to compute in u64 so that
@nr_cids near U32_MAX no longer wraps to a small value and bypasses
the bounds check in cmask_reframe()."

Despite this, could the last_word calculation here still wrap around?

If nr_cids is near U32_MAX, ((base & 63) + nr_cids - 1) might overflow the
32-bit u32, resulting in a small last_word value (potentially 0). This would
cause the head word to be cleared again instead of the intended tail word,
leaving tail padding uncleared.

diff --git a/kernel/sched/ext_types.h b/kernel/sched/ext_types.h
--- a/kernel/sched/ext_types.h
+++ b/kernel/sched/ext_types.h
[ ... ]
> +#define __SCX_CMASK_DEFINE(NAME, BASE, NR_CIDS, ALLOC_CIDS)			\
> +	_DEFINE_FLEX(struct scx_cmask, NAME, bits, SCX_CMASK_NR_WORDS(ALLOC_CIDS), \
> +		     = { .base = (BASE),					\
> +			 .nr_cids = (NR_CIDS),					\
> +			 .alloc_words = SCX_CMASK_NR_WORDS(ALLOC_CIDS) })

Does _DEFINE_FLEX with a designated initializer fully zero-initialize the
flexible array members?

In C, initializing specific struct members does not necessarily zero out
the remaining padding or flexible array bytes. The documentation mentions
this macro aliases zero-initialized storage, but without the explicit
memset() that was previously handled by scx_cmask_init(), could the bits
array end up containing stack garbage?

diff --git a/tools/sched_ext/include/scx/cid.bpf.h b/tools/sched_ext/include/scx/cid.bpf.h
--- a/tools/sched_ext/include/scx/cid.bpf.h
+++ b/tools/sched_ext/include/scx/cid.bpf.h
[ ... ]
> +static __always_inline void __cmask_init(struct scx_cmask __arena *m, u32 base,
> +					 u32 nr_cids, u32 alloc_cids)
>  {
> -	u32 nr_words = CMASK_NR_WORDS(nr_cids), i;
> +	u32 alloc_words, i;
> +
> +	if (unlikely(nr_cids > alloc_cids)) {
> +		scx_bpf_error("__cmask_init: nr_cids=%u exceeds alloc_cids=%u",
> +			      nr_cids, alloc_cids);
> +		return;
> +	}

Because scx_bpf_error() schedules an asynchronous abort rather than halting
the BPF program synchronously, will the program continue executing with an
uninitialized mask?

The kernel counterpart __scx_cmask_init() safely clamps the value to
alloc_cids instead of returning early. Would clamping the value or zeroing
the memory here prevent subsequent BPF logic from operating on uninitialized
memory?

> +	alloc_words = CMASK_NR_WORDS(alloc_cids);
>  
>  	m->base = base;
>  	m->nr_cids = nr_cids;
> +	m->alloc_words = alloc_words;
>  
>  	bpf_for(i, 0, CMASK_MAX_WORDS) {
> -		if (i >= nr_words)
> +		if (i >= alloc_words)
>  			break;
>  		m->bits[i] = 0;
>  	}
>  }

If alloc_cids is very large, could alloc_words exceed CMASK_MAX_WORDS?

If so, this initialization loop will stop at CMASK_MAX_WORDS (129), leaving
the remaining words uninitialized. However, m->alloc_words will still record
the larger size. Should there be a validation check against CMASK_MAX_WORDS
to prevent silent truncation in this and other bitmask operations?

[ ... ]
> +static __always_inline void cmask_reframe(struct scx_cmask __arena *m, u32 base, u32 nr_cids)
> +{
> +	if (CMASK_NR_WORDS(nr_cids) > m->alloc_words) {
> +		scx_bpf_error("cmask_reframe: nr_cids=%u exceeds alloc_words=%u",
> +			      nr_cids, m->alloc_words);
> +		return;
> +	}
> +	if (nr_cids) {
> +		u32 last_word = ((base & 63) + nr_cids - 1) / 64;

Similar to the kernel helper scx_cmask_reframe(), could this calculation wrap
around 32-bit limits if nr_cids is near U32_MAX, resulting in last_word being
calculated incorrectly?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519075838.2706712-1-tj@kernel.org?part=2