Re: [PATCH] mm, slab: fix use of SLAB_SUPPORTS_SYSFS in kmem_cache_release()

[Hyeonggon Yoo <42.hyeyoo@gmail.com>]

On Tue, Oct 1, 2024 at 11:02 PM Nilay Shroff <nilay@linux.ibm.com> wrote:
>
> The fix implemented in commit 4ec10268ed98 ("mm, slab: unlink slabinfo,
> sysfs and debugfs immediately") caused a subtle side effect due to which
> while destroying the kmem cache, the code path would never get into
> sysfs_slab_release() function even though SLAB_SUPPORTS_SYSFS is defined
> and slab state is FULL. Due to this side effect, we would never release
> kobject defined for kmem cache and leak the associated memory.
>
> The issue here's with the use of __is_defined() macro in kmem_cache_
> release(). The __is_defined() macro expands to __take_second_arg(
> arg1_or_junk 1, 0). If "arg1_or_junk" is defined to 1 then it expands to
> __take_second_arg(0, 1, 0) and returns 1. If "arg1_or_junk" is NOT defined
> to any value then it expands to __take_second_arg(... 1, 0) and returns 0.
>
> In this particular issue, SLAB_SUPPORTS_SYSFS is defined without any
> associated value and that causes __is_defined(SLAB_SUPPORTS_SYSFS) to
> always evaluate to 0 and hence it would never invoke sysfs_slab_release().
>
> This patch helps fix this issue by defining SLAB_SUPPORTS_SYSFS to 1.

Hi Nilay,

Thanks for your effort in investigating the issue and fixing it!
This makes sense to me, but is there any reason the code avoids using
IS_ENABLED()?

I think technically either IS_ENABLED() or __is_defined() (with your
fix) would work
in this case, but it made me think "What is the difference between
IS_ENABLED() and __is_defined()?"

IS_ENABLED() is already frequently used in mm and only few code snippets use
__is_defined() directly.

Best,
Hyeonggon

> Fixes: 4ec10268ed98 ("mm, slab: unlink slabinfo, sysfs and debugfs immediately")
> Reported-by: Yi Zhang <yi.zhang@redhat.com>
> Closes: https://lore.kernel.org/all/CAHj4cs9YCCcfmdxN43-9H3HnTYQsRtTYw1Kzq-L468GfLKAENA@mail.gmail.com/
> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
> ---
>  mm/slab.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/slab.h b/mm/slab.h
> index f22fb760b286..3e0a08ea4c42 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -310,7 +310,7 @@ struct kmem_cache {
>  };
>
>  #if defined(CONFIG_SYSFS) && !defined(CONFIG_SLUB_TINY)
> -#define SLAB_SUPPORTS_SYSFS
> +#define SLAB_SUPPORTS_SYSFS 1
>  void sysfs_slab_unlink(struct kmem_cache *s);
>  void sysfs_slab_release(struct kmem_cache *s);
>  #else
> --
> 2.45.2
>