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

[Petr Mladek <pmladek@suse.com>]

On Mon 2021-12-06 09:04:40, Greg Kroah-Hartman wrote:
> On Mon, Dec 06, 2021 at 10:13:53AM +0800, Ming Lei wrote:
> > On Fri, Dec 03, 2021 at 04:07:39PM +0100, Greg Kroah-Hartman wrote:
> > > On Mon, Nov 29, 2021 at 11:45:09AM +0800, 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.
> > > 
> > > The caller should NOT expect the kobject to be released.  That's the
> > > whole point of dynamic reference counted objects, you never "know" when
> > > the last object is released.  This option just makes it obvious so that
> > > you know when to fix up code that has this assumption.
> > 
> > > > Inside the cleanup handler, kobj->ktype and kobj->ktype->release are
> > > > required.
> > > 
> > > Yes. Is that a problem?
> > 
> > Of course for CONFIG_DEBUG_KOBJECT_RELEASE, which delays to call
> > ->release after random time, when the module for storing ->ktype and
> > ->ktype->release has been unloaded.
> > 
> > As I mentioned, the issue can be triggered 100% by 'modprobe -r
> > kset-example' when CONFIG_DEBUG_KOBJECT_RELEASE is enabled if the
> > 1st patch is applied.
> 
> Is there any "real" kernel code that this causes problems on?
> 
> Again, this is for debugging, yes, this tiny example will crash that
> way, but that is fine, as we can obviously see that the kernel code here
> is correct.
> 
> And if you really want to ensure that it works properly, let's wait on
> release before allowing that module to be unloaded.

This is exactly what this patch is trying to achieve. IMHO,
we should do it another way, see below.


> But again, module unload is NOT a normal operation and is not what
> this debugging option was created to help out with.

But people do unload module and especially when testing kernel.
IMHO, we want both CONFIG_DEBUG_KOBJECT_RELEASE and module unload
enabled when testing kernel.


> Again, the confusion between kobjects (which protect data) and module
> references (which protect code) is getting mixed up here.

This is perfect description of the problem. And the problem is real.

kobjects protect data but they need to call code (callbacks) when
they are released. module unload is special because it removes the
code. CONFIG_DEBUG_KOBJECT_RELEASE always delays the code calls.
It results into a crash even when everything works as expected.


Now, back to the proposed patch. I agree that it looks weird. It
makes CONFIG_DEBUG_KOBJECT_RELEASE useless in this scenario.

I have another idea. What about adding a pointer to
struct module *mod into struct kobj_type. Some reference
counter and wait_queue into struct module. They might be
used to block the module_exit() until the reference counter
reaches zero.

I mean something like:

Let's take samples/kobject/kset-sample.c as an example.
We could define:

static struct kobj_type foo_ktype = {
	.sysfs_ops = &foo_sysfs_ops,
	.release = foo_release,
	.default_groups = foo_default_groups,
	.mod = THIS_MODULE,
};

then we might do:

static int kobject_add_internal(struct kobject *kobj)
{
[...]
	if (kobject->ktype->mod)
		module_get_kobject_referece(kobject->ktype->mod);
[...]
}

and

static void kobject_cleanup(struct kobject *kobj)
{
[...]
	if (kobject->ktype->mod)
		module_put_kobject_referece(kobject->ktype->mod);
[...]
}

where

void module_get_kobject_referece(struct module *mod)
{
	mutex_lock(&module_mutex);
	mod->kobject_ref++;
	mutex_lock(&module_mutex);
}

void module_put_kobject_referece(struct module *mod)
{
	struct wait_queue_head *module_kobject_wq;

	mutex_lock(&module_mutex);
	mod->kobject_ref--;
	if (!mod->kobject_ref)
		wake_up(mod->kobj_release_wq);
	mutex_lock(&module_mutex);
}


and

SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
		unsigned int, flags)
{
[...]
	wait_event_interruptible(mod->kobj_release_wq, !mod->kobj_ref);
[...]
}

There might be many details to be solved.

But it looks like a win-win solution. It should make module unload
much more secure. Broken modules will just get blocked in
module_cleanup forever. CONFIG_DEBUG_KOBJECT_RELEASE will still
work as designed.

Best Regards,
Petr