Re: [PATCH bpf-next 4/5] selftests/bpf: Add arena-based bitmap data structure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `struct bitmap` uses a `u64` array, but bitwise helper macros rely on `sizeof(long)`, causing a severe memory layout and indexing mismatch between 64-bit BPF and 32-bit user-space.
- [High] `bmp_set_bit` and `bmp_clear_bit` perform non-atomic read-modify-write operations, leading to lost updates under concurrent access.
- [High] `bmp_alloc` incorrectly rejects bitmap sizes that are not exact multiples of `BITS_PER_LONG`, completely breaking allocations for standard cpumask sizes.
--

commit e05abd6424c958e46d415ca79df966bf83a06a24
Author: Emil Tsalapatis <emil@etsalapatis.com>

selftests/bpf: Add arena-based bitmap data structure

This commit adds an arena-based word-aligned bitmap data structure.
The structure is intended to be used as a building block for features
like sched-ext to represent cpumask structures.

> diff --git a/tools/testing/selftests/bpf/libarena/include/libarena/bitmap.h b/tools/testing/selftests/bpf/libarena/include/libarena/bitmap.h
> new file mode 100644
> index 0000000000000..11623a82e66a5
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/libarena/include/libarena/bitmap.h
> @@ -0,0 +1,31 @@
> +#pragma once
> +
> +#define BITS_PER_BYTE		8
> +#define BYTES_TO_BITS(nb)	((nb) * BITS_PER_BYTE)
> +
> +#define BITS_PER_LONG		(sizeof(long) * BITS_PER_BYTE)
> +#define BITS_TO_LONGS(nr)	(((nr) + BITS_PER_LONG - 1) / BITS_PER_LONG)
> +#define BIT_MASK(nr)		(1UL << ((nr) % BITS_PER_LONG))
> +#define BIT_WORD(nr)		((nr) / BITS_PER_LONG)
> +
> +struct bitmap {
> +	u64 bits[0];
> +};

[Severity: High]
Since struct bitmap defines its storage as a fixed-width u64 array, will
using sizeof(long) and 1UL in BIT_WORD and BIT_MASK cause a mismatch between
32-bit user-space and 64-bit BPF?

On a 32-bit user-space process, BITS_PER_LONG is 32. Accessing bit 32 would
compute BIT_WORD(32) = 1, pointing to bits 64-127 of the u64 array, while
the 64-bit BPF program would correctly calculate BIT_WORD(32) = 0.

Should these macros use fixed-width indexing constants like sizeof(u64) * 8
and 1ULL instead to maintain the shared memory contract?

[ ... ]

> diff --git a/tools/testing/selftests/bpf/libarena/src/bitmap.bpf.c b/tools/testing/selftests/bpf/libarena/src/bitmap.bpf.c
> new file mode 100644
> index 0000000000000..a8ca814aa0c85
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/libarena/src/bitmap.bpf.c
[ ... ]
> +__weak
> +struct bitmap __arena *bmp_alloc(size_t bits)
> +{
> +	struct bitmap __arena *bmp;
> +	size_t size = BITS_TO_LONGS(bits) * sizeof(bmp->bits[0]);
> +
> +	/* Assume long-aligned masks. */
> +	if (bits % BITS_PER_LONG)
> +		return NULL;

[Severity: High]
Does this logic artificially reject valid unaligned bit counts, preventing
its use for standard cpumask sizes where nr_cpu_ids is not a multiple of 64?

The allocation size is already safely rounded up to the nearest word
boundary via BITS_TO_LONGS(bits), and trailing bits seem to be handled cleanly
by operations like bmp_last_word_mask.

Could this check be removed so callers aren't forced to artificially round up
requested bits?

[ ... ]

> +__weak
> +void bmp_set_bit(u32 bit, struct bitmap __arena *bmp)
> +{
> +	bmp->bits[BIT_WORD(bit)] |= BIT_MASK(bit);
> +}
> +
> +__weak
> +void bmp_clear_bit(u32 bit, struct bitmap __arena *bmp)
> +{
> +	bmp->bits[BIT_WORD(bit)] &= ~BIT_MASK(bit);
> +}

[Severity: High]
Will these non-atomic read-modify-write operations race if multiple CPUs
concurrently set or clear different bits within the same 64-bit word?

Since the bitmap is intended for concurrent structures like sched-ext
cpumasks, updates could be silently clobbered.

Should these functions use atomic built-ins such as __sync_fetch_and_or
and __sync_fetch_and_and?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618085626.19633-1-emil@etsalapatis.com?part=4