Re: [PATCH net-next v5 2/7] mm/slab: Let kmem_buckets_create() take an alignment

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] When kmem_buckets_create() gets a non-zero @align, it uses
  that value as the cache alignment instead of the alignment of the…
- [Low] The new kernel-doc for kmem_buckets_create() (mm/slab_common.c
  lines 418-420, copied to include/linux/slab.h and…

--

mm/slab: Let kmem_buckets_create() take an alignment

This commit adds an align argument to kmem_buckets_create(). When align is
0, each bucket cache takes the alignment of the kmalloc cache of the same
size, so bucket allocations match kmalloc() when slab metadata is enabled.
Both existing callers now pass 0.

> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index b93fdb5cb0219..71926f726a07a 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -415,6 +415,9 @@ static struct kmem_cache *kmem_buckets_cache __ro_after_init;
>   *			 allocations via kmem_buckets_alloc()
>   * @name: A prefix string which is used in /proc/slabinfo to identify this
>   *	  cache. The individual caches with have their sizes as the suffix.
> + * @align: The required alignment for the objects, or 0 to give each cache
> + *	   the alignment of the kmalloc cache of the same size, as a caller
> + *	   moving from kmalloc() may depend on.

[Severity: Low]
The new kernel-doc calls @align "The required alignment for the objects".
Does every path covered by this doc honour it?

With CONFIG_SLAB_BUCKETS=n, kmem_buckets_create() returns without looking
at align:

	if (!IS_ENABLED(CONFIG_SLAB_BUCKETS))
		return ZERO_SIZE_PTR;

PASS_BUCKET_PARAM(_b) is also NULL in that config, so allocations are served
from the general caches.

When set creation fails, the doc says callers can keep using the NULL
result and that allocations "will fall back to kmalloc()". The caller in
ipc/msgutil.c:init_msg_buckets() never checks the return value.

In both cases, kmalloc_slab() does this:

	if (!b)
		b = &kmalloc_caches[type];

The object then gets only kmalloc's natural alignment for that size. Take
align=256 with 64-byte objects, which is the case in the series' kunit test
(the test skips itself when !CONFIG_SLAB_BUCKETS). That caller would get
less-aligned memory on some configs and see no warning.

Later in the series, allocations of kmalloc types the set does not cover
also go to the general caches and lose align in the same way.

Should the kernel-doc say that a non-zero align only applies when the set
was actually created? Or should these fallback paths honour it?

>   * @flags: SLAB flags (see kmem_cache_create() for details).
>   * @useroffset: Starting offset within an allocation that may be copied
>   *		to/from userspace.

[ ... ]

> @@ -487,7 +491,8 @@ kmem_buckets *kmem_buckets_create(const char *name, slab_flags_t flags,
>  			if (WARN_ON(!cache_name))
>  				goto fail;
>  			(*b)[aligned_idx] = kmem_cache_create_usercopy(cache_name, size,
> -					0, flags, cache_useroffset,
> +					align ?: kmalloc_caches[KMALLOC_NORMAL][idx]->align,

[Severity: Medium]
A non-zero align replaces the alignment of the mirrored kmalloc cache
instead of being combined with it. Below this, calculate_alignment() only
raises it to arch_slab_minalign():

	align = max(align, arch_slab_minalign());

Nothing raises it to ARCH_KMALLOC_MINALIGN, ARCH_DMA_MINALIGN, or the
power-of-two size alignment that create_boot_cache() gives kmalloc caches.

Could a caller that passes a small non-zero align end up with weaker
alignment than plain kmalloc()?

kmem_buckets_alloc() goes through __kmalloc_node_noprof(), which is
declared __assume_kmalloc_alignment. Some architectures leave
ARCH_KMALLOC_MINALIGN at its default of ARCH_DMA_MINALIGN. On those, an
explicit align below that value, with KASAN, SLUB_DEBUG_ON or slab_debug=
enabled, would break the compiler's alignment assumption.

dma_kmalloc_size_aligned() also decides whether to bounce from the size
alone:

	/*
	 * Larger kmalloc() sizes are guaranteed to be aligned to
	 * ARCH_DMA_MINALIGN.
	 */
	if (size >= 2 * ARCH_DMA_MINALIGN ||

For example, take align=64 on arm64 (ARCH_DMA_MINALIGN=128) with slab_debug
enabled. The 2048-byte objects would be only 64-byte aligned, and swiotlb
would never bounce them. Is that the same non-coherent DMA problem this
patch fixes for align == 0, just reached through a non-zero align?

Every in-tree caller passes 0 through the end of the series. The only
non-zero caller is the kunit test with align=256, so nothing triggers this
today. The comment in test_kmem_buckets_explicit_alignment says it "checks
the override both ways". That suggests alignment weaker than kmalloc's is
intended.

The same expression appears later in the series in
kmem_buckets_create_row(), as align ?: kmalloc_caches[type][idx]->align.

Would max(align, kmalloc_caches[KMALLOC_NORMAL][idx]->align) keep the
kmalloc guarantees that bucket users and the DMA layer rely on? Clamping to
at least ARCH_KMALLOC_MINALIGN might also work.

> +					flags, cache_useroffset,
>  					cache_usersize, ctor);
>  			kfree(cache_name);
>  			if (WARN_ON(!(*b)[aligned_idx]))

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002231120.late.500-kees%40kernel.org