Re: [PATCH slab/for-next 01/14] mm/slab: allow passing SLAB_MAY_ACCOUNT to kmem_cache_create()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm/slab: unhandled SLAB_MAY_ACCOUNT in create_unique_id() and nokmem path
--

commit 6d48a33523fd53d7ecd1bf1fa6ab70ec7126128f
Author: Harry Yoo (Meta) <harry@kernel.org>

mm/slab: allow passing SLAB_MAY_ACCOUNT to kmem_cache_create()

This commit adds SLAB_MAY_ACCOUNT to SLAB_CORE_FLAGS so callers of
kmem_cache_create() can explicitly specify SLAB_MAY_ACCOUNT for caches whose
objects may be allocated with __GFP_ACCOUNT.

> diff --git a/mm/slab.h b/mm/slab.h
> index e94ac63d10dc0..31783311e23ea 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -472,7 +472,7 @@ void flush_rcu_sheaves_on_cache(struct kmem_cache *s);
>  			 SLAB_CACHE_DMA32 | SLAB_PANIC | \
>  			 SLAB_TYPESAFE_BY_RCU | SLAB_DEBUG_OBJECTS | \
>  			 SLAB_NOLEAKTRACE | SLAB_RECLAIM_ACCOUNT | \
> -			 SLAB_TEMPORARY | SLAB_ACCOUNT | \
> +			 SLAB_TEMPORARY | SLAB_ACCOUNT | SLAB_MAY_ACCOUNT | \
>  			 SLAB_NO_USER_FLAGS | SLAB_KMALLOC | SLAB_NO_MERGE)
>  
>  #define SLAB_DEBUG_FLAGS (SLAB_RED_ZONE | SLAB_POISON | SLAB_STORE_USER | \

[Severity: Medium]
When callers pass SLAB_MAY_ACCOUNT to kmem_cache_create(), could this cause a
sysfs kobject naming collision in create_unique_id() as well as unhandled
SLAB_MAY_ACCOUNT state when mem_cgroup_kmem_disabled() is true?

In mm/slab_common.c, SLAB_MAY_ACCOUNT is included in SLAB_MERGE_SAME:

#define SLAB_MERGE_SAME (SLAB_RECLAIM_ACCOUNT | SLAB_CACHE_DMA | \
			 SLAB_CACHE_DMA32 | SLAB_ACCOUNT | SLAB_MAY_ACCOUNT)

so find_mergeable() will not merge two aliasable caches of the same size if
only one of them has SLAB_MAY_ACCOUNT set.

However, in mm/slub.c, when do_kmem_cache_create() -> sysfs_slab_add() ->
create_unique_id() generates the sysfs identifier for an aliasable cache,
SLAB_MAY_ACCOUNT is not encoded into the name:

static char *create_unique_id(struct kmem_cache *s)
{
	...
	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';
	if (p != name + 1)
		*p++ = '-';
	p += snprintf(p, ID_STR_LENGTH - (p - name), "%07u", s->size);

Because create_unique_id() omits SLAB_MAY_ACCOUNT, two unmerged aliasable
caches of the same size that differ only in SLAB_MAY_ACCOUNT will produce the
same sysfs ID string, causing kobject_init_and_add() in sysfs_slab_add() to
fail with -EEXIST and trigger a sysfs_warn_dup() warning.

In addition, in mm/slab_common.c, __kmem_cache_create_args() currently does:

	if (!mem_cgroup_kmem_disabled())
		flags |= SLAB_MAY_ACCOUNT;

If mem_cgroup_kmem_disabled() is true (for example, with
cgroup.memory=nokmem) and a caller passes SLAB_MAY_ACCOUNT, the flag is not
cleared. That prevents find_mergeable() from merging the cache with caches
that lack SLAB_MAY_ACCOUNT (triggering the create_unique_id() collision
above), and causes cache_needs_objcg() in mm/slab.h:

static inline bool cache_needs_objcg(struct kmem_cache *cache)
{
	return (cache->flags & SLAB_MAY_ACCOUNT);
}

to return true, reserving unused _objcg metadata space via
cache_obj_ext_size() and slab_obj_ext_size() when slab object extensions are
allocated.

Note that a later change in the series, commit 51cadd40978b ("mm/slab: do not
set SLAB_MAY_ACCOUNT for all caches"), clears SLAB_MAY_ACCOUNT in
__kmem_cache_create_args() when mem_cgroup_kmem_disabled() is true, but it
does not update create_unique_id(), where the sysfs ID collision will then
also occur on default boots once SLAB_MAY_ACCOUNT is no longer set on all
caches.

Should create_unique_id() encode SLAB_MAY_ACCOUNT, and should
__kmem_cache_create_args() strip SLAB_MAY_ACCOUNT when
mem_cgroup_kmem_disabled() is true here as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-memalloc-prof-dont-waste-slabobj-ext-metadata-v1-0-096800aa3330@kernel.org?part=1