Messages in this thread | | | Date | Tue, 7 Dec 2021 20:51:24 +0800 | From | Ming Lei <> | Subject | Re: [PATCH V2 2/2] kobject: wait until kobject is cleaned up before freeing module |
| |
On Tue, Dec 07, 2021 at 11:32:27AM +0100, Petr Mladek wrote: > 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.
Can you explain how this patch makes CONFIG_DEBUG_KOBJECT_RELEASE useless? The kobject is still cleaned up with random delay.
> > 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);
The question is why kobject is so special for taking one extra module ref here.
> } > > > 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.
The above approach might work, but it needs every driver with kobjects to be changed, so it is more complicated.
Thanks Ming
| |