| Message ID | 20241001140245.306087-1-nilay@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm, slab: fix use of SLAB_SUPPORTS_SYSFS in kmem_cache_release() | expand |
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 >
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 >>
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 > >> >
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 >
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
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
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> --- mm/slab.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)