Re: [PATCH 1/2] kobject: don't delay to cleanup module kobject

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Sat, Nov 27, 2021 at 12:28:48AM +0800, Ming Lei wrote:
> On Fri, Nov 26, 2021 at 05:08:16PM +0100, Greg Kroah-Hartman wrote:
> > On Fri, Nov 05, 2021 at 02:37:09PM +0800, Ming Lei wrote:
> > > CONFIG_DEBUG_KOBJECT_RELEASE is used for debugging kobject release/cleanup
> > > issue. The module kobject is released after module_exit() returns. If
> > > this kobject is delayed too much, and may cause other kobject's
> > > cleaned up a bit earlier before freeing module, then real issue is
> > > hidden.
> > > 
> > > So don't delay module kobject's cleanup, meantime module kobject is
> > > always cleaned up synchronously, and we needn't module kobject's
> > > cleanup.
> > > 
> > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > ---
> > >  lib/kobject.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/lib/kobject.c b/lib/kobject.c
> > > index ea53b30cf483..4c0dbe11be3d 100644
> > > --- a/lib/kobject.c
> > > +++ b/lib/kobject.c
> > > @@ -16,6 +16,7 @@
> > >  #include <linux/stat.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/random.h>
> > > +#include <linux/module.h>
> > >  
> > >  /**
> > >   * kobject_namespace() - Return @kobj's namespace tag.
> > > @@ -727,6 +728,10 @@ static void kobject_release(struct kref *kref)
> > >  	struct kobject *kobj = container_of(kref, struct kobject, kref);
> > >  #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
> > >  	unsigned long delay = HZ + HZ * (get_random_int() & 0x3);
> > > +
> > > +	if (kobj->ktype == &module_ktype)
> > > +		delay = 0;
> > 
> > No, there should not be anything "special" about module kobjects to get
> > this kind of treatment.  They should work like any other kobject and
> > clean up properly when needed.
> 
> Here setting 0 delay for module kobject is just for making DEBUG_KOBJECT_RELEASE
> reliable to detect/report issues. Otherwise if the random delay for module
> kobject is bigger than other kobjects, potential use-after-after won't
> be exposed.

So you now can not debug the module kobject code?

This needs to be documented really really really well why this kobject
type is somehow "special" in the code.  We should not special-case these
things unless you have a great reason, and I am not yet convinced.

thanks,

greg k-h