[PATCH 000/003] Attempt to cope with device changes and delayed kobject deallocation

[PATCH 000/003] Attempt to cope with device changes and delayed kobject deallocation

[Richard Watts]

Sometimes (eg. when a ttyACM device undergoes a usb reset) we get
into the situation where we are destroying and recreating a device
object (specifically, the tty representing the ttyACM device)
very quickly.

When CONFIG_DEBUG_KOBJECT_RELEASE=y, kobject release is delayed
and this allows the new ttyACM0 sysfs object to be linked to the
old (zero-referenced) tty sysfs object.

That object is then destroyed by the delayed kobject release and
this leads to an oops.

This patchset avoids that oops by:

  - using kref_get_unless_zero() to obtain the parent object.
  - inserting some code into sysfs_create_dir() to stop it
     complaining about duplicate directory names if the
     name we are a duplicate of has a zero reference count.

This looks like a pretty grotty way to get around it to me,
but perhaps it will prompt some discussion of what the right
way to fix this is (or indeed if we should simply leave it be
and have spurious oopses when CONFIG_DEBUG_KOBJECT_RELEASE=y).

Comments,  brickbats, etc. very welcome,

Patch against e26081808edadfd257c6c9d81014e3b25e9a6118
(master of
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git )


Richard.

[PATCH 001/003] Attempt to cope with device changes and delayed kobject deallocation

[Richard Watts]

Expose kobject_get_unless_zero() which will shortly be
  needed by get_device_parent().

Signed-off-by: Richard Watts <rrw@kynesim.co.uk>
---
  include/linux/kobject.h | 1 +
  lib/kobject.c           | 2 +-
  2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index 2d61b90..662b136 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -107,6 +107,7 @@ extern int __must_check kobject_rename(struct 
kobject *, const char *new_name);
  extern int __must_check kobject_move(struct kobject *, struct kobject *);

  extern struct kobject *kobject_get(struct kobject *kobj);
+extern struct kobject * __must_check kobject_get_unless_zero(struct 
kobject *kobj);
  extern void kobject_put(struct kobject *kobj);

  extern const void *kobject_namespace(struct kobject *kobj);
diff --git a/lib/kobject.c b/lib/kobject.c
index 3b841b9..3ba1db4 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -586,7 +586,7 @@ struct kobject *kobject_get(struct kobject *kobj)
  	return kobj;
  }

-static struct kobject * __must_check kobject_get_unless_zero(struct 
kobject *kobj)
+struct kobject * __must_check kobject_get_unless_zero(struct kobject *kobj)
  {
  	if (!kref_get_unless_zero(&kobj->kref))
  		kobj = NULL;
-- 
1.9.1

Re: [PATCH 001/003] Attempt to cope with device changes and delayed kobject deallocation

[Alex Bennée]

Richard Watts <rrw@kynesim.co.uk> writes:

> Expose kobject_get_unless_zero() which will shortly be
>   needed by get_device_parent().
>
> Signed-off-by: Richard Watts <rrw@kynesim.co.uk>
> ---
>   include/linux/kobject.h | 1 +
>   lib/kobject.c           | 2 +-
>   2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/kobject.h b/include/linux/kobject.h
> index 2d61b90..662b136 100644
> --- a/include/linux/kobject.h
> +++ b/include/linux/kobject.h
> @@ -107,6 +107,7 @@ extern int __must_check kobject_rename(struct 
> kobject *, const char *new_name);
>   extern int __must_check kobject_move(struct kobject *, struct kobject *);
>
>   extern struct kobject *kobject_get(struct kobject *kobj);
> +extern struct kobject * __must_check kobject_get_unless_zero(struct 
> kobject *kobj);
>   extern void kobject_put(struct kobject *kobj);
>
>   extern const void *kobject_namespace(struct kobject *kobj);
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 3b841b9..3ba1db4 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -586,7 +586,7 @@ struct kobject *kobject_get(struct kobject *kobj)
>   	return kobj;
>   }
>
> -static struct kobject * __must_check kobject_get_unless_zero(struct 
> kobject *kobj)
> +struct kobject * __must_check kobject_get_unless_zero(struct kobject *kobj)
>   {
>   	if (!kref_get_unless_zero(&kobj->kref))
>   		kobj = NULL;

Hi Richard,

checkpatch is complaining about some trailing whitespace in this patch.

> -- 
> 1.9.1

-- 
Alex Bennée

[PATCH 002/003] Attempt to cope with device changes and delayed kobject deallocation

[Richard Watts]

Avoid attempts to use device parents with zero refcounts

Signed-off-by: Richard Watts <rrw@kynesim.co.uk>
---
  drivers/base/core.c | 8 +++++++-
  1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 21d1303..207fbb2 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -762,7 +762,13 @@ static struct kobject *get_device_parent(struct 
device *dev,
  		spin_lock(&dev->class->p->glue_dirs.list_lock);
  		list_for_each_entry(k, &dev->class->p->glue_dirs.list, entry)
  			if (k->parent == parent_kobj) {
-				kobj = kobject_get(k);
+				/* Be careful: an old parent may still
+				 * be in this list awaiting deletion, despite
+				 * having a refcount of 0. We don't want to
+				 * add it here and then have some other thread
+				 * delete it.
+				 */
+				kobj = kobject_get_unless_zero(k);
  				break;
  			}
  		spin_unlock(&dev->class->p->glue_dirs.list_lock);
-- 
1.9.1

[PATCH 003/003] Attempt to cope with device changes and delayed kobject deallocation

[Richard Watts]

Avoid issuing duplicate name errors when a kobject's sysfs directory is
scheduled for deletion but has not yet been deleted, and a directory
  with the same name is created.

Signed-off-by: Richard Watts <rrw@kynesim.co.uk>
---
  fs/sysfs/dir.c | 15 +++++++++++++++
  1 file changed, 15 insertions(+)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 0b45ff4..e85de14 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -49,6 +49,21 @@ int sysfs_create_dir_ns(struct kobject *kobj, const 
void *ns)
  	else
  		parent = sysfs_root_kn;

+	kn = kernfs_find_and_get_ns(parent, kobject_name(kobj),
+				ns);
+	/* This check is non-atomic, but that is OK - the worst thing
+	 *  that can happen is a concurrent removal of the sysfs dir,
+	 *  which is exactly what we want anyway.
+	 */
+	if (kn) {
+		struct kobject *kobj_exist = (struct kobject *)kn->priv;
+
+		if (kobject_get_unless_zero(kobj_exist))
+			kobject_put(kobj_exist);
+		else
+			sysfs_remove_dir(kobj_exist);
+	}
+
  	if (!parent)
  		return -ENOENT;

-- 
1.9.1