* Re: [PATCH] mm, slab: fix use of SLAB_SUPPORTS_SYSFS in kmem_cache_release()
2024-10-01 14:02 [PATCH] mm, slab: fix use of SLAB_SUPPORTS_SYSFS in kmem_cache_release() Nilay Shroff
@ 2024-10-01 14:50 ` Hyeonggon Yoo
2024-10-01 15:39 ` Vlastimil Babka
2024-10-01 15:59 ` Yi Zhang
2024-10-01 16:27 ` Vlastimil Babka
2 siblings, 1 reply; 6+ messages in thread
From: Hyeonggon Yoo @ 2024-10-01 14:50 UTC (permalink / raw)
To: Nilay Shroff
Cc: linux-mm, linux-block, vbabka, akpm, cl, penberg, rientjes,
iamjoonsoo.kim, roman.gushchin, yi.zhang, shinichiro.kawasaki,
axboe, gjoyce
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mm, slab: fix use of SLAB_SUPPORTS_SYSFS in kmem_cache_release()
2024-10-01 14:50 ` Hyeonggon Yoo
@ 2024-10-01 15:39 ` Vlastimil Babka
2024-10-01 15:51 ` Hyeonggon Yoo
0 siblings, 1 reply; 6+ messages in thread
From: Vlastimil Babka @ 2024-10-01 15:39 UTC (permalink / raw)
To: Hyeonggon Yoo, Nilay Shroff
Cc: linux-mm, linux-block, akpm, cl, penberg, rientjes,
iamjoonsoo.kim, roman.gushchin, yi.zhang, shinichiro.kawasaki,
axboe, gjoyce
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
>>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mm, slab: fix use of SLAB_SUPPORTS_SYSFS in kmem_cache_release()
2024-10-01 15:39 ` Vlastimil Babka
@ 2024-10-01 15:51 ` Hyeonggon Yoo
0 siblings, 0 replies; 6+ messages in thread
From: Hyeonggon Yoo @ 2024-10-01 15:51 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Nilay Shroff, linux-mm, linux-block, akpm, cl, penberg, rientjes,
iamjoonsoo.kim, roman.gushchin, yi.zhang, shinichiro.kawasaki,
axboe, gjoyce
On Wed, Oct 2, 2024 at 12:39 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> 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.
Oh, you are right. After looking into the history, __is_defined() is
actually intended for
non-config macros.
With that in mind, the fix looks good to me.
Reviewed-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> > 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
> >>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, slab: fix use of SLAB_SUPPORTS_SYSFS in kmem_cache_release()
2024-10-01 14:02 [PATCH] mm, slab: fix use of SLAB_SUPPORTS_SYSFS in kmem_cache_release() Nilay Shroff
2024-10-01 14:50 ` Hyeonggon Yoo
@ 2024-10-01 15:59 ` Yi Zhang
2024-10-01 16:27 ` Vlastimil Babka
2 siblings, 0 replies; 6+ messages in thread
From: Yi Zhang @ 2024-10-01 15:59 UTC (permalink / raw)
To: Nilay Shroff
Cc: linux-mm, linux-block, vbabka, akpm, cl, penberg, rientjes,
iamjoonsoo.kim, roman.gushchin, 42.hyeyoo, shinichiro.kawasaki,
axboe, gjoyce
On Tue, Oct 1, 2024 at 10:03 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.
>
> 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>
Tested-by: Yi Zhang <yi.zhang@redhat.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
>
--
Best Regards,
Yi Zhang
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mm, slab: fix use of SLAB_SUPPORTS_SYSFS in kmem_cache_release()
2024-10-01 14:02 [PATCH] mm, slab: fix use of SLAB_SUPPORTS_SYSFS in kmem_cache_release() Nilay Shroff
2024-10-01 14:50 ` Hyeonggon Yoo
2024-10-01 15:59 ` Yi Zhang
@ 2024-10-01 16:27 ` Vlastimil Babka
2 siblings, 0 replies; 6+ messages in thread
From: Vlastimil Babka @ 2024-10-01 16:27 UTC (permalink / raw)
To: Nilay Shroff, linux-mm, linux-block
Cc: akpm, cl, penberg, rientjes, iamjoonsoo.kim, roman.gushchin,
42.hyeyoo, yi.zhang, shinichiro.kawasaki, axboe, gjoyce
On 10/1/24 16:02, Nilay Shroff 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.
>
> 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>
Added to slab/for-next, thanks!
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread