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

[Vlastimil Babka <vbabka@suse.cz>]

On 10/1/24 16:50, Hyeonggon Yoo wrote:
> 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.

Oops, thanks a lot for debugging and fixing this!

> 
> 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.

I was wary of using IS_ENABLED() because that's intended for CONFIG_ macros
and SLAB_SUPPORTS_SYSFS isn't one, so even if it worked now, it wouldn't be
guaranteed to stay working.

> 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
>>