[PATCH V2 0/2] kobject: avoid to cleanup kobject after module is unloaded

[PATCH V2 0/2] kobject: avoid to cleanup kobject after module is unloaded

[Ming Lei]

Hello,

The 1st patch improves CONFIG_DEBUG_KOBJECT_RELEASE, so that kobject
cleanup after unloading module can be triggered easily. With this patch,
'modprobe kset-example' can trigger the issue very quickly.

The 2nd patch fixes the issue of cleaning up object after module is
unloaded, since kobj->ktype and related callbacks are often allocated
as module static variable. We need to make sure kobject is really
cleaned up before freeing module. This issue is reported by Petr Mladek.

V2:
	- add comment on 1st patch, suggested by Greg, 1/2
	- fix build failure in case that CONFIG_MODULE is disabled, and add
	comment in kobj_module_callback(), 2/2


Ming Lei (2):
  kobject: don't delay to cleanup module kobject
  kobject: wait until kobject is cleaned up before freeing module

 include/linux/kobject.h |  1 +
 lib/kobject.c           | 82 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+)

-- 
2.31.1

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

[Ming Lei]

CONFIG_DEBUG_KOBJECT_RELEASE is used for debugging kobject release and
cleanup issue. The module kobject is released after module_exit() returns.
If this kobject is delayed too much, and may cause other kobjects 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 CONFIG_DEBUG_KOBJECT_RELEASE is
actually needless for module kobject.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 lib/kobject.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/lib/kobject.c b/lib/kobject.c
index 4a56f519139d..b81319b0bd5a 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,19 @@ 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);
+
+	/*
+	 * Don't delay to release module kobject so that we can detect late
+	 * kobject release more effectively because module unloading waits
+	 * for completion of module kobject release, see mod_kobject_put.
+	 *
+	 * Meantime mod_kobject_put() always waits for completion of module
+	 * kobject's release, CONFIG_DEBUG_KOBJECT_RELEASE is basically
+	 * useless for debugging module kobject's release.
+	 */
+	if (kobj->ktype == &module_ktype)
+		delay = 0;
+
 	pr_info("kobject: '%s' (%p): %s, parent %p (delayed %ld)\n",
 		 kobject_name(kobj), kobj, __func__, kobj->parent, delay);
 	INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
-- 
2.31.1

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

[Ming Lei]

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.

When the ->ktype and ->ktype->release are allocated as module static
variable, it can cause trouble because the delayed cleanup handler may
be run after the module is unloaded.

Fixes the issue by flushing scheduled kobject cleanup work before
freeing module.

Reported-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 include/linux/kobject.h |  1 +
 lib/kobject.c           | 68 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index efd56f990a46..8d4e26171654 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -70,6 +70,7 @@ struct kobject {
 	struct kernfs_node	*sd; /* sysfs directory entry */
 	struct kref		kref;
 #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
+	struct list_head	node;
 	struct delayed_work	release;
 #endif
 	unsigned int state_initialized:1;
diff --git a/lib/kobject.c b/lib/kobject.c
index b81319b0bd5a..d52e05d1a68d 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -17,6 +17,12 @@
 #include <linux/slab.h>
 #include <linux/random.h>
 #include <linux/module.h>
+#include <linux/delay.h>
+
+#ifdef CONFIG_DEBUG_KOBJECT_RELEASE
+static LIST_HEAD(kobj_cleanup_list);
+static DEFINE_SPINLOCK(kobj_cleanup_lock);
+#endif
 
 /**
  * kobject_namespace() - Return @kobj's namespace tag.
@@ -682,6 +688,13 @@ static void kobject_cleanup(struct kobject *kobj)
 	struct kobject *parent = kobj->parent;
 	struct kobj_type *t = get_ktype(kobj);
 	const char *name = kobj->name;
+#ifdef CONFIG_DEBUG_KOBJECT_RELEASE
+	unsigned long flags;
+
+	spin_lock_irqsave(&kobj_cleanup_lock, flags);
+	list_del(&kobj->node);
+	spin_unlock_irqrestore(&kobj_cleanup_lock, flags);
+#endif
 
 	pr_debug("kobject: '%s' (%p): %s, parent %p\n",
 		 kobject_name(kobj), kobj, __func__, kobj->parent);
@@ -716,11 +729,55 @@ static void kobject_cleanup(struct kobject *kobj)
 }
 
 #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
+/*
+ * Module notifier call back, flushing scheduled kobject cleanup work
+ * before freeing module
+ */
+static int kobj_module_callback(struct notifier_block *nb,
+				   unsigned long val, void *data)
+{
+	LIST_HEAD(pending);
+
+#ifdef CONFIG_MODULES
+	if (val != MODULE_STATE_GOING)
+		return NOTIFY_DONE;
+#endif
+
+	spin_lock_irq(&kobj_cleanup_lock);
+	list_splice_init(&kobj_cleanup_list, &pending);
+	spin_unlock_irq(&kobj_cleanup_lock);
+
+	while (!list_empty_careful(&pending))
+		msleep(jiffies_to_msecs(HZ / 10));
+
+	/*
+	 * kobject_cleanup() may be in-progress, so we have to flush work
+	 * to make sure it is done.
+	 */
+	flush_scheduled_work();
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block kobj_module_nb = {
+	.notifier_call = kobj_module_callback,
+};
+
 static void kobject_delayed_cleanup(struct work_struct *work)
 {
 	kobject_cleanup(container_of(to_delayed_work(work),
 				     struct kobject, release));
 }
+
+static int __init kobj_delayed_cleanup_init(void)
+{
+	WARN_ON(register_module_notifier(&kobj_module_nb));
+	return 0;
+}
+#else
+static int __init kobj_delayed_cleanup_init(void)
+{
+	return 0;
+}
 #endif
 
 static void kobject_release(struct kref *kref)
@@ -728,6 +785,7 @@ 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);
+	unsigned long flags;
 
 	/*
 	 * Don't delay to release module kobject so that we can detect late
@@ -745,6 +803,10 @@ static void kobject_release(struct kref *kref)
 		 kobject_name(kobj), kobj, __func__, kobj->parent, delay);
 	INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
 
+	spin_lock_irqsave(&kobj_cleanup_lock, flags);
+	list_add(&kobj->node, &kobj_cleanup_list);
+	spin_unlock_irqrestore(&kobj_cleanup_lock, flags);
+
 	schedule_delayed_work(&kobj->release, delay);
 #else
 	kobject_cleanup(kobj);
@@ -1155,3 +1217,9 @@ void kobj_ns_drop(enum kobj_ns_type type, void *ns)
 	spin_unlock(&kobj_ns_type_lock);
 }
 EXPORT_SYMBOL_GPL(kobj_ns_drop);
+
+static int __init kobj_subsys_init(void)
+{
+	return kobj_delayed_cleanup_init();
+}
+core_initcall(kobj_subsys_init)
-- 
2.31.1

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

[Greg Kroah-Hartman]

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?

> 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.

Why is module_exit() somehow special here?  What is so odd about that?

> When the ->ktype and ->ktype->release are allocated as module static
> variable, it can cause trouble because the delayed cleanup handler may
> be run after the module is unloaded.

Why is ktype and release part of module code?

What module kobject is causing this problem?

> Fixes the issue by flushing scheduled kobject cleanup work before
> freeing module.

Why are modules special here?

And if you enable this option, and then start unloading kernel modules,
yes, things can go wrong, but that's not what this kernel option is for
at all.

This feels like a hack for not a real problem.

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Mon, Nov 29, 2021 at 11:45:08AM +0800, Ming Lei wrote:
> CONFIG_DEBUG_KOBJECT_RELEASE is used for debugging kobject release and
> cleanup issue. The module kobject is released after module_exit() returns.
> If this kobject is delayed too much, and may cause other kobjects 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 CONFIG_DEBUG_KOBJECT_RELEASE is
> actually needless for module kobject.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  lib/kobject.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 4a56f519139d..b81319b0bd5a 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,19 @@ 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);
> +
> +	/*
> +	 * Don't delay to release module kobject so that we can detect late
> +	 * kobject release more effectively because module unloading waits
> +	 * for completion of module kobject release, see mod_kobject_put.
> +	 *
> +	 * Meantime mod_kobject_put() always waits for completion of module
> +	 * kobject's release, CONFIG_DEBUG_KOBJECT_RELEASE is basically
> +	 * useless for debugging module kobject's release.
> +	 */
> +	if (kobj->ktype == &module_ktype)
> +		delay = 0;

again, no, module kobjects are not more special than anything else.

greg k-h

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

[Ming Lei]

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.

Yes, so CONFIG_DEBUG_KOBJECT_RELEASE needs to be fixed.

> 
> > 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.

> 
> > 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.
> 
> Why is module_exit() somehow special here?  What is so odd about that?

After module_exit() is done, the module will be unloaded, then any code
or data stored in the module can't be referred.

> 
> > When the ->ktype and ->ktype->release are allocated as module static
> > variable, it can cause trouble because the delayed cleanup handler may
> > be run after the module is unloaded.
> 
> Why is ktype and release part of module code?

Lots of driver defines ktype and ktype->release in its module static
variable.

> 
> What module kobject is causing this problem?

Any modules which defines its ktype and ktype->release in its module
static variable, which is pretty common.

> 
> > Fixes the issue by flushing scheduled kobject cleanup work before
> > freeing module.
> 
> Why are modules special here?
> 
> And if you enable this option, and then start unloading kernel modules,
> yes, things can go wrong, but that's not what this kernel option is for
> at all.
> 
> This feels like a hack for not a real problem.

I think it is caused by CONFIG_DEBUG_KOBJECT_RELEASE, that is why this
patch is posted. Otherwise I'd suggest to remove
CONFIG_DEBUG_KOBJECT_RELEASE, which supposes to not panic kernel since
there isn't anything wrong from driver side.


Thanks,
Ming

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

[Greg Kroah-Hartman]

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.
> 
> Yes, so CONFIG_DEBUG_KOBJECT_RELEASE needs to be fixed.

What is broken with it today?  It is there for you to find problems in
your kernel code that uses kobjects.  What oops/crash/whatever is it
causing that you feel it should not be causing?

A module's kobject is "owned" by the module core, not the module code
that is being unloaded, so I don't see the problem here.  More details
please.

> > > 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.  But again, module
unload is NOT a normal operation and is not what this debugging option
was created to help out with.

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

> > > 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.
> > 
> > Why is module_exit() somehow special here?  What is so odd about that?
> 
> After module_exit() is done, the module will be unloaded, then any code
> or data stored in the module can't be referred.
> 
> > 
> > > When the ->ktype and ->ktype->release are allocated as module static
> > > variable, it can cause trouble because the delayed cleanup handler may
> > > be run after the module is unloaded.
> > 
> > Why is ktype and release part of module code?
> 
> Lots of driver defines ktype and ktype->release in its module static
> variable.

They do?  Where?

> > What module kobject is causing this problem?
> 
> Any modules which defines its ktype and ktype->release in its module
> static variable, which is pretty common.

What non-example code does this?  Let's fix that.

> > > Fixes the issue by flushing scheduled kobject cleanup work before
> > > freeing module.
> > 
> > Why are modules special here?
> > 
> > And if you enable this option, and then start unloading kernel modules,
> > yes, things can go wrong, but that's not what this kernel option is for
> > at all.
> > 
> > This feels like a hack for not a real problem.
> 
> I think it is caused by CONFIG_DEBUG_KOBJECT_RELEASE, that is why this
> patch is posted. Otherwise I'd suggest to remove
> CONFIG_DEBUG_KOBJECT_RELEASE, which supposes to not panic kernel since
> there isn't anything wrong from driver side.

Perhaps just put a nice warning in that debug option that says "beware
of unloading modules with this option enabled."

Or better yet, forbid it if that option is enabled :)

thanks,

greg k-h

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

[Ming Lei]

On Mon, Dec 06, 2021 at 09:04:40AM +0100, 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.
> > 
> > Yes, so CONFIG_DEBUG_KOBJECT_RELEASE needs to be fixed.
> 
> What is broken with it today?  It is there for you to find problems in
> your kernel code that uses kobjects.  What oops/crash/whatever is it
> causing that you feel it should not be causing?
> 
> A module's kobject is "owned" by the module core, not the module code

No, this patch is nothing to do with module's kobject, we are talking
about any kobjects allocated/released from one driver built as module.

> that is being unloaded, so I don't see the problem here.  More details
> please.

If CONFIG_DEBUG_KOBJECT_RELEASE is enabled, kobject_release() will
schedule a (random time)delay work to run kobject_cleanup(), and the delay
work may be run after the module which allocates/frees the kobject is
unloaded.

kobject_cleanup():

	struct kobj_type *t = get_ktype(kobj);

	...
	if (t && t->release) {
                pr_debug("kobject: '%s' (%p): calling ktype release\n",
                         kobject_name(kobj), kobj);
                t->release(kobj);
	}

Both kobj_type and ->release are allocated in the module data/text section,
so kernel panic is triggered when 't && t->release' is run from the
delay work context.

> 
> > > > 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.

Nothing is wrong with kset-example, the issue is just that foo_ktype and
foo_release are allocated in code/data section of the module 'kset-example'.

There are ~150 such uses:

[linux]$ git grep -n "static struct kobj_type" ./  | grep "{" | wc
    153     923   11676

Most of the code can be built as module, so all should have such problem,
that is why I think it as one generic issue, not kset-example specific.
Here kset-example is referred just for showing the issue easily.

> 
> And if you really want to ensure that it works properly, let's wait on
> release before allowing that module to be unloaded.  But again, module

Then all modules which uses kobject need such change.

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

But CONFIG_MODULE and CONFIG_DEBUG_KOBJECT_RELEASE can be enabled at the
same time.

> 
> Again, the confusion between kobjects (which protect data) and module
> references (which protect code) is getting mixed up here.
> 
> > > > 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.
> > > 
> > > Why is module_exit() somehow special here?  What is so odd about that?
> > 
> > After module_exit() is done, the module will be unloaded, then any code
> > or data stored in the module can't be referred.
> > 
> > > 
> > > > When the ->ktype and ->ktype->release are allocated as module static
> > > > variable, it can cause trouble because the delayed cleanup handler may
> > > > be run after the module is unloaded.
> > > 
> > > Why is ktype and release part of module code?
> > 
> > Lots of driver defines ktype and ktype->release in its module static
> > variable.
> 
> They do?  Where?
> 
> > > What module kobject is causing this problem?
> > 
> > Any modules which defines its ktype and ktype->release in its module
> > static variable, which is pretty common.
> 
> What non-example code does this?  Let's fix that.
> 
> > > > Fixes the issue by flushing scheduled kobject cleanup work before
> > > > freeing module.
> > > 
> > > Why are modules special here?
> > > 
> > > And if you enable this option, and then start unloading kernel modules,
> > > yes, things can go wrong, but that's not what this kernel option is for
> > > at all.
> > > 
> > > This feels like a hack for not a real problem.
> > 
> > I think it is caused by CONFIG_DEBUG_KOBJECT_RELEASE, that is why this
> > patch is posted. Otherwise I'd suggest to remove
> > CONFIG_DEBUG_KOBJECT_RELEASE, which supposes to not panic kernel since
> > there isn't anything wrong from driver side.
> 
> Perhaps just put a nice warning in that debug option that says "beware
> of unloading modules with this option enabled."
> 
> Or better yet, forbid it if that option is enabled :)

You mean disabling CONFIG_DEBUG_KOBJECT_RELEASE if CONFIG_MODULE is
enabled?


Thanks,
Ming

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

[Petr Mladek]

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

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

[Ming Lei]

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

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

[kernel test robot]

Hi Ming,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on driver-core/driver-core-testing]
[also build test ERROR on v5.16-rc4 next-20211207]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Ming-Lei/kobject-avoid-to-cleanup-kobject-after-module-is-unloaded/20211129-114940
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 2043727c2882928a10161ddee52b196b7db402fd
config: s390-randconfig-r033-20211207 (https://download.01.org/0day-ci/archive/20211207/202112072131.XiIJR8RS-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/39e48129051282ab6eff0746002e84cf56218bd7
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Ming-Lei/kobject-avoid-to-cleanup-kobject-after-module-is-unloaded/20211129-114940
        git checkout 39e48129051282ab6eff0746002e84cf56218bd7
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=s390 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   lib/kobject.c: In function 'kobject_release':
>> lib/kobject.c:741:29: error: 'module_ktype' undeclared (first use in this function); did you mean 'module_state'?
     741 |         if (kobj->ktype == &module_ktype)
         |                             ^~~~~~~~~~~~
         |                             module_state
   lib/kobject.c:741:29: note: each undeclared identifier is reported only once for each function it appears in


vim +741 lib/kobject.c

   725	
   726	static void kobject_release(struct kref *kref)
   727	{
   728		struct kobject *kobj = container_of(kref, struct kobject, kref);
   729	#ifdef CONFIG_DEBUG_KOBJECT_RELEASE
   730		unsigned long delay = HZ + HZ * (get_random_int() & 0x3);
   731	
   732		/*
   733		 * Don't delay to release module kobject so that we can detect late
   734		 * kobject release more effectively because module unloading waits
   735		 * for completion of module kobject release, see mod_kobject_put.
   736		 *
   737		 * Meantime mod_kobject_put() always waits for completion of module
   738		 * kobject's release, CONFIG_DEBUG_KOBJECT_RELEASE is basically
   739		 * useless for debugging module kobject's release.
   740		 */
 > 741		if (kobj->ktype == &module_ktype)
   742			delay = 0;
   743	
   744		pr_info("kobject: '%s' (%p): %s, parent %p (delayed %ld)\n",
   745			 kobject_name(kobj), kobj, __func__, kobj->parent, delay);
   746		INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
   747	
   748		schedule_delayed_work(&kobj->release, delay);
   749	#else
   750		kobject_cleanup(kobj);
   751	#endif
   752	}
   753	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

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

[Petr Mladek]

On Tue 2021-12-07 20:51:24, Ming Lei wrote:
> 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.

To make it clear. The patch still allows to detect many problems,
like use-after-free.

I wrote "in this scenario". By "this scenario" I mean that a module
might be removed while some kobjects might still use its code.

IMHO, the confusion is because CONFIG_DEBUG_KOBJECT_RELEASE does
really bad job in this scenario. It is supposed to help to catch
these problems. But it actively creates the problem even when
the code is correct.

Your patch is trying to remove the false positives. My concern is
that it reduces the chance to see real problems.


IMHO, there is a design problem:

   + CONFIG_DEBUG_KOBJECT_RELEASE delays the release because nobody
     knows which kobject_put() is the last one.

   + kobject release callback need to call a code from the module. But
     module_exit() does not wait because it is not aware of the still
     referenced callbacks.


By other words, all the crashes caused by CONFIG_DEBUG_KOBJECT_RELEASE and
module removal are yelling at developers that there is this design problem.

IMHO, the right solution is to fix this design problem instead of
calming down CONFIG_DEBUG_KOBJECT_RELEASE.


> > 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.

As explained above. There is a design problem. modules do not wait
for kobject release. kobject release is not synchronous by design.

Motivation for the more complex solution:

Bad reference counting or dependencies of kobjects might cause:

    + memory leak (not good but not fatal)

    + data use after free (serious but there is still chance to
      survive)

    + code use after module removal (always fatal)

From this POV, early module removal is more fatal than other possible
problems. It would deserve some effort.


Regarding the complexity. If the approach with kobject_type worked
then we would need to add ".mod = THIS_MODULE" into all/most statically
defined kobject_type structures:

git grep "static struct kobj_type" | wc -l
156

Even mass change (using coccinelle?) might be safe. THIS_MODULE is NULL
when the code is compiled in. It should use the right module when
the code is built as module...

Of course, it is possible that I miss something important. My view is
based a lot on some feeling/intuition. I am still trying to better
understand and describe it.

Best Regards,
Petr

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

[Ming Lei]

On Tue, Dec 07, 2021 at 03:02:57PM +0100, Petr Mladek wrote:
> On Tue 2021-12-07 20:51:24, Ming Lei wrote:
> > 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.
> 
> To make it clear. The patch still allows to detect many problems,
> like use-after-free.
> 
> I wrote "in this scenario". By "this scenario" I mean that a module
> might be removed while some kobjects might still use its code.
> 
> IMHO, the confusion is because CONFIG_DEBUG_KOBJECT_RELEASE does
> really bad job in this scenario. It is supposed to help to catch
> these problems. But it actively creates the problem even when
> the code is correct.
> 
> Your patch is trying to remove the false positives. My concern is
> that it reduces the chance to see real problems.
> 
> 
> IMHO, there is a design problem:
> 
>    + CONFIG_DEBUG_KOBJECT_RELEASE delays the release because nobody
>      knows which kobject_put() is the last one.
> 
>    + kobject release callback need to call a code from the module. But
>      module_exit() does not wait because it is not aware of the still
>      referenced callbacks.

Yeah, I think it is one fundamental issue. There is neither kobject_put_final()
interface, nor reference counter returned from kobject_put(). Any kobject_put()
called before module_exit() might trigger use after free if this kobject or any
its ancestor's release handler is called after module_exit() returns.

Recently Changhui reports such problem on scsi_debug, turns out the scsi
host's parent device release handler can be called after the scsi driver's
module is unloaded.

sdebug_add_host_helper():

	sdbg_host->dev.release = &sdebug_release_adapter;

> 
> 
> By other words, all the crashes caused by CONFIG_DEBUG_KOBJECT_RELEASE and
> module removal are yelling at developers that there is this design problem.
> 
> IMHO, the right solution is to fix this design problem instead of
> calming down CONFIG_DEBUG_KOBJECT_RELEASE.

Now I agree.

> 
> 
> > > 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.
> 
> As explained above. There is a design problem. modules do not wait
> for kobject release. kobject release is not synchronous by design.

It could be one generic referencing counting vs. module unload.

Either the reference counter data or the associated released handler
can't be referred after the module is unloaded.

> 
> Motivation for the more complex solution:
> 
> Bad reference counting or dependencies of kobjects might cause:
> 
>     + memory leak (not good but not fatal)
> 
>     + data use after free (serious but there is still chance to
>       survive)
> 
>     + code use after module removal (always fatal)
> 
> From this POV, early module removal is more fatal than other possible
> problems. It would deserve some effort.
> 
> 
> Regarding the complexity. If the approach with kobject_type worked
> then we would need to add ".mod = THIS_MODULE" into all/most statically
> defined kobject_type structures:
> 
> git grep "static struct kobj_type" | wc -l
> 156

You didn't count device release handler, which should have more use
cases. And almost every 'struct device' and its container is allocated
dynamically actually, but code is linked statically.


Thanks, 
Ming