Re: [PATCH RFC v2 2/8] mm, slab: introduce slab_debug=N

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm, slab: missing SLAB_DEBUG_NOOP in SLAB_MERGE_SAME merges mismatched caches
--

commit 2d842bc4bf4f28b2b4c86a9001856691abe3624c
Author: Vlastimil Babka (SUSE) <vbabka@kernel.org>

mm, slab: introduce slab_debug=N

This commit introduces the slab_debug=N boot option and the corresponding
SLAB_DEBUG_NOOP slab flag to force slab debugging slowpaths and disable
percpu sheaf caching without enabling actual debug checks. It also excludes
SLAB_DEBUG_NOOP from SLAB_NEVER_MERGE and avoids disabling printk pointer
hashing when only SLAB_DEBUG_NOOP is enabled.

> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 01c64ef661e7f..02dfe4351e8b7 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -47,9 +47,9 @@ struct kmem_cache *kmem_cache;
>   * Any flag that adds per-object metadata should be included,
>   * since slab merging can update s->inuse that affects the metadata layout.
>   */
> -#define SLAB_NEVER_MERGE (SLAB_DEBUG_FLAGS | SLAB_TYPESAFE_BY_RCU | \
> -		SLAB_NOLEAKTRACE | SLAB_FAILSLAB | SLAB_NO_MERGE | \
> -		SLAB_OBJ_EXT_IN_OBJ)
> +#define SLAB_NEVER_MERGE ((SLAB_DEBUG_FLAGS & ~SLAB_DEBUG_NOOP) | \
> +		SLAB_TYPESAFE_BY_RCU | SLAB_NOLEAKTRACE | SLAB_FAILSLAB | \
> +		SLAB_NO_MERGE | SLAB_OBJ_EXT_IN_OBJ)
>  
>  #define SLAB_MERGE_SAME (SLAB_RECLAIM_ACCOUNT | SLAB_CACHE_DMA | \
>  			 SLAB_CACHE_DMA32 | SLAB_ACCOUNT | SLAB_MAY_ACCOUNT)

[Severity: Medium]
Should SLAB_DEBUG_NOOP also be added to SLAB_MERGE_SAME?

Excluding SLAB_DEBUG_NOOP from SLAB_NEVER_MERGE allows caches with
SLAB_DEBUG_NOOP to participate in slab merging, which is checked in
find_mergeable():

mm/slab_common.c:find_mergeable() {
	flags = kmem_cache_flags(flags, name);
	if (slab_args_unmergeable(args, flags))
		return NULL;
	...
	list_for_each_entry_reverse(s, &slab_caches, list) {
		if (slab_unmergeable(s))
			continue;

		if (size > s->size)
			continue;

		if ((flags & SLAB_MERGE_SAME) != (s->flags & SLAB_MERGE_SAME))
			continue;
}

When SLAB_DEBUG_NOOP is enabled for a subset of caches (for example via
per-cache boot parameters such as slab_debug=N,<cache_name> or
slab_debug=N;-,<cache_name>, or by passing SLAB_DEBUG_NOOP to
kmem_cache_create()), find_mergeable() does not require both caches to agree
on SLAB_DEBUG_NOOP.

As a result, a cache matched by slab_debug=N,<cache_name> can merge into an
existing compatible cache created without SLAB_DEBUG_NOOP (such as a
kmalloc-* cache), keeping percpu sheaves enabled and silently ignoring the
requested option. Conversely, if an earlier mergeable cache is created with
SLAB_DEBUG_NOOP, subsequent compatible caches without SLAB_DEBUG_NOOP will
merge into it and be forced onto the debug slowpaths with zero sheaf
capacity.

If SLAB_DEBUG_NOOP is added to SLAB_MERGE_SAME, does create_unique_id() in
mm/slub.c also need to encode SLAB_DEBUG_NOOP so that aliasable caches
differing only in SLAB_DEBUG_NOOP do not generate colliding sysfs IDs in
sysfs_slab_add()?

mm/slub.c:create_unique_id() {
	/*
	 * First flags affecting slabcache operations. We will only
	 * get here for aliasable slabs so we do not need to support
	 * too many flags. The flags here must cover all flags that
	 * are matched during merging to guarantee that the id is
	 * unique.
	 */
	if (s->flags & SLAB_CACHE_DMA)
		*p++ = 'd';
	if (s->flags & SLAB_CACHE_DMA32)
		*p++ = 'D';
	if (s->flags & SLAB_RECLAIM_ACCOUNT)
		*p++ = 'a';
	if (s->flags & SLAB_CONSISTENCY_CHECKS)
		*p++ = 'F';
	if (s->flags & SLAB_ACCOUNT)
		*p++ = 'A';
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024-slub_tiny_rework-v2-0-bf1419ab5ffd@kernel.org?part=2