Re: [PATCH 2/2] kobject: wait until kobject is cleaned up before freeing module

[Petr Mladek]

On Wed 2021-11-10 08:03:04, Greg Kroah-Hartman wrote:
> On Wed, Nov 10, 2021 at 09:20:27AM +0800, Ming Lei wrote:
> > On Tue, Nov 09, 2021 at 02:14:09PM +0100, Petr Mladek wrote:
> > > On Tue 2021-11-09 10:00:27, Ming Lei wrote:
> > > > On Mon, Nov 08, 2021 at 06:26:25PM +0100, Petr Mladek wrote:
> > > > > On Fri 2021-11-05 14:37:10, Ming Lei wrote:
> > > > > > kobject_put() may become asynchronously because of
> > > > > > CONFIG_DEBUG_KOBJECT_RELEASE, so once kobject_put() returns, the caller may
> > > > > > expect the kobject is released after the last refcnt is dropped, however
> > > > > > CONFIG_DEBUG_KOBJECT_RELEASE just schedules one delayed work function
> > > > > > for cleaning up the kobject. Inside the cleanup handler, kobj->ktype and
> > > > > > kobj->ktype->release are required.
> > > > > > 
> > > > > > It is supposed that no activity is on kobject itself any more since
> > > > > > module_exit() is started, so it is reasonable for the kobject user or
> > > > > > driver to expect that kobject can be really released in the last run of
> > > > > > kobject_put() in module_exit() code path. Otherwise, it can be thought as
> > > > > > one driver's bug since the module is going away.
> > > > > 
> > > > > Honestly, this looks a bit fragile. What if there is still another
> > > > > reference from some reason. IMHO, it is easy to do it wrong.
> > > > > The kobject stuff is super-tricky.
> > > > > 
> > > > > Yes, there is the argument that it is a drivers bug when it does not
> > > > > work.
> > > > 
> > > > That is another 'issue'(even not sure if there is really), and it isn't covered
> > > > in this patchset, which focuses on fixing CONFIG_DEBUG_KOBJECT_RELEASE, so
> > > > please do not mix the two here.
> > > 
> > > Yes, it is another issue but the relation is very important.
> > > 
> > > My understanding is that this patch prevents problems caused by
> > > the delayed work. The kobject is added into kobj_cleanup_list
> > > only when the delayed work is scheduled. The patch has no effect
> > > if the delayed work is not used.
> > > 
> > > From my POV, this patch kind of removes the effect of the delayed
> > > work. My point is:
> > > 
> > > Does it still make sense to use the delayed work in the first place?
> > > Will the delayed work still help to catch some problems?
> > 
> > That depends on the user of CONFIG_DEBUG_KOBJECT_RELEASE, if users
> > thought it is useless, I think it is fine to remove it.
> > 
> > Greg, any idea about if CONFIG_DEBUG_KOBJECT_RELEASE is useful now?
> 
> Yes it is, it finds driver bugs where they do things wrong.

Please, do you have any idea what particular wrong things might happen?

IMHO, one bug might be that the driver module might be removed when
there are still users, some reference still exists. This patch
causes that CONFIG_DEBUG_KOBJECT_RELEASE will not longer help
to catch this kind of problems.

Is there any other common bug type that might be discovered by
the delayed release?

I just want to be sure that this patch does not make
CONFIG_DEBUG_KOBJECT_RELEASE useless.

Best Regards,
Petr