[PATCH 0/2] Remove ksets from sysfs eagerly

[PATCH 0/2] Remove ksets from sysfs eagerly

[Bjorn Helgaas]

With CONFIG_DEBUG_KOBJECT_RELEASE=y, kobject release is delayed even after
the last reference to the object is dropped.

With this turned on, Veaceslav and Zdenek noticed that when we enable,
disable, and re-enable MSIs, we often see errors like "sysfs: cannot create
duplicate filename '/devices/pci0000:00/.../msi_irqs'".  This happens
when we unload and reload drivers that use MSI.

The reason is that the MSI disable path uses kset_unregister(dev->msi_kset),
which doesn't remove the kset from sysfs until the the kobject release
function is called.  With CONFIG_DEBUG_KOBJECT_RELEASE, the release is
obviously delayed, but even without it, the release will be delayed if
somebody has the kset sysfs file open when kset_unregister() is called.

We could work around this by explicitly calling
"kobject_del(&dev->msi_kset->kobj)", but that seems clunky and other
kset_unregister() users are likely to have similar problems.

The idea of this patch is to make kset_unregister() unlink the kset from
sysfs immediately, before dropping the kset reference.

---

Bjorn Helgaas (2):
      kobject: remove kset from sysfs immediately in kset_unregister()
      kobject: fix kset sample error path


 Documentation/kobject.txt      |    3 ++-
 lib/kobject.c                  |    1 +
 samples/kobject/kset-example.c |    1 +
 3 files changed, 4 insertions(+), 1 deletion(-)

[PATCH 1/2] kobject: remove kset from sysfs immediately in kset_unregister()

[Bjorn Helgaas]

There's no explicit "unlink from sysfs" interface for ksets, so I think
callers of kset_unregister() expect the kset to be removed from sysfs
immediately, without waiting for the last reference to be released.

This patch makes the sysfs removal happen immediately, so the caller may
create a new kset with the same name as soon as kset_unregister() returns.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 Documentation/kobject.txt |    3 ++-
 lib/kobject.c             |    1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/kobject.txt b/Documentation/kobject.txt
index c5182bb..8e8b501 100644
--- a/Documentation/kobject.txt
+++ b/Documentation/kobject.txt
@@ -342,7 +342,8 @@ kset use:
 
 When you are finished with the kset, call:
   void kset_unregister(struct kset *kset);
-to destroy it.
+to destroy it.  This removes the kset from sysfs and, after the kset
+reference count goes to zero, releases it.
 
 An example of using a kset can be seen in the
 samples/kobject/kset-example.c file in the kernel tree.
diff --git a/lib/kobject.c b/lib/kobject.c
index 9621751..9098992 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -753,6 +753,7 @@ void kset_unregister(struct kset *k)
 {
 	if (!k)
 		return;
+	kobject_del(&k->kobj);
 	kobject_put(&k->kobj);
 }
 

[PATCH 2/2] kobject: fix kset sample error path

[Bjorn Helgaas]

Previously, example_init() leaked a kset if any of the object creations
failed.  This fixes the leak by calling kset_unregister() in the error
path.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 samples/kobject/kset-example.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/samples/kobject/kset-example.c b/samples/kobject/kset-example.c
index d0c687f..5dce351 100644
--- a/samples/kobject/kset-example.c
+++ b/samples/kobject/kset-example.c
@@ -262,6 +262,7 @@ baz_error:
 bar_error:
 	destroy_foo_obj(foo_obj);
 foo_error:
+	kset_unregister(example_kset);
 	return -EINVAL;
 }
 

Re: [PATCH 1/2] kobject: remove kset from sysfs immediately in kset_unregister()

[Greg Kroah-Hartman]

On Tue, Oct 08, 2013 at 02:20:16PM -0600, Bjorn Helgaas wrote:
> There's no explicit "unlink from sysfs" interface for ksets, so I think
> callers of kset_unregister() expect the kset to be removed from sysfs
> immediately, without waiting for the last reference to be released.
> 
> This patch makes the sysfs removal happen immediately, so the caller may
> create a new kset with the same name as soon as kset_unregister() returns.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

With the PCI MSI attribute change, this patch is no longer needed,
right?

thanks,

greg k-h

> ---
>  Documentation/kobject.txt |    3 ++-
>  lib/kobject.c             |    1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/kobject.txt b/Documentation/kobject.txt
> index c5182bb..8e8b501 100644
> --- a/Documentation/kobject.txt
> +++ b/Documentation/kobject.txt
> @@ -342,7 +342,8 @@ kset use:
>  
>  When you are finished with the kset, call:
>    void kset_unregister(struct kset *kset);
> -to destroy it.
> +to destroy it.  This removes the kset from sysfs and, after the kset
> +reference count goes to zero, releases it.
>  
>  An example of using a kset can be seen in the
>  samples/kobject/kset-example.c file in the kernel tree.
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 9621751..9098992 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -753,6 +753,7 @@ void kset_unregister(struct kset *k)
>  {
>  	if (!k)
>  		return;
> +	kobject_del(&k->kobj);
>  	kobject_put(&k->kobj);
>  }
>  

Re: [PATCH 1/2] kobject: remove kset from sysfs immediately in kset_unregister()

[Bjorn Helgaas]

[+cc Matt, Tejun]

On Tue, Dec 03, 2013 at 10:04:25AM -0800, Greg Kroah-Hartman wrote:
> On Tue, Oct 08, 2013 at 02:20:16PM -0600, Bjorn Helgaas wrote:
> > There's no explicit "unlink from sysfs" interface for ksets, so I think
> > callers of kset_unregister() expect the kset to be removed from sysfs
> > immediately, without waiting for the last reference to be released.
> > 
> > This patch makes the sysfs removal happen immediately, so the caller may
> > create a new kset with the same name as soon as kset_unregister() returns.
> > 
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> With the PCI MSI attribute change, this patch is no longer needed,
> right?

Well, yes and no.  The attached patch extends Russell's delayed kobject
release to make the delay somewhat random.  I *think* that should be
valid, but correct me if I'm wrong.

With the random delay patch, it's easy to trigger the "sysfs: cannot
create duplicate filename" error, e.g., by unloading and reloading the
edd module, because the module unload only waits for the /sys/module/edd
object to be released.  Other objects, such as the /sys/firmware/edd
kset, may be released later.

Modules like edd could be changed to explicitly call kobject_del() on
the kset kobject.  Maybe that's a better approach, so we consistently
use kobject_del() to remove things from sysfs.  But I don't know whether
it's really useful to allow the kset to hang around in sysfs after
kset_unregister(), and it's sort of subtle to track down problems like
this.

Several other kset_unregister() callers have this situation, so if we
don't change it to call kobject_del() internally, maybe we should add a
note to Documentation/kobject.txt about calling kobject_del() first.
The existing hint only talks about using it when you need a two-stage
delete, which isn't really the case here.

If we *do* decide to change kset_unregister(), I have an updated
documentation patch that makes it more clear that the release may
happen after kset_unregister() returns.

Bjorn

> > ---
> >  Documentation/kobject.txt |    3 ++-
> >  lib/kobject.c             |    1 +
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/kobject.txt b/Documentation/kobject.txt
> > index c5182bb..8e8b501 100644
> > --- a/Documentation/kobject.txt
> > +++ b/Documentation/kobject.txt
> > @@ -342,7 +342,8 @@ kset use:
> >  
> >  When you are finished with the kset, call:
> >    void kset_unregister(struct kset *kset);
> > -to destroy it.
> > +to destroy it.  This removes the kset from sysfs and, after the kset
> > +reference count goes to zero, releases it.
> >  
> >  An example of using a kset can be seen in the
> >  samples/kobject/kset-example.c file in the kernel tree.
> > diff --git a/lib/kobject.c b/lib/kobject.c
> > index 9621751..9098992 100644
> > --- a/lib/kobject.c
> > +++ b/lib/kobject.c
> > @@ -753,6 +753,7 @@ void kset_unregister(struct kset *k)
> >  {
> >  	if (!k)
> >  		return;
> > +	kobject_del(&k->kobj);
> >  	kobject_put(&k->kobj);
> >  }
> >  



kobject: delay kobject release for random time

From: Bjorn Helgaas <bhelgaas@google.com>

This delays kobject release functions for a random time between 1 and 8
seconds, which effectively changes the order in which they're called.
---
 lib/kobject.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/lib/kobject.c b/lib/kobject.c
index 5b4b8886435e..c87df2f7dbff 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -18,6 +18,7 @@
 #include <linux/export.h>
 #include <linux/stat.h>
 #include <linux/slab.h>
+#include <linux/random.h>
 
 /**
  * kobject_namespace - return @kobj's namespace tag
@@ -625,10 +626,13 @@ static void kobject_release(struct kref *kref)
 {
 	struct kobject *kobj = container_of(kref, struct kobject, kref);
 #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
-	pr_info("kobject: '%s' (%p): %s, parent %p (delayed)\n",
-		 kobject_name(kobj), kobj, __func__, kobj->parent);
+	unsigned long delay = HZ + HZ * (get_random_int() & 0x3);
+
+	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);
-	schedule_delayed_work(&kobj->release, HZ);
+
+	schedule_delayed_work(&kobj->release, delay);
 #else
 	kobject_cleanup(kobj);
 #endif

Re: [PATCH 1/2] kobject: remove kset from sysfs immediately in kset_unregister()

[Greg Kroah-Hartman]

On Tue, Dec 03, 2013 at 03:41:43PM -0700, Bjorn Helgaas wrote:
> [+cc Matt, Tejun]
> 
> On Tue, Dec 03, 2013 at 10:04:25AM -0800, Greg Kroah-Hartman wrote:
> > On Tue, Oct 08, 2013 at 02:20:16PM -0600, Bjorn Helgaas wrote:
> > > There's no explicit "unlink from sysfs" interface for ksets, so I think
> > > callers of kset_unregister() expect the kset to be removed from sysfs
> > > immediately, without waiting for the last reference to be released.
> > > 
> > > This patch makes the sysfs removal happen immediately, so the caller may
> > > create a new kset with the same name as soon as kset_unregister() returns.
> > > 
> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > With the PCI MSI attribute change, this patch is no longer needed,
> > right?
> 
> Well, yes and no.  The attached patch extends Russell's delayed kobject
> release to make the delay somewhat random.  I *think* that should be
> valid, but correct me if I'm wrong.

Heh, I like it, mind if I queue it up?

> With the random delay patch, it's easy to trigger the "sysfs: cannot
> create duplicate filename" error, e.g., by unloading and reloading the
> edd module, because the module unload only waits for the /sys/module/edd
> object to be released.  Other objects, such as the /sys/firmware/edd
> kset, may be released later.
> 
> Modules like edd could be changed to explicitly call kobject_del() on
> the kset kobject.  Maybe that's a better approach, so we consistently
> use kobject_del() to remove things from sysfs.  But I don't know whether
> it's really useful to allow the kset to hang around in sysfs after
> kset_unregister(), and it's sort of subtle to track down problems like
> this.
> 
> Several other kset_unregister() callers have this situation, so if we
> don't change it to call kobject_del() internally, maybe we should add a
> note to Documentation/kobject.txt about calling kobject_del() first.
> The existing hint only talks about using it when you need a two-stage
> delete, which isn't really the case here.
> 
> If we *do* decide to change kset_unregister(), I have an updated
> documentation patch that makes it more clear that the release may
> happen after kset_unregister() returns.

Ok, I'll take this patch, it makes sense on the module unload path for
example.  Thanks for the explanation.

greg k-h

Re: [PATCH 1/2] kobject: remove kset from sysfs immediately in kset_unregister()

[Bjorn Helgaas]

On Thu, Dec 5, 2013 at 5:01 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Dec 03, 2013 at 03:41:43PM -0700, Bjorn Helgaas wrote:
>> [+cc Matt, Tejun]
>>
>> On Tue, Dec 03, 2013 at 10:04:25AM -0800, Greg Kroah-Hartman wrote:
>> > On Tue, Oct 08, 2013 at 02:20:16PM -0600, Bjorn Helgaas wrote:
>> > > There's no explicit "unlink from sysfs" interface for ksets, so I think
>> > > callers of kset_unregister() expect the kset to be removed from sysfs
>> > > immediately, without waiting for the last reference to be released.
>> > >
>> > > This patch makes the sysfs removal happen immediately, so the caller may
>> > > create a new kset with the same name as soon as kset_unregister() returns.
>> > >
>> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> >
>> > With the PCI MSI attribute change, this patch is no longer needed,
>> > right?
>>
>> Well, yes and no.  The attached patch extends Russell's delayed kobject
>> release to make the delay somewhat random.  I *think* that should be
>> valid, but correct me if I'm wrong.
>
> Heh, I like it, mind if I queue it up?

Sure, that'd be fine.  I'll resend these two patches with my sign-off
and updated changelogs.

>> With the random delay patch, it's easy to trigger the "sysfs: cannot
>> create duplicate filename" error, e.g., by unloading and reloading the
>> edd module, because the module unload only waits for the /sys/module/edd
>> object to be released.  Other objects, such as the /sys/firmware/edd
>> kset, may be released later.
>>
>> Modules like edd could be changed to explicitly call kobject_del() on
>> the kset kobject.  Maybe that's a better approach, so we consistently
>> use kobject_del() to remove things from sysfs.  But I don't know whether
>> it's really useful to allow the kset to hang around in sysfs after
>> kset_unregister(), and it's sort of subtle to track down problems like
>> this.
>>
>> Several other kset_unregister() callers have this situation, so if we
>> don't change it to call kobject_del() internally, maybe we should add a
>> note to Documentation/kobject.txt about calling kobject_del() first.
>> The existing hint only talks about using it when you need a two-stage
>> delete, which isn't really the case here.
>>
>> If we *do* decide to change kset_unregister(), I have an updated
>> documentation patch that makes it more clear that the release may
>> happen after kset_unregister() returns.
>
> Ok, I'll take this patch, it makes sense on the module unload path for
> example.  Thanks for the explanation.