Re: [RFC PATCH v3 11/13] lib/sbm: Add helpers to allocate, set, clear, and traverse the bits on sbm

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] sched/fair: out-of-bounds memory corruption in __sbm_op() during CPU hotplug
- [High] lib/sbm: premature decrement of max index in free_sbm_index() hides active CPUs
- [Low] lib/sbm: sbm_alloc() massively over-allocates cacheline-aligned memory
--

commit 8d84c933216fc8041e9a487dfb8a2121359eb99f
Author: Peter Zijlstra <peterz@infradead.org>

lib/sbm: Add helpers to allocate, set, clear, and traverse the bits on sbm

This commit introduces a set of helpers to allocate a sparsebitmap (sbm) 
structure based on architecture-configured lengths. It provides macros and 
inline functions to set, clear, test, and iterate over bits in the sbm array.

> diff --git a/include/linux/sbm.h b/include/linux/sbm.h
> --- a/include/linux/sbm.h
> +++ b/include/linux/sbm.h
[ ... ]
> @@ -9,12 +11,95 @@
> +#define __sbm_op(sbm, func)				\
> +({							\
> +	int idx = sbm_cpu_to_idx(cpu);			\
> +	int nr = idx >> __sbm_shift;			\
> +	int bit = idx & __sbm_mask;			\
> +							\
> +	func(bit, &sbm[nr].bitmap);			\
> +})

[Severity: High]
Can this sequence lead to out-of-bounds memory corruption during CPU
hotplug?

When a CPU is brought offline, free_sbm_index() sets its index to -1.
If the dying CPU continues to run the idle task to tear down, it can
trigger tick_nohz_idle_stop_tick() and consequently call sbm_cpu_set().

Because __sbm_op lacks bounds checking on the mapped idx, computing
idx = -1 results in nr = -1, which then unconditionally executes func()
on &sbm[-1].bitmap.

> diff --git a/lib/sbm.c b/lib/sbm.c
> --- a/lib/sbm.c
> +++ b/lib/sbm.c
[ ... ]
> @@ -45,6 +51,44 @@ int sbm_idx_to_cpu(int idx)
[ ... ]
> +struct sbm *sbm_alloc(void)
> +{
> +	return kzalloc_objs(struct sbm, sbm_max_threads_per_instance * sbm_num_instance);
> +}

[Severity: Low]
Does this code allocate significantly more memory than needed?

Each struct sbm encapsulates a full bitmask that accommodates
sbm_max_threads_per_instance CPUs. Since the index calculation
nr = idx >> __sbm_shift directly corresponds to the leaf index, the
array only seems to require exactly sbm_num_instance elements.

Allocating sbm_max_threads_per_instance * sbm_num_instance elements
appears to request considerably more cacheline-aligned memory than is
actually necessary to map all bits.

[ ... ]
> @@ -127,6 +173,15 @@ void free_sbm_index(int cpu)
[ ... ]
>  	if (find_first_bit(&__sbm_idx_metadata[leaf].allocated_mask, BITS_PER_LONG) ==
>  	    BITS_PER_LONG)
>  		__sbm_idx_metadata[leaf].instance_id = -1;
> +
> +	if (leaf == sbm_max_populated_index) {
> +		for (idx = leaf - 1; idx > -1; idx--) {
> +			if (__sbm_idx_metadata[idx].instance_id != -1)
> +				break;
> +		}
> +
> +		WRITE_ONCE(sbm_max_populated_index, max(idx, 0));
> +	}
>  }

[Severity: High]
Could this prematurely shrink the maximum index and strand active CPUs?

When clearing a bit in a leaf's allocated_mask, if the leaf matches
sbm_max_populated_index, this loop unconditionally scans backward to find
the next non-empty leaf and overwrites sbm_max_populated_index.

However, it does this without verifying if the current maximum leaf
actually became completely empty (instance_id == -1). If other CPUs
remain actively allocated in that same leaf, they appear to be
immediately hidden since the global max index is truncated below them.

This would cause functions like sbm_empty() and sbm_find_next_bit() to
permanently ignore these active CPUs during NOHZ idle load balancing.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001192849.74788-1-kprateek.nayak@amd.com?part=11