On Wed, Oct 2, 2024 at 12:39 AM Vlastimil Babka <vbabka@xxxxxxx> wrote:
>
> On 10/1/24 16:50, Hyeonggon Yoo wrote:
> > On Tue, Oct 1, 2024 at 11:02 PM Nilay Shroff <nilay@xxxxxxxxxxxxx> 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@xxxxxxxxx>
> > Best,
> > Hyeonggon
> >
> >> Fixes: 4ec10268ed98 ("mm, slab: unlink slabinfo, sysfs and debugfs immediately")
> >> Reported-by: Yi Zhang <yi.zhang@xxxxxxxxxx>
> >> Closes: https://lore.kernel.org/all/CAHj4cs9YCCcfmdxN43-9H3HnTYQsRtTYw1Kzq-L468GfLKAENA@xxxxxxxxxxxxxx/
> >> Signed-off-by: Nilay Shroff <nilay@xxxxxxxxxxxxx>
> >> ---
> >> 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
> >>
>
