| Message ID | 20220930084742.771804-1-linux@rasmusvillemoes.dk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: slub: remove dead and buggy code from sysfs_slab_add() | expand |
On Fri, Sep 30, 2022 at 10:47:42AM +0200, Rasmus Villemoes wrote: > The function sysfs_slab_add() has two callers: > > One is slab_sysfs_init(), which first initializes slab_kset, and only > when that succeeds sets slab_state to FULL, and then proceeds to call > sysfs_slab_add() for all previously created slabs. > > The other is __kmem_cache_create(), but only after a > > if (slab_state <= UP) > return 0; > > check. > > So in other words, sysfs_slab_add() is never called without > slab_kset (aka the return value of cache_kset()) being non-NULL. > > And this is just as well, because if we ever did take this path and > called kobject_init(&s->kobj), and then later when called again from > slab_sysfs_init() would end up calling kobject_init_and_add(), we > would hit > > if (kobj->state_initialized) { > /* do not error out as sometimes we can recover */ > pr_err("kobject (%p): tried to init an initialized object, something is seriously wrong.\n", > dump_stack(); > } > > in kobject.c. > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > mm/slub.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/mm/slub.c b/mm/slub.c > index 4b98dff9be8e..04a7f75a7b1f 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -5937,11 +5937,6 @@ static int sysfs_slab_add(struct kmem_cache *s) > struct kset *kset = cache_kset(s); > int unmergeable = slab_unmergeable(s); > > - if (!kset) { > - kobject_init(&s->kobj, &slab_ktype); > - return 0; > - } > - > if (!unmergeable && disable_higher_order_debug && > (slub_debug & DEBUG_METADATA_FLAGS)) > unmergeable = 1; > -- > 2.37.2 I assumed that it's hit when SLUB failed to initialize slab_kset in slab_sysfs_init(). (Yeah, it is too unlikely, though....) And obviously it's a bug if sysfs_slab_add() is called early than slab_sysfs_init().
On 03/10/2022 09.02, Hyeonggon Yoo wrote: > On Fri, Sep 30, 2022 at 10:47:42AM +0200, Rasmus Villemoes wrote: >> The function sysfs_slab_add() has two callers: >> >> One is slab_sysfs_init(), which first initializes slab_kset, and only >> when that succeeds sets slab_state to FULL, and then proceeds to call >> sysfs_slab_add() for all previously created slabs. >> >> The other is __kmem_cache_create(), but only after a >> >> if (slab_state <= UP) >> return 0; >> >> check. >> >> So in other words, sysfs_slab_add() is never called without >> slab_kset (aka the return value of cache_kset()) being non-NULL. >> >> And this is just as well, because if we ever did take this path and >> called kobject_init(&s->kobj), and then later when called again from >> slab_sysfs_init() would end up calling kobject_init_and_add(), we >> would hit >> >> if (kobj->state_initialized) { >> /* do not error out as sometimes we can recover */ >> pr_err("kobject (%p): tried to init an initialized object, something is seriously wrong.\n", >> dump_stack(); >> } >> >> in kobject.c. >> >> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> >> --- >> mm/slub.c | 5 ----- >> 1 file changed, 5 deletions(-) >> >> diff --git a/mm/slub.c b/mm/slub.c >> index 4b98dff9be8e..04a7f75a7b1f 100644 >> --- a/mm/slub.c >> +++ b/mm/slub.c >> @@ -5937,11 +5937,6 @@ static int sysfs_slab_add(struct kmem_cache *s) >> struct kset *kset = cache_kset(s); >> int unmergeable = slab_unmergeable(s); >> >> - if (!kset) { >> - kobject_init(&s->kobj, &slab_ktype); >> - return 0; >> - } >> - >> if (!unmergeable && disable_higher_order_debug && >> (slub_debug & DEBUG_METADATA_FLAGS)) >> unmergeable = 1; >> -- >> 2.37.2 > > I assumed that it's hit when SLUB failed to initialize slab_kset in > slab_sysfs_init(). (Yeah, it is too unlikely, though....) No, it is not, because if the creation of slab_kset fails, slab_sysfs_init() returns early, and hence slab_state never transitions to FULL. I don't see anywhere else where slab_state could become FULL (of course in slab.c and slob.c, but those are not built when slub.c is), so I do believe my analysis in the commit log is correct. > And obviously it's a bug if sysfs_slab_add() is called early than > slab_sysfs_init(). Yes, and that's already what the existing slab_state check guards. Rasmus
On Mon, Oct 03, 2022 at 11:38:30AM +0200, Rasmus Villemoes wrote: > On 03/10/2022 09.02, Hyeonggon Yoo wrote: > > On Fri, Sep 30, 2022 at 10:47:42AM +0200, Rasmus Villemoes wrote: > >> The function sysfs_slab_add() has two callers: > >> > >> One is slab_sysfs_init(), which first initializes slab_kset, and only > >> when that succeeds sets slab_state to FULL, and then proceeds to call > >> sysfs_slab_add() for all previously created slabs. > >> > >> The other is __kmem_cache_create(), but only after a > >> > >> if (slab_state <= UP) > >> return 0; > >> > >> check. > >> > >> So in other words, sysfs_slab_add() is never called without > >> slab_kset (aka the return value of cache_kset()) being non-NULL. > >> > >> And this is just as well, because if we ever did take this path and > >> called kobject_init(&s->kobj), and then later when called again from > >> slab_sysfs_init() would end up calling kobject_init_and_add(), we > >> would hit > >> > >> if (kobj->state_initialized) { > >> /* do not error out as sometimes we can recover */ > >> pr_err("kobject (%p): tried to init an initialized object, something is seriously wrong.\n", > >> dump_stack(); > >> } > >> > >> in kobject.c. > >> > >> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > >> --- > >> mm/slub.c | 5 ----- > >> 1 file changed, 5 deletions(-) > >> > >> diff --git a/mm/slub.c b/mm/slub.c > >> index 4b98dff9be8e..04a7f75a7b1f 100644 > >> --- a/mm/slub.c > >> +++ b/mm/slub.c > >> @@ -5937,11 +5937,6 @@ static int sysfs_slab_add(struct kmem_cache *s) > >> struct kset *kset = cache_kset(s); > >> int unmergeable = slab_unmergeable(s); > >> > >> - if (!kset) { > >> - kobject_init(&s->kobj, &slab_ktype); > >> - return 0; > >> - } > >> - > >> if (!unmergeable && disable_higher_order_debug && > >> (slub_debug & DEBUG_METADATA_FLAGS)) > >> unmergeable = 1; > >> -- > >> 2.37.2 > > > > I assumed that it's hit when SLUB failed to initialize slab_kset in > > slab_sysfs_init(). (Yeah, it is too unlikely, though....) > > No, it is not, because if the creation of slab_kset fails, > slab_sysfs_init() returns early, and hence slab_state never transitions > to FULL. Yeah, you are right ;-) I misread that. > I don't see anywhere else where slab_state could become FULL > (of course in slab.c and slob.c, but those are not built when slub.c > is), so I do believe my analysis in the commit log is correct. Right. > > And obviously it's a bug if sysfs_slab_add() is called early than > > slab_sysfs_init(). > > Yes, and that's already what the existing slab_state check guards. > > Rasmus Looks good to me, Reviewed-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> Thanks!
On Fri, 30 Sep 2022, Rasmus Villemoes wrote: > The function sysfs_slab_add() has two callers: > > One is slab_sysfs_init(), which first initializes slab_kset, and only > when that succeeds sets slab_state to FULL, and then proceeds to call > sysfs_slab_add() for all previously created slabs. > > The other is __kmem_cache_create(), but only after a > > if (slab_state <= UP) > return 0; > > check. > > So in other words, sysfs_slab_add() is never called without > slab_kset (aka the return value of cache_kset()) being non-NULL. > > And this is just as well, because if we ever did take this path and > called kobject_init(&s->kobj), and then later when called again from > slab_sysfs_init() would end up calling kobject_init_and_add(), we > would hit > > if (kobj->state_initialized) { > /* do not error out as sometimes we can recover */ > pr_err("kobject (%p): tried to init an initialized object, something is seriously wrong.\n", > dump_stack(); > } > > in kobject.c. > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> Acked-by: David Rientjes <rientjes@google.com>
On 9/30/22 10:47, Rasmus Villemoes wrote: > The function sysfs_slab_add() has two callers: > > One is slab_sysfs_init(), which first initializes slab_kset, and only > when that succeeds sets slab_state to FULL, and then proceeds to call > sysfs_slab_add() for all previously created slabs. > > The other is __kmem_cache_create(), but only after a > > if (slab_state <= UP) > return 0; > > check. > > So in other words, sysfs_slab_add() is never called without > slab_kset (aka the return value of cache_kset()) being non-NULL. > > And this is just as well, because if we ever did take this path and > called kobject_init(&s->kobj), and then later when called again from > slab_sysfs_init() would end up calling kobject_init_and_add(), we > would hit > > if (kobj->state_initialized) { > /* do not error out as sometimes we can recover */ > pr_err("kobject (%p): tried to init an initialized object, something is seriously wrong.\n", > dump_stack(); > } > > in kobject.c. > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> Thanks, added to slab.git for-6.2/slub-sysfs > --- > mm/slub.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/mm/slub.c b/mm/slub.c > index 4b98dff9be8e..04a7f75a7b1f 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -5937,11 +5937,6 @@ static int sysfs_slab_add(struct kmem_cache *s) > struct kset *kset = cache_kset(s); > int unmergeable = slab_unmergeable(s); > > - if (!kset) { > - kobject_init(&s->kobj, &slab_ktype); > - return 0; > - } > - > if (!unmergeable && disable_higher_order_debug && > (slub_debug & DEBUG_METADATA_FLAGS)) > unmergeable = 1;
diff --git a/mm/slub.c b/mm/slub.c index 4b98dff9be8e..04a7f75a7b1f 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -5937,11 +5937,6 @@ static int sysfs_slab_add(struct kmem_cache *s) struct kset *kset = cache_kset(s); int unmergeable = slab_unmergeable(s); - if (!kset) { - kobject_init(&s->kobj, &slab_ktype); - return 0; - } - if (!unmergeable && disable_higher_order_debug && (slub_debug & DEBUG_METADATA_FLAGS)) unmergeable = 1;
The function sysfs_slab_add() has two callers: One is slab_sysfs_init(), which first initializes slab_kset, and only when that succeeds sets slab_state to FULL, and then proceeds to call sysfs_slab_add() for all previously created slabs. The other is __kmem_cache_create(), but only after a if (slab_state <= UP) return 0; check. So in other words, sysfs_slab_add() is never called without slab_kset (aka the return value of cache_kset()) being non-NULL. And this is just as well, because if we ever did take this path and called kobject_init(&s->kobj), and then later when called again from slab_sysfs_init() would end up calling kobject_init_and_add(), we would hit if (kobj->state_initialized) { /* do not error out as sometimes we can recover */ pr_err("kobject (%p): tried to init an initialized object, something is seriously wrong.\n", dump_stack(); } in kobject.c. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- mm/slub.c | 5 ----- 1 file changed, 5 deletions(-)