Re: [PATCH net-next v5 4/7] mm/slab: Add tests for the existing kmem_buckets behaviour

[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] test_kmem_buckets_destroy() (lib/tests/slub_kunit.c) can fail
  at random on CONFIG_KFENCE=y kernels.
- [Low] test_kmem_buckets_sizes() (lib/tests/slub_kunit.c) checks less
  than its comment and the commit message say it does.

--

mm/slab: Add tests for the existing kmem_buckets behaviour

This adds KUnit tests to lib/tests/slub_kunit.c for kmem_buckets. They
cover cache isolation, size class routing, default and explicit cache
alignment, the CONFIG_SLAB_BUCKETS=n fallback, and teardown of a set's
caches by kmem_buckets_destroy().

> diff --git a/lib/tests/slub_kunit.c b/lib/tests/slub_kunit.c
> index e3b63f0338d5f..3c923a3af825b 100644
> --- a/lib/tests/slub_kunit.c
> +++ b/lib/tests/slub_kunit.c

[ ... ]

> @@ -474,6 +475,251 @@ static int test_init(struct kunit *test)

[ ... ]

> +/*
> + * Every size class gets its own cache in the set, including the ones that
> + * are not powers of two and are filled in from an aligned index. Sizes past
> + * the largest cache are served by the page allocator, bucket set or not.
> + */
> +static void test_kmem_buckets_sizes(struct kunit *test)
> +{
> +	static const size_t sizes[] = { 8, 96, 192, 1024, 4096 };

[ ... ]

> +	for (i = 0; i < ARRAY_SIZE(sizes); i++) {
> +		p = kmem_buckets_alloc(b, sizes[i], GFP_KERNEL);
> +		KUNIT_ASSERT_NOT_NULL(test, p);
> +		c = cache_of(p);
> +		kfree(p);
> +		KUNIT_ASSERT_NOT_NULL(test, c);
> +
> +		KUNIT_EXPECT_TRUE_MSG(test, strstarts(c->name, "sized_buckets-"),
> +				      "size %zu: expected a bucket cache, got %s",
> +				      sizes[i], c->name);
> +		KUNIT_EXPECT_GE(test, c->object_size, sizes[i]);

[Severity: Low]
Is this check strong enough to back up the comment above and the commit
message? The commit message says:

    Each size class is served by the set, including 96 and 192, which are
    not powers of two and are filled in from an aligned index by
    kmem_buckets_create().

The test only checks the name prefix and uses >= on object_size. It would
still pass if a size went to a larger bucket cache, for example 8 bytes
ending up in sized_buckets-8k.

On x86_64, __kmalloc_minalign() returns 8, which is the same as
ARCH_KMALLOC_MINALIGN. So new_kmalloc_cache() aliases nothing, and this
branch in kmem_buckets_create() never runs there:

mm/slab_common.c:kmem_buckets_create() {
    ...
		if (idx != aligned_idx)
			(*b)[idx] = (*b)[aligned_idx];
    ...
}

On configurations where minalign is larger than ARCH_KMALLOC_MINALIGN
(for example arm64 without swiotlb), sizes such as 8 and 96 share a cache
with a larger class. "Every size class gets its own cache" doesn't hold
there, and the >= check hides that.

Would it be tighter to compare c->object_size against
kmalloc_size_roundup(sizes[i]), or against the object_size of the
matching general kmalloc cache? The test also looks unchanged at the end
of the series.

> +	}

[ ... ]

> +/* Destroying a set has to take its caches down, not just free the set. */
> +static void test_kmem_buckets_destroy(struct kunit *test)
> +{

[ ... ]

> +	b = kmem_buckets_create("destroyed_buckets", 0, 0, 0, INT_MAX, NULL);
> +	KUNIT_ASSERT_NOT_NULL(test, b);
> +
> +	/*
> +	 * Deliberately leaked, as test_leak_destroy() leaks its own: the
> +	 * teardown below has to find it. kmem_cache_destroy() unlists the
> +	 * cache either way, so the name is still released.
> +	 */
> +	p = kmem_buckets_alloc(b, 128, GFP_KERNEL);
> +	KUNIT_EXPECT_NOT_NULL(test, p);
> +
> +	kmem_buckets_destroy(b);
> +
> +	KUNIT_EXPECT_EQ(test, 2, slab_errors);

[Severity: Medium]
Can this fail at random on CONFIG_KFENCE=y kernels?

kmem_buckets_create() only adds SLAB_NO_MERGE to the bucket caches.
Nothing sets SLAB_SKIP_KFENCE, so slab_alloc_node()->kfence_alloc() can
serve this 128 byte allocation from the KFENCE pool.

In that case the leaked object is not in any slab on the node partial
list:

kmem_buckets_destroy()
  kmem_cache_destroy()
    __kmem_cache_shutdown()
      free_partial()           <- no in-use slab, so list_slab_objects()
                                  is never called and returns 0
    kmem_cache_release()
      kfence_shutdown_cache()  <- object becomes a zombie, no kunit error

slab_errors would then stay at 0, and the check against 2 would fail.
Whether it fails depends only on the KFENCE sample timer.

The existing test_leak_destroy() avoids this by going through
test_kmem_cache_create(), which does:

	s->flags |= SLAB_SKIP_KFENCE;

Could this test set SLAB_SKIP_KFENCE on the 128 byte bucket cache before
allocating? Another option is to skip or retry when is_kfence_address(p)
is true. This also looks unchanged at the end of the series.

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