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

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


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           | 67 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)

-- 
2.31.1

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

[Ming Lei]

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;
+
 	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

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

[Greg Kroah-Hartman]

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.

thanks,

greg k-h

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

[Ming Lei]

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.


Thanks,
Ming

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

[Greg Kroah-Hartman]

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

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

[Ming Lei]

On Fri, Nov 26, 2021 at 05:33:45PM +0100, Greg Kroah-Hartman wrote:
> 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?

So far, module kobject code is always released in sync way, see
mod_kobject_put(), and CONFIG_DEBUG_KOBJECT_RELEASE is basically useless
for module kobject.

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

OK, I will add comment on this special usage in V2 so that you can
review it further.


Thanks,
Ming

[PATCH 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           | 62 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index ea30529fba08..e5e3419cf36b 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 4c0dbe11be3d..f5fd6017d8ce 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,49 @@ 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);
+
+	if (val != MODULE_STATE_GOING)
+		return NOTIFY_DONE;
+
+	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));
+
+	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 +779,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;
 
 	if (kobj->ktype == &module_ktype)
 		delay = 0;
@@ -736,6 +788,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);
@@ -1146,3 +1202,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 2/2] kobject: wait until kobject is cleaned up before freeing module

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2835 bytes --]

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.15 next-20211105]
[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/20211105-144026
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 27e0bcd02990f7291adb0f111e300f06c495d509
config: mips-buildonly-randconfig-r003-20211105 (attached as .config)
compiler: mipsel-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/4a9e015dd3d3e39d5723ea46579ba21f5394806a
        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/20211105-144026
        git checkout 4a9e015dd3d3e39d5723ea46579ba21f5394806a
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=mips 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 'kobj_module_callback':
>> lib/kobject.c:741:20: error: 'MODULE_STATE_GOING' undeclared (first use in this function)
     741 |         if (val != MODULE_STATE_GOING)
         |                    ^~~~~~~~~~~~~~~~~~
   lib/kobject.c:741:20: note: each undeclared identifier is reported only once for each function it appears in


vim +/MODULE_STATE_GOING +741 lib/kobject.c

   730	
   731	#ifdef CONFIG_DEBUG_KOBJECT_RELEASE
   732	/*
   733	 * Module notifier call back, flushing scheduled kobject cleanup work
   734	 * before freeing module
   735	 */
   736	static int kobj_module_callback(struct notifier_block *nb,
   737					   unsigned long val, void *data)
   738	{
   739		LIST_HEAD(pending);
   740	
 > 741		if (val != MODULE_STATE_GOING)
   742			return NOTIFY_DONE;
   743	
   744		spin_lock_irq(&kobj_cleanup_lock);
   745		list_splice_init(&kobj_cleanup_list, &pending);
   746		spin_unlock_irq(&kobj_cleanup_lock);
   747	
   748		while (!list_empty_careful(&pending))
   749			msleep(jiffies_to_msecs(HZ / 10));
   750	
   751		flush_scheduled_work();
   752		return NOTIFY_DONE;
   753	}
   754	

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28594 bytes --]

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

[Ming Lei]

On Fri, Nov 05, 2021 at 10:10:07PM +0800, kernel test robot wrote:
> 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.15 next-20211105]
> [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/20211105-144026
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 27e0bcd02990f7291adb0f111e300f06c495d509
> config: mips-buildonly-randconfig-r003-20211105 (attached as .config)
> compiler: mipsel-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/4a9e015dd3d3e39d5723ea46579ba21f5394806a
>         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/20211105-144026
>         git checkout 4a9e015dd3d3e39d5723ea46579ba21f5394806a
>         # save the attached .config to linux build tree
>         mkdir build_dir
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=mips 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 'kobj_module_callback':
> >> lib/kobject.c:741:20: error: 'MODULE_STATE_GOING' undeclared (first use in this function)
>      741 |         if (val != MODULE_STATE_GOING)
>          |                    ^~~~~~~~~~~~~~~~~~
>    lib/kobject.c:741:20: note: each undeclared identifier is reported only once for each function it appears in

Hello,

Thanks for the report!

This must be triggered when CONFIG_MODULES is off, and the following patch
should kill the failure:


diff --git a/lib/kobject.c b/lib/kobject.c
index f5fd6017d8ce..c054dca0001a 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -738,8 +738,10 @@ static int kobj_module_callback(struct notifier_block *nb,
 {
 	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);


Thanks,
Ming

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

[Petr Mladek]

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. But I wonder if we could create API that might be used by
drivers and report the actuall bug. Something like:


First, update kobject_put() so that it returns 1 when the release
method was called:

/**
 * __kobject_put - decrement kobject refcount
 * @kobject: pointer to kobject
 *
 * Decrement the refcount, and if 0, call release().
 * Return 1 if the object was removed, otherwise return 0.  Beware, if this
 * function returns 0, you still can not count on the kref from remaining in
 * memory.  Only use the return value if you want to see if the kref is now
 * gone, not present.
 */
int __kobject_put(struct kobject *kobj)
{
	if (!kobj)
		return 0;

	if (!kobj->state_initialized) {
		WARN(1, KERN_WARNING
		     "kobject: '%s' (%p): is not initialized, yet kobject_put() is being called.\n",
		     kobject_name(kobj), kobj);
	}

	return kref_put(&kobj->kref, kobject_release);
}

/**
 * kobject_remove_sync - remove the kobject in two stages and wait
 *      until it is removed.
 * @kobject: pointer to kobject
 *
 * Return 0 on success, -EBUSY when someone else still owns a
 *     reference on the kobject.
 *
 * IMPORTANT: The caller is reponsible that nobody else has explicit
 *	reference on the kobject. The only expection are callbacks
 *	used by the related sysfs interface. The two stage removal
 *	makes sure that there is no pending operation when
 *	the final put is called.
 */
int kobject_remove_sync(struct kobject *kobj)
{
#ifdef CONFIG_DEBUG_KOBJECT_RELEASE
	DECLARE_COMPLETION(released);

	if (kobj)
		kobj->released = released;
#endif

	/* Remove sysfs */
	kobject_del(kobj);

	/* This should be the final put */
	if (WARN(!__kobject_put(kobj), "Synchronous  kobject release failed. Someone still had a reference.\n")) 
		return -EBUSY;

#ifdef CONFIG_DEBUG_KOBJECT_RELEASE
	wait_for_completion(&released);
#endif
	return 0;
}

, where the pointer struct completion *released will be added
into struct kobject. And kobject_cleanup() will call complete()
after the object is released.

The completion must be defined in kobject_remove_sync() and passed
via pointer. It is because struct kobject itself might be freed
by release() callback.


> 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.
> 
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -716,11 +729,49 @@ 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);
> +
> +	if (val != MODULE_STATE_GOING)
> +		return NOTIFY_DONE;
> +
> +	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));

I was curious why this is needed. I guess that it is because
flush_sched_work() will not wait for delayed work items that
are still waiting for the timer. Am I right, please?

> +
> +	flush_scheduled_work();

I guess that this is needed because the kobj is removed from the list
before it is released. Am I right, please?

It would be worth to document it. IMHO, it is not obvious why
it is that complicated.


More thoughts:

The advantage of the module going callback is that it is transparent.
It should fix the problem for most existing users in most situations.

But it will solve the problem only when someone puts the final
reference before the module going callback is called. It does
not guarantee that it really works.

Note that CONFIG_DEBUG_KOBJECT_RELEASE and the workqueue is there
only to make exactly this problem more visible. I mean that
the final kobject_put() need not be final.

It would be great to have somehing for users that want to know
that they do the right thing. For example, it is a must-to-have
for the livepatch code. IMHO, kobject_remove_sync() or something
similar would make the kobject API more safe to use.

> +	return NOTIFY_DONE;
> +}
> +

Best Regards,
Petr

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

[Ming Lei]

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.

The usual module use model is that module can't be used if someone is
use it. After module_exit() is started, no one should & can 'use' the module
any more, that is done by module's refcnt.

So far the driver core subsystem doesn't use module owner info, so no
need to grab module refcnt in case that kobject is used. And that
actually doesn't work here, given we can't hold the module refcnt
during the kobject's lifetime, otherwise the module won't be unloaded
at all. Sort of chicken and egg problem given kobject is often released
during module_exit().

But driver core does provide kobject_del() interface to wait until
all show()/store() are done.

So once the driver said now no one uses that device and the module
can be unloaded, then kobject_del() is done inside module_exit(),
who can hold kobject's extra reference? If there is, the caller should
have grabbed the module refcnt to prevent module from being unloaded.

We usually think it is driver bug, see one fixed recently:

https://lore.kernel.org/linux-scsi/20211008050118.1440686-1-ming.lei@redhat.com/

Do you have real report?

> But I wonder if we could create API that might be used by
> drivers and report the actuall bug. Something like:
> 
> 
> First, update kobject_put() so that it returns 1 when the release
> method was called:
> 
> /**
>  * __kobject_put - decrement kobject refcount
>  * @kobject: pointer to kobject
>  *
>  * Decrement the refcount, and if 0, call release().
>  * Return 1 if the object was removed, otherwise return 0.  Beware, if this
>  * function returns 0, you still can not count on the kref from remaining in
>  * memory.  Only use the return value if you want to see if the kref is now
>  * gone, not present.
>  */
> int __kobject_put(struct kobject *kobj)
> {
> 	if (!kobj)
> 		return 0;
> 
> 	if (!kobj->state_initialized) {
> 		WARN(1, KERN_WARNING
> 		     "kobject: '%s' (%p): is not initialized, yet kobject_put() is being called.\n",
> 		     kobject_name(kobj), kobj);
> 	}
> 
> 	return kref_put(&kobj->kref, kobject_release);
> }
> 
> /**
>  * kobject_remove_sync - remove the kobject in two stages and wait
>  *      until it is removed.
>  * @kobject: pointer to kobject
>  *
>  * Return 0 on success, -EBUSY when someone else still owns a
>  *     reference on the kobject.
>  *
>  * IMPORTANT: The caller is reponsible that nobody else has explicit
>  *	reference on the kobject. The only expection are callbacks
>  *	used by the related sysfs interface. The two stage removal
>  *	makes sure that there is no pending operation when
>  *	the final put is called.
>  */
> int kobject_remove_sync(struct kobject *kobj)
> {
> #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
> 	DECLARE_COMPLETION(released);
> 
> 	if (kobj)
> 		kobj->released = released;
> #endif
> 
> 	/* Remove sysfs */
> 	kobject_del(kobj);
> 
> 	/* This should be the final put */
> 	if (WARN(!__kobject_put(kobj), "Synchronous  kobject release failed. Someone still had a reference.\n")) 
> 		return -EBUSY;

Not sure if this interface is useful:

1) who need this interface?
- It is basically not possible to audit all drivers for the conversion, and not
necessary too.

2) this may break some common open()/release model:

- user open() one device, here module refcnt is hold, and device/kobject refcnt
  is hold too

- device needs to be deleted by kobject_del() via sysfs or ioctl or
  others, if kobject_remove_sync() is used, it will complain, but it
  is just false warning. There are lots of such examples.

3) this way may break some uses if spin_lock() is held before calling
kobject_put().

4) not usable in deleting kobject itself, or break the deleting me interface
simply.

5) actually only one implicit rule is here: kobject needs to be released
before module exit is done if the kobject is created by this module

6) much less flexible than the usual two stage removal

So IMO, it is wrong direction for fixing the 'issue'.

More importantly I'd see one real such report first before figuring out solution,
then we can see if it is driver issue or generic kobject API problem, doesn't it
make sense?

> 
> #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
> 	wait_for_completion(&released);
> #endif
> 	return 0;
> }
> 
> , where the pointer struct completion *released will be added
> into struct kobject. And kobject_cleanup() will call complete()
> after the object is released.
> 
> The completion must be defined in kobject_remove_sync() and passed
> via pointer. It is because struct kobject itself might be freed
> by release() callback.
> 
> 
> > 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.
> > 
> > --- a/lib/kobject.c
> > +++ b/lib/kobject.c
> > @@ -716,11 +729,49 @@ 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);
> > +
> > +	if (val != MODULE_STATE_GOING)
> > +		return NOTIFY_DONE;
> > +
> > +	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));
> 
> I was curious why this is needed. I guess that it is because
> flush_sched_work() will not wait for delayed work items that
> are still waiting for the timer. Am I right, please?

Right.

Actually flush_workqueue() can't cover delayed work item too,
also asynchronous function doesn't support delayed function.

> 
> > +
> > +	flush_scheduled_work();
> 
> I guess that this is needed because the kobj is removed from the list
> before it is released. Am I right, please?

Right.

> 
> It would be worth to document it. IMHO, it is not obvious why
> it is that complicated.

Fine. As I mentioned, I tried to do it via single kernel API(flush_work
queue or asynchronous function), but there isn't anyone who is capable
of the requirement.

> 
> 
> More thoughts:
> 
> The advantage of the module going callback is that it is transparent.
> It should fix the problem for most existing users in most situations.
> 
> But it will solve the problem only when someone puts the final
> reference before the module going callback is called. It does
> not guarantee that it really works.

I think that is one implicit rule. The simple question is that
after module is unloaded, who will/can use that kobject? Isn't it
a driver bug?

> 
> Note that CONFIG_DEBUG_KOBJECT_RELEASE and the workqueue is there
> only to make exactly this problem more visible. I mean that
> the final kobject_put() need not be final.

No, I don't understand 'the final kobject_put() need not be final',
isn't the kobject_remove_sync() done for the final release?

> 
> It would be great to have somehing for users that want to know
> that they do the right thing. For example, it is a must-to-have
> for the livepatch code. IMHO, kobject_remove_sync() or something
> similar would make the kobject API more safe to use.

kobject_remove_sync() isn't good, see my above comment. Even it can't
fix livepatch after the wq hack is removed.


Thanks,
Ming

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

[Petr Mladek]

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?

My point is that if this makes CONFIG_DEBUG_KOBJECT_RELEASE useless
than we should remove CONFIG_DEBUG_KOBJECT_RELEASE instead.
But then we need something else to prevent bugs that this
functionality helped to debug.


> The usual module use model is that module can't be used if someone is
> use it. After module_exit() is started, no one should & can 'use' the module
> any more, that is done by module's refcnt.

This statement is suspicious. IMHO, no one should 'use' the module
after module_exit() _finishes_.

It should be perfectly fine to use the module after module_exit()
_starts_ as long as module_exit() callback could wait until
the existing users stop using the module.


> So far the driver core subsystem doesn't use module owner info, so no
> need to grab module refcnt in case that kobject is used. And that
> actually doesn't work here, given we can't hold the module refcnt
> during the kobject's lifetime, otherwise the module won't be unloaded
> at all. Sort of chicken and egg problem given kobject is often released
> during module_exit().
>
> But driver core does provide kobject_del() interface to wait until
> all show()/store() are done.
>
> So once the driver said now no one uses that device and the module
> can be unloaded, then kobject_del() is done inside module_exit(),
> who can hold kobject's extra reference? If there is, the caller should
> have grabbed the module refcnt to prevent module from being unloaded.
> 
> We usually think it is driver bug, see one fixed recently:
> 
> https://lore.kernel.org/linux-scsi/20211008050118.1440686-1-ming.lei@redhat.com/

Honestly, I do not understand how the extra module_get()/module_put()
could prevent the race caused by delayed kobject clean up.

scsi_device_dev_release() uses try_module_get(). But it always calls
scsi_device_dev_release_usercontext(). Why is it safe when
try_module_get() failed?

I guess that the patch reduced the size of the race window but
a race might still be there.

Anyway, it is pity that the commit message does not include the
backtrace of the page fault. Or that it does not better describe
the race that is fixed there.


> > But I wonder if we could create API that might be used by
> > drivers and report the actuall bug. Something like:
> >
> > int kobject_remove_sync(struct kobject *kobj)
>
> Not sure if this interface is useful:
> 
> 1) who need this interface?
> - It is basically not possible to audit all drivers for the conversion, and not
> necessary too.

It might be useful for people that want to create interface and
drivers that are safe to unload.


> 2) this may break some common open()/release model:
> 
> - user open() one device, here module refcnt is hold, and device/kobject refcnt
>   is hold too
> 
> - device needs to be deleted by kobject_del() via sysfs or ioctl or
>   others, if kobject_remove_sync() is used, it will complain, but it
>   is just false warning. There are lots of such examples.

It might be solved the same way as sysfs_break_active_protection().
I mean that the API might be aware of that it is removing itself.


> 3) this way may break some uses if spin_lock() is held before calling
> kobject_put().

This is not specific to the new API. The same problem is also with kobject_del().


> 4) not usable in deleting kobject itself, or break the deleting me interface
> simply.

It will be useful when the deleting is offloaded to a workqueue.
I still consider is less hacky than using sysfs_break_active_protection().


> 5) actually only one implicit rule is here: kobject needs to be released
> before module exit is done if the kobject is created by this module

It the kobject is created by a module, it should be also destroyed
by the module. And this is where some remove_kobject() API might
be useful.


> 6) much less flexible than the usual two stage removal

We still could create a two stage API. The point is that it might be
useful to have an API that will wait until the kobject is really
released.

Best Regards,
Petr

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

[Ming Lei]

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?

> 
> My point is that if this makes CONFIG_DEBUG_KOBJECT_RELEASE useless

No, that isn't true. CONFIG_DEBUG_KOBJECT_RELEASE still works in its
original way and the delay is still there, what this patch is doing is
to make it safe by completing the delayed cleanup before freeing module,
because ->ktype & related callbacks are often allocated in module static
memory.

> than we should remove CONFIG_DEBUG_KOBJECT_RELEASE instead.
> But then we need something else to prevent bugs that this
> functionality helped to debug.
> 
> 
> > The usual module use model is that module can't be used if someone is
> > use it. After module_exit() is started, no one should & can 'use' the module
> > any more, that is done by module's refcnt.
> 
> This statement is suspicious. IMHO, no one should 'use' the module
> after module_exit() _finishes_.

I said after module_exit() is started, not module_exit() is finished.

> 
> It should be perfectly fine to use the module after module_exit()
> _starts_ as long as module_exit() callback could wait until
> the existing users stop using the module.

No, once module_exit() is started, try_module_get() will return false,
so module isn't supposed to be used any more. See delete_module(),
->exit() is only called in case of module refcnt being zero.

> 
> 
> > So far the driver core subsystem doesn't use module owner info, so no
> > need to grab module refcnt in case that kobject is used. And that
> > actually doesn't work here, given we can't hold the module refcnt
> > during the kobject's lifetime, otherwise the module won't be unloaded
> > at all. Sort of chicken and egg problem given kobject is often released
> > during module_exit().
> >
> > But driver core does provide kobject_del() interface to wait until
> > all show()/store() are done.
> >
> > So once the driver said now no one uses that device and the module
> > can be unloaded, then kobject_del() is done inside module_exit(),
> > who can hold kobject's extra reference? If there is, the caller should
> > have grabbed the module refcnt to prevent module from being unloaded.
> > 
> > We usually think it is driver bug, see one fixed recently:
> > 
> > https://lore.kernel.org/linux-scsi/20211008050118.1440686-1-ming.lei@redhat.com/
> 
> Honestly, I do not understand how the extra module_get()/module_put()
> could prevent the race caused by delayed kobject clean up.

If module_get() is successful, module_exit() can't be called
until the module refcnt is released.

> 
> scsi_device_dev_release() uses try_module_get(). But it always calls
> scsi_device_dev_release_usercontext(). Why is it safe when
> try_module_get() failed?
> 
> I guess that the patch reduced the size of the race window but
> a race might still be there.
> 
> Anyway, it is pity that the commit message does not include the
> backtrace of the page fault. Or that it does not better describe
> the race that is fixed there.
> 
> 
> > > But I wonder if we could create API that might be used by
> > > drivers and report the actuall bug. Something like:
> > >
> > > int kobject_remove_sync(struct kobject *kobj)
> >
> > Not sure if this interface is useful:
> > 
> > 1) who need this interface?
> > - It is basically not possible to audit all drivers for the conversion, and not
> > necessary too.
> 
> It might be useful for people that want to create interface and
> drivers that are safe to unload.

It is one generic issue because most of ->ktype and related callbacks are stored
in module static memory, so all modules should be unloaded safely. If the interface
is created, in theory the last kobject_put() called in _all__ module_exit() should be
converted to this new interface. That is why I said it is basically not possible,
cause it isn't easy to figure one which is the last one in each
driver/module.

Again, any real report which needs such new interface? If there isn't, I
don't think it makes sense to take effort for making one, especially the
new API is very hard to use/convert.

> 
> 
> > 2) this may break some common open()/release model:
> > 
> > - user open() one device, here module refcnt is hold, and device/kobject refcnt
> >   is hold too
> > 
> > - device needs to be deleted by kobject_del() via sysfs or ioctl or
> >   others, if kobject_remove_sync() is used, it will complain, but it
> >   is just false warning. There are lots of such examples.
> 
> It might be solved the same way as sysfs_break_active_protection().
> I mean that the API might be aware of that it is removing itself.
> 
> 
> > 3) this way may break some uses if spin_lock() is held before calling
> > kobject_put().
> 
> This is not specific to the new API. The same problem is also with kobject_del().

No, spin_lock can't be held for kobject_del() which will sleep always.

> 
> 
> > 4) not usable in deleting kobject itself, or break the deleting me interface
> > simply.
> 
> It will be useful when the deleting is offloaded to a workqueue.

How is it useful?

> I still consider is less hacky than using sysfs_break_active_protection().

OK, if you think wq is better, care to post a patch to replace
sysfs_break_active_protection() with justification so that every user
can benefit from it?

> 
> 
> > 5) actually only one implicit rule is here: kobject needs to be released
> > before module exit is done if the kobject is created by this module
> 
> It the kobject is created by a module, it should be also destroyed
> by the module. And this is where some remove_kobject() API might
> be useful.

Yeah, but why new API is required, you have to provide one real report
for supporting the idea of new API.

> 
> 
> > 6) much less flexible than the usual two stage removal
> 
> We still could create a two stage API. The point is that it might be
> useful to have an API that will wait until the kobject is really
> released.

You have to know which one is the final release first, not mention not
every kobject_put() can wait.


Thanks,
Ming

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

[Greg Kroah-Hartman]

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.

I've been ignoring this thread until after 5.16-rc1 is out, sorry.

greg k-h

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

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

[Greg Kroah-Hartman]

On Wed, Nov 10, 2021 at 10:05:12AM +0100, Petr Mladek wrote:
> 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?

The most obvious issue is when release functions access things that they
shouldn't be accessing because either the code is gone, or the data is
gone.

I think some problems in the tty layer are still present if you enable
this option, but I can't remember the details, sorry.  Search the
archives for when this was added to the tree, or for bug reports with
the option enabled for more details.

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

This option is there for debugging kernel code.  If your patch series
makes it obsolete, that's fine, as long as we still have a way to still
debug these types of things easily.

I'll review this after 5.16-rc1 is out.

thanks,

greg k-h