Attempt to cope with device changes and delayed kobject deallocation
Subsystems (2)
cover Attempt to cope with device changes and delayed kobject deallocation
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 ofgit://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git )
Richard.
PATCH 1/3 Attempt to cope with device changes and delayed kobject deallocation
Expose kobject_get_unless_zero() which will shortly be
needed by get_device_parent().
--- include/linux/kobject.h | 1 + lib/kobject.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)
include/linux/kobject.h +1 -0
@@ -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);
lib/kobject.c +1 -1
@@ -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 ;
Signature · 1 line
1.9.1
Quoted 36 lines · Richard Watts
Expose
kobject_get_unless_zero()which will shortly be
needed byget_device_parent().Signed-off-by Richard Watts <[email protected]>--- include/linux/kobject.h | 1 + lib/kobject.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)include/linux/kobject.h +1
-0@@ -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);lib/kobject.c +1
-1@@ -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))
Quoted 1 line
kobj = NULL;
Hi Richard,
checkpatch is complaining about some trailing whitespace in this patch.
Quoted 2 lines
Signature · 1 line
1.9.1
Signature · 1 line
Alex Bennée
PATCH 2/3 Attempt to cope with device changes and delayed kobject deallocation
In reply to Richard WattsAvoid attempts to use device parents with zero refcounts
--- drivers/base/core.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/base/core.c +7 -1
@@ -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);
Signature · 1 line
1.9.1
PATCH 3/3 Attempt to cope with device changes and delayed kobject deallocation
In reply to Richard WattsAvoid 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.
--- fs/sysfs/dir.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
fs/sysfs/dir.c +15 -0
@@ -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 ;
Signature · 1 line
1.9.1