fix sysfs symlinks

RFC In review Maneesh Soni 8 participants 49 messages Apr 13, 2004 – May 14, 2004

Subsystems (3)

Networking Library code Driver core

lore

Share & tools

lore

Maneesh Soni

author

2004-04-13 12:36 UTC #0 lore raw

RFC 1/1 fix sysfs symlinks

Hi,

As pointed by Al Viro, the current symlinks support in sysfs is incorrect as it always sees the old target if target is renamed and obviously does not follow the new target. The page symlink operations as used by current sysfs code always see the target information at the time of creation.

Thanks
Maneesh

o The following patch implements ->readlink and ->follow_link operations for sysfs. The pointer to target kobject is saved in the link dentry's d_fsdata field. The target path is generated everytime we do ->readlink and ->follow_link.

o Apart from being correct this patch also saves some memory by not using a whole page for saving the target information.

 fs/sysfs/symlink.c |  136 +++++++++++++++++++++++++++++++++++++----------------
 fs/sysfs/sysfs.h   |   16 ++++++
 2 files changed, 112 insertions(+), 40 deletions(-)

fs/sysfs/symlink.c renamed from fs/sysfs/symlink.c~sysfs-symlinks-fix +96 ~~-40~~

diff -puN fs/sysfs/symlink.c~sysfs-symlinks-fix fs/sysfs/symlink.c
@@ -8,27 +8,17 @@
 
 #include "sysfs.h"
 
+static struct inode_operations sysfs_symlink_inode_operations = {
+	.readlink = sysfs_readlink,
+	.follow_link = sysfs_follow_link,
+};
 
 static int init_symlink(struct inode * inode)
 {
-	inode->i_op = &page_symlink_inode_operations;
+	inode->i_op = &sysfs_symlink_inode_operations;
 	return 0;
 }
 
-static int sysfs_symlink(struct inode * dir, struct dentry *dentry, const char * symname)
-{
-	int error;
-
-	error = sysfs_create(dentry, S_IFLNK|S_IRWXUGO, init_symlink);
-	if (!error) {
-		int l = strlen(symname)+1;
-		error = page_symlink(dentry->d_inode, symname, l);
-		if (error)
-			iput(dentry->d_inode);
-	}
-	return error;
-}
-
 static int object_depth(struct kobject * kobj)
 {
 	struct kobject * p = kobj;

@@ -74,37 +64,17 @@ int sysfs_create_link(struct kobject * k
 	struct dentry * dentry = kobj->dentry;
 	struct dentry * d;
 	int error = 0;
-	int size;
-	int depth;
-	char * path;
-	char * s;
-
-	depth = object_depth(kobj);
-	size = object_path_length(target) + depth * 3 - 1;
-	if (size > PATH_MAX)
-		return -ENAMETOOLONG;
-	pr_debug("%s: depth = %d, size = %d\n",__FUNCTION__,depth,size);
-
-	path = kmalloc(size,GFP_KERNEL);
-	if (!path)
-		return -ENOMEM;
-	memset(path,0,size);
-
-	for (s = path; depth--; s += 3)
-		strcpy(s,"../");
-
-	fill_object_path(target,path,size);
-	pr_debug("%s: path = '%s'\n",__FUNCTION__,path);
 
 	down(&dentry->d_inode->i_sem);
 	d = sysfs_get_dentry(dentry,name);
-	if (!IS_ERR(d))
-		error = sysfs_symlink(dentry->d_inode,d,path);
-	else
+	if (!IS_ERR(d)) {
+		error = sysfs_create(d, S_IFLNK|S_IRWXUGO, init_symlink);
+		if (!error)
+			d->d_fsdata = target;
+	} else 
 		error = PTR_ERR(d);
 	dput(d);
 	up(&dentry->d_inode->i_sem);
-	kfree(path);
 	return error;
 }

@@ -120,6 +90,92 @@ void sysfs_remove_link(struct kobject * 
 	sysfs_hash_and_remove(kobj->dentry,name);
 }
 
+static int sysfs_get_target_path(struct kobject * kobj, struct kobject * target,
+				   char **path)
+{
+	char * s;
+	int depth, size;
+	int error = 0;
+
+	depth = object_depth(kobj);
+	size = object_path_length(target) + depth * 3 - 1;
+	if (size > PATH_MAX)
+		return -ENAMETOOLONG;
+
+	pr_debug("%s: depth = %d, size = %d\n",__FUNCTION__,depth,size);
+
+	*path = kmalloc(size, GFP_KERNEL);
+	if (!*path)
+		return -ENOMEM;
+		
+	memset(*path, 0, size);
+
+	for (s = *path; depth--; s += 3)
+		strcpy(s,"../");
+
+	fill_object_path(target, *path, size);
+	pr_debug("%s: path = '%s'\n",__FUNCTION__, *path);
+
+	return error;
+}
+
+static char * sysfs_getlink(struct dentry *dentry)
+{
+	struct kobject *kobj, *target_kobj;
+	struct dentry * target_parent;
+        char *path;
+        int error = 0;
+
+	kobj = sysfs_get_kobject(dentry->d_parent);
+	if (!kobj)
+		return ERR_PTR(-EINVAL);
+
+	target_kobj = sysfs_get_kobject(dentry);
+	if (!target_kobj) {
+		kobject_put(kobj);
+		return ERR_PTR(-EINVAL);
+	}
+	target_parent = target_kobj->dentry->d_parent;
+
+	down(&target_parent->d_inode->i_sem);
+	error = sysfs_get_target_path(kobj, target_kobj, &path);
+	up(&target_parent->d_inode->i_sem);
+	
+	kobject_put(kobj);
+	kobject_put(target_kobj);
+	if (error)
+		return ERR_PTR(error);
+
+        return path;
+}
+
+int sysfs_readlink(struct dentry *dentry, char __user *buffer, int buflen)
+{
+	int res = 0;
+	char * link = sysfs_getlink(dentry);
+
+        if (!IS_ERR(link)) {
+	        res = vfs_readlink(dentry, buffer, buflen, link);
+		kfree(link);
+		return res;
+	}
+
+	return PTR_ERR(link);
+}
+
+int sysfs_follow_link(struct dentry *dentry, struct nameidata *nd)
+{
+        int res = 0;
+        char *link = sysfs_getlink(dentry);
+	
+	if (!IS_ERR(link)) {
+		res = vfs_follow_link(nd, link);
+		kfree(link);
+		return res;
+	}
+
+	return PTR_ERR(link);
+}
 
 EXPORT_SYMBOL(sysfs_create_link);
 EXPORT_SYMBOL(sysfs_remove_link);

fs/sysfs/sysfs.h renamed from fs/sysfs/sysfs.h~sysfs-symlinks-fix +16 -0

diff -puN fs/sysfs/sysfs.h~sysfs-symlinks-fix fs/sysfs/sysfs.h
@@ -11,3 +11,19 @@ extern void sysfs_hash_and_remove(struct
 
 extern int sysfs_create_subdir(struct kobject *, const char *, struct dentry **);
 extern void sysfs_remove_subdir(struct dentry *);
+
+extern int sysfs_readlink(struct dentry *dentry, char __user *buffer, int buflen);
+extern int sysfs_follow_link(struct dentry *dentry, struct nameidata *nd);
+
+static inline struct kobject * sysfs_get_kobject(struct dentry * dentry)
+{
+	struct kobject * kobj = NULL;
+
+	spin_lock(&dentry->d_lock);
+	if (!d_unhashed(dentry))
+		kobj = kobject_get(dentry->d_fsdata);
+	spin_unlock(&dentry->d_lock);
+
+	return kobj;
+}
+

Signature · 6 lines

Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

[email protected]

2004-04-13 13:36 UTC #1 lore raw

Quoted 6 lines · Maneesh Soni

Hi,
As pointed by Al Viro, the current symlinks support in sysfs is incorrect as it always sees the old target if target is renamed and obviously does not follow the new target. The page symlink operations as used by current sysfs code always see the target information at the time of creation.

a) we ought to take a reference to target when creating a symlink (and drop it on removal)

b) sysfs_get_target_path() should leave allocation to caller.

c) AFAICS we should simply allocate a page instead of messing with kmalloc().

Maneesh Soni

author

2004-04-14 06:36 UTC #2 lore raw

Quoted 10 lines · On Tue, Apr 13, 2004 at 02:36:15PM +0100…

Quoted 6 lines · Maneesh Soni
Hi,
As pointed by Al Viro, the current symlinks support in sysfs is incorrect as it always sees the old target if target is renamed and obviously does not follow the new target. The page symlink operations as used by current sysfs code always see the target information at the time of creation.
a) we ought to take a reference to target when creating a symlink (and drop it on removal)

I am not sure, if pinning the kobject for the life time of symlink (dentry) may result in same problems like rmmod hang which we saw in case of pinning kobject for the life time of its directory (dentry).

As in the following patch, we do take ref on both the parent and target kobject while _generating_ the target information in sysfs_getlink().

Quoted 4 lines

b) sysfs_get_target_path() should leave allocation to caller.
c) AFAICS we should simply allocate a page instead of messing with kmalloc().

Yes, both these have made code look much better and we still save some memory by not pinning a page for life time of the symlink. We allocate and free the page during ->readlink and ->follow_link.

Thanks
Maneesh

o Apart from being correct this patch also saves some memory by not pinning a whole page for saving the target information.

 fs/sysfs/symlink.c |  133 +++++++++++++++++++++++++++++++++++++----------------
 fs/sysfs/sysfs.h   |   16 ++++++
 2 files changed, 109 insertions(+), 40 deletions(-)

fs/sysfs/symlink.c renamed from fs/sysfs/symlink.c~sysfs-symlinks-fix +93 ~~-40~~

diff -puN fs/sysfs/symlink.c~sysfs-symlinks-fix fs/sysfs/symlink.c
@@ -8,27 +8,17 @@
 
 #include "sysfs.h"
 
+static struct inode_operations sysfs_symlink_inode_operations = {
+	.readlink = sysfs_readlink,
+	.follow_link = sysfs_follow_link,
+};
 
 static int init_symlink(struct inode * inode)
 {
-	inode->i_op = &page_symlink_inode_operations;
+	inode->i_op = &sysfs_symlink_inode_operations;
 	return 0;
 }
 
-static int sysfs_symlink(struct inode * dir, struct dentry *dentry, const char * symname)
-{
-	int error;
-
-	error = sysfs_create(dentry, S_IFLNK|S_IRWXUGO, init_symlink);
-	if (!error) {
-		int l = strlen(symname)+1;
-		error = page_symlink(dentry->d_inode, symname, l);
-		if (error)
-			iput(dentry->d_inode);
-	}
-	return error;
-}
-
 static int object_depth(struct kobject * kobj)
 {
 	struct kobject * p = kobj;

@@ -74,37 +64,17 @@ int sysfs_create_link(struct kobject * k
 	struct dentry * dentry = kobj->dentry;
 	struct dentry * d;
 	int error = 0;
-	int size;
-	int depth;
-	char * path;
-	char * s;
-
-	depth = object_depth(kobj);
-	size = object_path_length(target) + depth * 3 - 1;
-	if (size > PATH_MAX)
-		return -ENAMETOOLONG;
-	pr_debug("%s: depth = %d, size = %d\n",__FUNCTION__,depth,size);
-
-	path = kmalloc(size,GFP_KERNEL);
-	if (!path)
-		return -ENOMEM;
-	memset(path,0,size);
-
-	for (s = path; depth--; s += 3)
-		strcpy(s,"../");
-
-	fill_object_path(target,path,size);
-	pr_debug("%s: path = '%s'\n",__FUNCTION__,path);
 
 	down(&dentry->d_inode->i_sem);
 	d = sysfs_get_dentry(dentry,name);
-	if (!IS_ERR(d))
-		error = sysfs_symlink(dentry->d_inode,d,path);
-	else
+	if (!IS_ERR(d)) {
+		error = sysfs_create(d, S_IFLNK|S_IRWXUGO, init_symlink);
+		if (!error)
+			d->d_fsdata = kobject_get(target);
+	} else 
 		error = PTR_ERR(d);
 	dput(d);
 	up(&dentry->d_inode->i_sem);
-	kfree(path);
 	return error;
 }

@@ -120,6 +90,89 @@ void sysfs_remove_link(struct kobject * 
 	sysfs_hash_and_remove(kobj->dentry,name);
 }
 
+static int sysfs_get_target_path(struct kobject * kobj, struct kobject * target,
+				   char *path)
+{
+	char * s;
+	int depth, size;
+	int error = 0;
+
+	depth = object_depth(kobj);
+	size = object_path_length(target) + depth * 3 - 1;
+	if (size > PATH_MAX)
+		return -ENAMETOOLONG;
+
+	pr_debug("%s: depth = %d, size = %d\n", __FUNCTION__, depth, size);
+
+	for (s = path; depth--; s += 3)
+		strcpy(s,"../");
+
+	fill_object_path(target, path, size);
+	pr_debug("%s: path = '%s'\n", __FUNCTION__, path);
+
+	return error;
+}
+
+static int sysfs_getlink(struct dentry *dentry, char * path)
+{
+	struct kobject *kobj, *target_kobj;
+	struct dentry * target_parent;
+        int error = 0;
+
+	kobj = sysfs_get_kobject(dentry->d_parent);
+	if (!kobj)
+		return -EINVAL;
+
+	target_kobj = sysfs_get_kobject(dentry);
+	if (!target_kobj) {
+		kobject_put(kobj);
+		return -EINVAL;
+	}
+	target_parent = target_kobj->dentry->d_parent;
+
+	down(&target_parent->d_inode->i_sem);
+	error = sysfs_get_target_path(kobj, target_kobj, path);
+	up(&target_parent->d_inode->i_sem);
+	
+	kobject_put(kobj);
+	kobject_put(target_kobj);
+	return error;
+
+}
+
+int sysfs_readlink(struct dentry *dentry, char __user *buffer, int buflen)
+{
+	int error = 0;
+	unsigned long page = get_zeroed_page(GFP_KERNEL);
+
+	if (!page)
+		return -ENOMEM;
+
+	error = sysfs_getlink(dentry, (char *) page);
+        if (!error)
+	        error = vfs_readlink(dentry, buffer, buflen, (char *) page);
+
+	free_page(page);
+
+	return error;
+}
+
+int sysfs_follow_link(struct dentry *dentry, struct nameidata *nd)
+{
+        int error = 0;
+	unsigned long page = get_zeroed_page(GFP_KERNEL);
+
+	if (!page)
+		return -ENOMEM;
+
+	error = sysfs_getlink(dentry, (char *) page); 
+        if (!error)
+	        error = vfs_follow_link(nd, (char *) page);
+
+	free_page(page);
+
+	return error;
+}
 
 EXPORT_SYMBOL(sysfs_create_link);
 EXPORT_SYMBOL(sysfs_remove_link);

fs/sysfs/sysfs.h renamed from fs/sysfs/sysfs.h~sysfs-symlinks-fix +16 -0

diff -puN fs/sysfs/sysfs.h~sysfs-symlinks-fix fs/sysfs/sysfs.h
@@ -11,3 +11,19 @@ extern void sysfs_hash_and_remove(struct
 
 extern int sysfs_create_subdir(struct kobject *, const char *, struct dentry **);
 extern void sysfs_remove_subdir(struct dentry *);
+
+extern int sysfs_readlink(struct dentry *dentry, char __user *buffer, int buflen);
+extern int sysfs_follow_link(struct dentry *dentry, struct nameidata *nd);
+
+static inline struct kobject * sysfs_get_kobject(struct dentry * dentry)
+{
+	struct kobject * kobj = NULL;
+
+	spin_lock(&dentry->d_lock);
+	if (!d_unhashed(dentry))
+		kobj = kobject_get(dentry->d_fsdata);
+	spin_unlock(&dentry->d_lock);
+
+	return kobj;
+}
+

Signature · 6 lines

Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

[email protected]

2004-04-14 07:02 UTC #3 lore raw

Quoted 3 lines · Maneesh Soni

I am not sure, if pinning the kobject for the life time of symlink (dentry) may result in same problems like rmmod hang which we saw in case of pinning kobject for the life time of its directory (dentry).

Erm... If rmmod _ever_ waits for refcount on kobject to reach zero, it's already broken. Do you have any examples of such behaviour?

fs/sysfs/symlink.c void sysfs_remove_link(struct kobject * · line 113

Quoted 3 lines

fs/sysfs/symlink.c renamed from fs/sysfs/symlink.c~sysfs-symlinks-fix +3 -0
+	pr_debug("%s: path = '%s'\n", __FUNCTION__, path);
+
+	return error;

ITYM

	return 0;

BTW, replace leading spaces with tab, please - you've got a tabdamage very visible in patch.

Maneesh Soni

author

2004-04-14 07:13 UTC #4 lore raw

Quoted 7 lines · On Wed, Apr 14, 2004 at 08:02:27AM +0100…

Quoted 3 lines · Maneesh Soni
I am not sure, if pinning the kobject for the life time of symlink (dentry) may result in same problems like rmmod hang which we saw in case of pinning kobject for the life time of its directory (dentry).
Erm... If rmmod _ever_ waits for refcount on kobject to reach zero, it's already broken. Do you have any examples of such behaviour?

One such example is here in this bug report related to pcmica yenta socket.

	http://bugme.osdl.org/show_bug.cgi?id=1884

Another one is very recent in this long thread related to USB

	http://thread.gmane.org/gmane.linux.usb.devel/20468

Quoted 10 lines

Review
Quoted 4 lines
(quoted diff) +3 -0
+	pr_debug("%s: path = '%s'\n", __FUNCTION__, path);
+
+	return error;
ITYM
	return 0;
BTW, replace leading spaces with tab, please - you've got a tabdamage very visible in patch.

Corrected..

o Apart from being correct this patch also saves some memory by not pinning a whole page for saving the target information.

 fs/sysfs/symlink.c |  132 ++++++++++++++++++++++++++++++++++++-----------------
 fs/sysfs/sysfs.h   |   16 ++++++
 2 files changed, 108 insertions(+), 40 deletions(-)

fs/sysfs/symlink.c renamed from fs/sysfs/symlink.c~sysfs-symlinks-fix +92 ~~-40~~

diff -puN fs/sysfs/symlink.c~sysfs-symlinks-fix fs/sysfs/symlink.c
@@ -8,27 +8,17 @@
 
 #include "sysfs.h"
 
+static struct inode_operations sysfs_symlink_inode_operations = {
+	.readlink = sysfs_readlink,
+	.follow_link = sysfs_follow_link,
+};
 
 static int init_symlink(struct inode * inode)
 {
-	inode->i_op = &page_symlink_inode_operations;
+	inode->i_op = &sysfs_symlink_inode_operations;
 	return 0;
 }
 
-static int sysfs_symlink(struct inode * dir, struct dentry *dentry, const char * symname)
-{
-	int error;
-
-	error = sysfs_create(dentry, S_IFLNK|S_IRWXUGO, init_symlink);
-	if (!error) {
-		int l = strlen(symname)+1;
-		error = page_symlink(dentry->d_inode, symname, l);
-		if (error)
-			iput(dentry->d_inode);
-	}
-	return error;
-}
-
 static int object_depth(struct kobject * kobj)
 {
 	struct kobject * p = kobj;

@@ -74,37 +64,17 @@ int sysfs_create_link(struct kobject * k
 	struct dentry * dentry = kobj->dentry;
 	struct dentry * d;
 	int error = 0;
-	int size;
-	int depth;
-	char * path;
-	char * s;
-
-	depth = object_depth(kobj);
-	size = object_path_length(target) + depth * 3 - 1;
-	if (size > PATH_MAX)
-		return -ENAMETOOLONG;
-	pr_debug("%s: depth = %d, size = %d\n",__FUNCTION__,depth,size);
-
-	path = kmalloc(size,GFP_KERNEL);
-	if (!path)
-		return -ENOMEM;
-	memset(path,0,size);
-
-	for (s = path; depth--; s += 3)
-		strcpy(s,"../");
-
-	fill_object_path(target,path,size);
-	pr_debug("%s: path = '%s'\n",__FUNCTION__,path);
 
 	down(&dentry->d_inode->i_sem);
 	d = sysfs_get_dentry(dentry,name);
-	if (!IS_ERR(d))
-		error = sysfs_symlink(dentry->d_inode,d,path);
-	else
+	if (!IS_ERR(d)) {
+		error = sysfs_create(d, S_IFLNK|S_IRWXUGO, init_symlink);
+		if (!error)
+			d->d_fsdata = kobject_get(target);
+	} else 
 		error = PTR_ERR(d);
 	dput(d);
 	up(&dentry->d_inode->i_sem);
-	kfree(path);
 	return error;
 }

@@ -120,6 +90,88 @@ void sysfs_remove_link(struct kobject * 
 	sysfs_hash_and_remove(kobj->dentry,name);
 }
 
+static int sysfs_get_target_path(struct kobject * kobj, struct kobject * target,
+				   char *path)
+{
+	char * s;
+	int depth, size;
+
+	depth = object_depth(kobj);
+	size = object_path_length(target) + depth * 3 - 1;
+	if (size > PATH_MAX)
+		return -ENAMETOOLONG;
+
+	pr_debug("%s: depth = %d, size = %d\n", __FUNCTION__, depth, size);
+
+	for (s = path; depth--; s += 3)
+		strcpy(s,"../");
+
+	fill_object_path(target, path, size);
+	pr_debug("%s: path = '%s'\n", __FUNCTION__, path);
+
+	return 0;
+}
+
+static int sysfs_getlink(struct dentry *dentry, char * path)
+{
+	struct kobject *kobj, *target_kobj;
+	struct dentry * target_parent;
+	int error = 0;
+
+	kobj = sysfs_get_kobject(dentry->d_parent);
+	if (!kobj)
+		return -EINVAL;
+
+	target_kobj = sysfs_get_kobject(dentry);
+	if (!target_kobj) {
+		kobject_put(kobj);
+		return -EINVAL;
+	}
+	target_parent = target_kobj->dentry->d_parent;
+
+	down(&target_parent->d_inode->i_sem);
+	error = sysfs_get_target_path(kobj, target_kobj, path);
+	up(&target_parent->d_inode->i_sem);
+	
+	kobject_put(kobj);
+	kobject_put(target_kobj);
+	return error;
+
+}
+
+int sysfs_readlink(struct dentry *dentry, char __user *buffer, int buflen)
+{
+	int error = 0;
+	unsigned long page = get_zeroed_page(GFP_KERNEL);
+
+	if (!page)
+		return -ENOMEM;
+
+	error = sysfs_getlink(dentry, (char *) page);
+	if (!error)
+	        error = vfs_readlink(dentry, buffer, buflen, (char *) page);
+
+	free_page(page);
+
+	return error;
+}
+
+int sysfs_follow_link(struct dentry *dentry, struct nameidata *nd)
+{
+	int error = 0;
+	unsigned long page = get_zeroed_page(GFP_KERNEL);
+
+	if (!page)
+		return -ENOMEM;
+
+	error = sysfs_getlink(dentry, (char *) page); 
+	if (!error)
+	        error = vfs_follow_link(nd, (char *) page);
+
+	free_page(page);
+
+	return error;
+}
 
 EXPORT_SYMBOL(sysfs_create_link);
 EXPORT_SYMBOL(sysfs_remove_link);

fs/sysfs/sysfs.h renamed from fs/sysfs/sysfs.h~sysfs-symlinks-fix +16 -0

diff -puN fs/sysfs/sysfs.h~sysfs-symlinks-fix fs/sysfs/sysfs.h
@@ -11,3 +11,19 @@ extern void sysfs_hash_and_remove(struct
 
 extern int sysfs_create_subdir(struct kobject *, const char *, struct dentry **);
 extern void sysfs_remove_subdir(struct dentry *);
+
+extern int sysfs_readlink(struct dentry *dentry, char __user *buffer, int buflen);
+extern int sysfs_follow_link(struct dentry *dentry, struct nameidata *nd);
+
+static inline struct kobject * sysfs_get_kobject(struct dentry * dentry)
+{
+	struct kobject * kobj = NULL;
+
+	spin_lock(&dentry->d_lock);
+	if (!d_unhashed(dentry))
+		kobj = kobject_get(dentry->d_fsdata);
+	spin_unlock(&dentry->d_lock);
+
+	return kobj;
+}
+

Signature · 6 lines

Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

[email protected]

2004-04-14 07:27 UTC #5 lore raw

Quoted 6 lines · Maneesh Soni

Quoted 5 lines · On Wed, Apr 14, 2004 at 08:02:27AM +0100…
Quoted 3 lines · Maneesh Soni
I am not sure, if pinning the kobject for the life time of symlink (dentry) may result in same problems like rmmod hang which we saw in case of pinning kobject for the life time of its directory (dentry).

Quoted 8 lines

Quoted 2 lines
Erm... If rmmod _ever_ waits for refcount on kobject to reach zero, it's already broken. Do you have any examples of such behaviour?
One such example is here in this bug report related to pcmica yenta socket.
	http://bugme.osdl.org/show_bug.cgi?id=1884
Another one is very recent in this long thread related to USB
	http://thread.gmane.org/gmane.linux.usb.devel/20468

USB folks have no clue on sane lifetime rules, film at 11...

Russell King

2004-04-15 08:17 UTC #6 lore raw

In reply to [email protected]

Quoted 7 lines · On Wed, Apr 14, 2004 at 08:02:27AM +0100…

Quoted 3 lines · Maneesh Soni
I am not sure, if pinning the kobject for the life time of symlink (dentry) may result in same problems like rmmod hang which we saw in case of pinning kobject for the life time of its directory (dentry).
Erm... If rmmod _ever_ waits for refcount on kobject to reach zero, it's already broken. Do you have any examples of such behaviour?

Every single module which unregisters a struct device_driver.

* driver_unregister - remove driver from system.
* @drv: driver.
...

 *      This will block until the driver refcount reaches 0, and it is
 *      released. Only modular drivers will call this function, and we
 *      have to guarantee that it won't complete, letting the driver
 *      unload until all references are gone.

Signature · 4 lines

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

[email protected]

2004-04-15 10:38 UTC #7 lore raw

Quoted 4 lines · Russell King

Quoted 2 lines
Erm... If rmmod _ever_ waits for refcount on kobject to reach zero, it's already broken. Do you have any examples of such behaviour?
Every single module which unregisters a struct device_driver.

Ehh... So we have a pile of deadlocks (root-only, but still...) and a lovely user-exploitable DoS. Consider the following:

open an AF_UNIX socket pair.
go through sysfs directories of all drivers, opening all of them put obtained descriptors into SCM_RIGHTS packet and send it close all these descriptors
sleep

Voila - later rmmod attempts will hang (not just say "busy") and no, fuser won't catch your process. And IIRC, serialization in module.c will lead to nasty consequences for any subsequent attempts of module insertion.

Do we really need to embed those structures? E.g. pci_driver (the main source of those guys, AFAICS) could very well make ->driver dynamically allocated at pci_register_driver() and have it freed by its ->release(). With no waiting of any kind. The only places that would require changes would be drivers/pci/pci-driver.c and definition of to_pci_driver() - nobody else ever touches ->driver.

OTOH, eisa looks worse and the rest of them could be even uglier ;-/ Sigh...

Russell King

2004-04-15 15:19 UTC #8 lore raw

Quoted 2 lines · On Thu, Apr 15, 2004 at 11:38:49AM +0100…

OTOH, eisa looks worse and the rest of them could be even uglier ;-/ Sigh...

This also provides enough of a reason to finally go in and fix the platform_device/driver code to be more reasonable - currently its left up to platform device drivers to do all the conversion from struct device to struct platform_device.

Not only that, but they also subscribe to the "PM v1" model (using struct device_driver suspend/resume methods) whereas sysfs was updated to "PM v2" a while ago (using the bus_type suspend/resume).

Thankfully, it's only ARM and PCMCIA which make use of platform devices today, so it wouldn't be that difficult to go around fixing them up.

So take that as another reason to fix struct device_driver. 8)

However, should I also mention about the possibility of the following being in the same category; they are also typically statically allocated...

struct bus_type
struct class
struct platform_device

I think these may be worse than struct device_driver because I don't see their unregister functions even doing any form of "wait until unused" - so rather than being deadlock prone, they're oops-prone.

Sigh, sometimes life is <insert your favourite word to describe this>. ;(

Signature · 4 lines

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Greg KH

2004-04-15 16:10 UTC #9 lore raw

Quoted 9 lines · Russell King

However, should I also mention about the possibility of the following being in the same category; they are also typically statically allocated...
struct bus_type
struct class
struct platform_device

Quoted 5 lines

I think these may be worse than struct device_driver because I don't see their unregister functions even doing any form of "wait until unused" - so rather than being deadlock prone, they're oops-prone.
Sigh, sometimes life is <insert your favourite word to describe this>. ;(

Yeah, I agree. For 2.7, I want to make static allocation of anything that contains a kobject or kref not allowed to help fix things like this.

So once again we are back at the "module unload is hard" problem :)

thanks,

greg k-h

[email protected]

2004-04-15 16:13 UTC #10 lore raw

Quoted 5 lines · Greg KH

Yeah, I agree. For 2.7, I want to make static allocation of anything that contains a kobject or kref not allowed to help fix things like this.
So once again we are back at the "module unload is hard" problem :)

ITYM "for once we have a kind of objects that does disappear only on module unload, so yes, this time it's really module unload that is hard".

[email protected]

2004-04-15 19:14 UTC #11 lore raw

Quoted 9 lines · On Thu, Apr 15, 2004 at 05:13:32PM +0100…

Quoted 5 lines · Greg KH
Yeah, I agree. For 2.7, I want to make static allocation of anything that contains a kobject or kref not allowed to help fix things like this.
So once again we are back at the "module unload is hard" problem :)
ITYM "for once we have a kind of objects that does disappear only on module unload, so yes, this time it's really module unload that is hard".

BTW, how about a new section that would
a) be allocated separately at module load time
b) contain a kobject with ->release() freeing that section
c) be populated with structures containing kobjects and having
no ->release(); main kobject would be pinned down by them. Original refcount in each of those guys would be 1.

module_exit() would unregister all stuff we have in there and then drop the references to them. No waiting for anything and when all references to these objects are gone, we get the section freed. That can happen way after the completion of rmmod - as the matter of fact we could have the same module loaded again by that time.

AFAICS, that would solve the problem with static objects. Comments?

Greg KH

2004-04-15 21:28 UTC #12 lore raw

Quoted 8 lines · On Thu, Apr 15, 2004 at 08:14:47PM +0100…

BTW, how about a new section that would
a) be allocated separately at module load time
b) contain a kobject with ->release() freeing that section
c) be populated with structures containing kobjects and having
no ->release(); main kobject would be pinned down by them. Original refcount in each of those guys would be 1.

Quoted 7 lines

module_exit() would unregister all stuff we have in there and then drop the references to them. No waiting for anything and when all references to these objects are gone, we get the section freed. That can happen way after the completion of rmmod - as the matter of fact we could have the same module loaded again by that time.
AFAICS, that would solve the problem with static objects. Comments?

Yes, that would be very nice to have. It would also have to work pretty much the same way with the code built into the kernel (with the exception that module_exit() would never get called.

Sounds like some fun linker magic is called for here...

thanks,

greg k-h

Rusty Russell

2004-04-17 06:15 UTC #13 lore raw

In reply to [email protected]

Quoted 7 lines · On Fri, 2004-04-16 at 05:14, [email protected] wrote:

BTW, how about a new section that would
a) be allocated separately at module load time
b) contain a kobject with ->release() freeing that section
c) be populated with structures containing kobjects and having
no ->release(); main kobject would be pinned down by them. Original refcount in each of those guys would be 1.

Quoted 5 lines

module_exit() would unregister all stuff we have in there and then drop the references to them. No waiting for anything and when all references to these objects are gone, we get the section freed. That can happen way after the completion of rmmod - as the matter of fact we could have the same module loaded again by that time.

Or you could skip the extra section, and keep all the module memory until later. Instead of a section marker, you then set the release of those static things to "static_release" which does the put on the module memory kref:

void static_release(struct kobject *kobj)

{
	struct module *mod;

	down(&module_mutex);
	list_for_each_entry(mod, &modules, list) {
		BUG_ON(within(kobj, mod->module_init, mod->init_size);
		if (within(kobj, mod->module_core, mod->core_size)) {
			kref_put(&mod->mem_kref);
			up(&module_mutex);
			return;
		}
	}
	up(&module_mutex);
	BUG();
}

One question which comes to mind, who does the original kref_get()s on the module memory? Even if we did have a separate section, the module loader can't know how many objects objects are in there...

Cheers,
Rusty.

Signature · 1 line

Anyone who quotes me in their signature is an idiot -- Rusty Russell

[email protected]

2004-04-17 19:39 UTC #14 lore raw

Quoted 8 lines · Rusty Russell

Quoted 3 lines
to these objects are gone, we get the section freed. That can happen way after the completion of rmmod - as the matter of fact we could have the same module loaded again by that time.
Or you could skip the extra section, and keep all the module memory until later. Instead of a section marker, you then set the release of those static things to "static_release" which does the put on the module memory kref:

That will keep too much allocated after rmmod - it's OK to have one or two fixed-sized structures pinned down for a while, but entire .data can be too large to treat it that way.

Rusty Russell

2004-04-17 23:45 UTC #15 lore raw

Quoted 5 lines · On Sun, 2004-04-18 at 05:39, [email protected] wrote:

Quoted 4 lines · Rusty Russell
Quoted 3 lines
to these objects are gone, we get the section freed. That can happen way after the completion of rmmod - as the matter of fact we could have the same module loaded again by that time.

Quoted 8 lines

Quoted 4 lines
Or you could skip the extra section, and keep all the module memory until later. Instead of a section marker, you then set the release of those static things to "static_release" which does the put on the module memory kref:
That will keep too much allocated after rmmod - it's OK to have one or two fixed-sized structures pinned down for a while, but entire .data can be too large to treat it that way.

I disagree. Removal is rare, modules are usually small, it usually won't be pinned down for long, and the implementation is simple.

But that's an implementation detail. You didn't answer my question, on how you initialize the reference count on this memory.

Cheers,
Rusty.

Signature · 1 line

Anyone who quotes me in their signature is an idiot -- Rusty Russell

Greg KH

2004-04-15 22:19 UTC #16 lore raw

In reply to [email protected]

Quoted 10 lines · On Tue, Apr 13, 2004 at 02:36:15PM +0100…

Quoted 6 lines · Maneesh Soni
Hi,
As pointed by Al Viro, the current symlinks support in sysfs is incorrect as it always sees the old target if target is renamed and obviously does not follow the new target. The page symlink operations as used by current sysfs code always see the target information at the time of creation.
a) we ought to take a reference to target when creating a symlink (and drop it on removal)

No, we don't want that. It's ok to have a dangling symlink in the fs if the device the link was pointing to is now gone. All of the struct class_device stuff relies on the fact that a struct device can go away at any time, and nothing bad will happen (with the exception of a stale symlink.)

Yeah, it can cause a few odd looking trees when you unplug and replug a device a bunch of times, all the while grabbing a reference to the class device, but once everything is released by the user, it is cleaned up properly, with no harm done to anything.

thanks,

greg k-h

[email protected]

2004-04-16 15:28 UTC #17 lore raw

Quoted 10 lines · Greg KH

No, we don't want that. It's ok to have a dangling symlink in the fs if the device the link was pointing to is now gone. All of the struct class_device stuff relies on the fact that a struct device can go away at any time, and nothing bad will happen (with the exception of a stale symlink.)
Yeah, it can cause a few odd looking trees when you unplug and replug a device a bunch of times, all the while grabbing a reference to the class device, but once everything is released by the user, it is cleaned up properly, with no harm done to anything.

Except that these "symlinks" are expected to follow the target upon renames. Which means that we either need a very messy scanning of the entire tree on every rename (obviously not feasible) or we need to store pointer to target and regenerate the path. Which, in turn, requires holding a reference.

Horst von Brand

2004-04-16 18:04 UTC #18 lore raw

Quoted 12 lines · [email protected] said:

Quoted 10 lines · Greg KH
No, we don't want that. It's ok to have a dangling symlink in the fs if the device the link was pointing to is now gone. All of the struct class_device stuff relies on the fact that a struct device can go away at any time, and nothing bad will happen (with the exception of a stale symlink.)
Yeah, it can cause a few odd looking trees when you unplug and replug a device a bunch of times, all the while grabbing a reference to the class device, but once everything is released by the user, it is cleaned up properly, with no harm done to anything.

Quoted 5 lines

Except that these "symlinks" are expected to follow the target upon renames. Which means that we either need a very messy scanning of the entire tree on every rename (obviously not feasible) or we need to store pointer to target and regenerate the path. Which, in turn, requires holding a reference.

Sounds an awful lot like ordinary hard links...

Signature · 4 lines

Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

[email protected]

2004-04-16 18:11 UTC #19 lore raw

Quoted 7 lines · Horst von Brand

Quoted 5 lines
Except that these "symlinks" are expected to follow the target upon renames. Which means that we either need a very messy scanning of the entire tree on every rename (obviously not feasible) or we need to store pointer to target and regenerate the path. Which, in turn, requires holding a reference.
Sounds an awful lot like ordinary hard links...

a) to directories?
b) userland really wants (relative) pathname here
c) still would require pinning the target down, so that doesn't simplify anything.

Greg KH

2004-04-16 23:43 UTC #20 lore raw

In reply to [email protected]

Quoted 7 lines · On Fri, Apr 16, 2004 at 04:24:48PM +0100…

Quoted 6 lines · Greg KH
No, we don't want that. It's ok to have a dangling symlink in the fs if the device the link was pointing to is now gone. All of the struct class_device stuff relies on the fact that a struct device can go away at any time, and nothing bad will happen (with the exception of a stale symlink.)

Quoted 7 lines

Quoted 4 lines
Yeah, it can cause a few odd looking trees when you unplug and replug a device a bunch of times, all the while grabbing a reference to the class device, but once everything is released by the user, it is cleaned up properly, with no harm done to anything.
Except that these "symlinks" are expected to follow the target upon renames.

Since when did we ever assume that renaming a kobject would rename the symlinks that might point to it? Renaming kobjects are a hack that way, if you use them, you need to be aware of this limitation.

So I really do not see the need for this change at all.

thanks,

greg k-h

[email protected]

2004-04-16 23:47 UTC #21 lore raw

Quoted 3 lines · Greg KH

Since when did we ever assume that renaming a kobject would rename the symlinks that might point to it? Renaming kobjects are a hack that way, if you use them, you need to be aware of this limitation.

Since we assume that these symlinks actually reflect some relationship between the objects and are really needed for something. If they are not - why the hell do we keep them at all?

Jeff Garzik

2004-04-17 00:04 UTC #22 lore raw

Quoted 11 lines · [email protected] wrote:

Quoted 3 lines · Greg KH
Since when did we ever assume that renaming a kobject would rename the symlinks that might point to it? Renaming kobjects are a hack that way, if you use them, you need to be aware of this limitation.
Since we assume that these symlinks actually reflect some relationship between the objects and are really needed for something. If they are not - why the hell do we keep them at all?

I was wondering the same thing :)

Ideally one would think that userland can deduce relationships by looking at the attribute information sysfs already provides -- and if not, it's just one more bit of info to export via sysfs.

Jeff

Russell King

2004-04-17 08:07 UTC #23 lore raw

Quoted 3 lines · Jeff Garzik

Ideally one would think that userland can deduce relationships by looking at the attribute information sysfs already provides -- and if not, it's just one more bit of info to export via sysfs.

They can? So, does userspace need to know the PCI IDs associated with each driver so it can match the devices? Without the symlinks in /sys/bus/foo/devices, how do we know which devices are PCI devices and which aren't? etc...

Sure you can say "well, this device seems to have a this that and the other attribute, which appears to match what we think a PCI device should have" but then you're assuming that group of attributes only appears for PCI devices.

What about other bus types? Do I really need to teach userspace about the relationships between all the various bus types we have on ARM and how to work out what these relationships are by guessing?

Please. The symlinks are necessary and they are the sole source of the relationship information.

Signature · 4 lines

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

[email protected]

2004-04-17 08:22 UTC #24 lore raw

Quoted 6 lines · Russell King

What about other bus types? Do I really need to teach userspace about the relationships between all the various bus types we have on ARM and how to work out what these relationships are by guessing?
Please. The symlinks are necessary and they are the sole source of the relationship information.

In which case you want them to be associated with target, not the current pathname of target. And no, I don't buy the "so far all renames happen *here* and all symlinks are pointing *there*, so we don't care" - that won't last.

When do we have a legitimate reason for dangling symlinks in sysfs, anyway?

Greg KH

2004-04-20 16:23 UTC #25 lore raw

Quoted 5 lines · On Sat, Apr 17, 2004 at 09:22:06AM +0100…

Quoted 4 lines · Russell King
What about other bus types? Do I really need to teach userspace about the relationships between all the various bus types we have on ARM and how to work out what these relationships are by guessing?

Quoted 8 lines

Quoted 2 lines
Please. The symlinks are necessary and they are the sole source of the relationship information.
In which case you want them to be associated with target, not the current pathname of target. And no, I don't buy the "so far all renames happen *here* and all symlinks are pointing *there*, so we don't care" - that won't last.
When do we have a legitimate reason for dangling symlinks in sysfs, anyway?

Ok, in thinking about it some more, we don't. And I don't have a problem with grabbing the reference to the target anymore either (after looking over the code). So no more objections from me about this :)

thanks,

greg k-h

Maneesh Soni

author

2004-04-21 10:09 UTC #26 lore raw

Quoted 13 lines · Greg KH

Quoted 12 lines · On Sat, Apr 17, 2004 at 09:22:06AM +0100…
Quoted 6 lines · Russell King
What about other bus types? Do I really need to teach userspace about the relationships between all the various bus types we have on ARM and how to work out what these relationships are by guessing?
Please. The symlinks are necessary and they are the sole source of the relationship information.
In which case you want them to be associated with target, not the current pathname of target. And no, I don't buy the "so far all renames happen *here* and all symlinks are pointing *there*, so we don't care" - that won't last.

Quoted 6 lines

Quoted 1 line
When do we have a legitimate reason for dangling symlinks in sysfs, anyway?
Ok, in thinking about it some more, we don't. And I don't have a problem with grabbing the reference to the target anymore either (after looking over the code). So no more objections from me about this :)

Please see the patch below against 2.6.6-rc2-mm1. In the last version I missed releasing the target kobject whenever link is deleted. I have corrected this now. Sorry for the missing code and please review again.

Regards
Maneesh

o The symlinks code in sysfs doesnot point to the correct target kobject whenever target kobject is renamed and suffers from dangling symlinks if target kobject is removed.

o The following patch implements ->readlink and ->follow_link operations for sysfs instead of using the page_symlink_inode_operations. The pointer to target kobject is saved in the link dentry's d_fsdata field. The target path is generated everytime we do ->readlink and ->follow_link. This results in generating the correct target path during readlink and follow_link operations inspite of renamed target kobject.

o This also pins the target kobject during link creation and the ref. is released when the link is removed.

o Apart from being correct this patch also saves some memory by not pinning a whole page for saving the target information.

 fs/sysfs/dir.c     |    6 ++
 fs/sysfs/inode.c   |    5 +
 fs/sysfs/symlink.c |  135 +++++++++++++++++++++++++++++++++++++----------------
 fs/sysfs/sysfs.h   |    2 
 4 files changed, 108 insertions(+), 40 deletions(-)

fs/sysfs/sysfs.h renamed from fs/sysfs/sysfs.h~sysfs-symlinks-fix +2 -0

diff -puN fs/sysfs/sysfs.h~sysfs-symlinks-fix fs/sysfs/sysfs.h
@@ -12,6 +12,8 @@ extern void sysfs_hash_and_remove(struct
 extern int sysfs_create_subdir(struct kobject *, const char *, struct dentry **);
 extern void sysfs_remove_subdir(struct dentry *);
 
+extern int sysfs_readlink(struct dentry *, char __user *, int );
+extern int sysfs_follow_link(struct dentry *, struct nameidata *);
 
 static inline struct kobject *sysfs_get_kobject(struct dentry *dentry)
 {

fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs-symlinks-fix +6 -0

diff -puN fs/sysfs/dir.c~sysfs-symlinks-fix fs/sysfs/dir.c
@@ -135,6 +135,12 @@ restart:
 			 * Unlink and unhash.
 			 */
 			spin_unlock(&dcache_lock);
+			/* release the target kobject in case of 
+			 * a symlink
+			 */
+			if (S_ISLNK(d->d_inode->i_mode))
+				kobject_put(d->d_fsdata);
+			
 			d_delete(d);
 			simple_unlink(dentry->d_inode,d);
 			dput(d);

fs/sysfs/inode.c renamed from fs/sysfs/inode.c~sysfs-symlinks-fix +5 -0

diff -puN fs/sysfs/inode.c~sysfs-symlinks-fix fs/sysfs/inode.c
@@ -96,6 +96,11 @@ void sysfs_hash_and_remove(struct dentry
 			pr_debug("sysfs: Removing %s (%d)\n", victim->d_name.name,
 				 atomic_read(&victim->d_count));
 
+			/* release the target kobject in case of 
+			 * a symlink
+			 */
+			if (S_ISLNK(victim->d_inode->i_mode))
+				kobject_put(victim->d_fsdata);
 			d_delete(victim);
 			simple_unlink(dir->d_inode,victim);
 		}

fs/sysfs/symlink.c renamed from fs/sysfs/symlink.c~sysfs-symlinks-fix +95 ~~-40~~

165 lines not on this page · raw message

Signature · 6 lines

Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

[email protected]

2004-04-22 21:38 UTC #27 lore raw

fs/sysfs/dir.c restart: · line 146

Quoted 9 lines · Maneesh Soni

fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs-symlinks-fix +6 -0

+			/* release the target kobject in case of 
+			 * a symlink
+			 */
+			if (S_ISLNK(d->d_inode->i_mode))
+				kobject_put(d->d_fsdata);
+			
 			d_delete(d);
 			simple_unlink(dentry->d_inode,d);
 			dput(d);

I would unhash before doing kobject_put() here. Otherwise you are risking ->follow_link() or ->readlink() coming between kobject_put() and unhashing, which will screw you when sysfs_get_kobject() tries to grab a reference to (already freed) ->d_fsdata.

fs/sysfs/inode.c void sysfs_hash_and_remove(struct dentry · line 104

Quoted 6 lines

fs/sysfs/inode.c renamed from fs/sysfs/inode.c~sysfs-symlinks-fix +5 -0

+			/* release the target kobject in case of 
+			 * a symlink
+			 */
+			if (S_ISLNK(victim->d_inode->i_mode))
+				kobject_put(victim->d_fsdata);
 			d_delete(victim);

Ditto.

fs/sysfs/symlink.c int sysfs_create_link(struct kobject * k · line 79

Quoted 10 lines

fs/sysfs/symlink.c renamed from fs/sysfs/symlink.c~sysfs-symlinks-fix +8 -0

+	if (!IS_ERR(d)) {
+		error = sysfs_create(d, S_IFLNK|S_IRWXUGO, init_symlink);
+		if (!error)
+			/* 
+			 * associate the link dentry with the target kobject 
+			 */
+			d->d_fsdata = kobject_get(target);
+	} else 
 		error = PTR_ERR(d);
 	dput(d);

Huh? Not to mention anything else, dput(d) is guaranteed to screw you if IS_ERR(d) is true.

fs/sysfs/symlink.c void sysfs_remove_link(struct kobject * · line 137

Quoted 3 lines

fs/sysfs/symlink.c renamed from fs/sysfs/symlink.c~sysfs-symlinks-fix +3 -0
+	down(&target_parent->d_inode->i_sem);
+	error = sysfs_get_target_path(kobj, target_kobj, path);
+	up(&target_parent->d_inode->i_sem);

You need to be careful in sysfs_get_target_path() - this ->i_sem doesn't prevent renames of ancestors. rwsem held exclusive by renaming and shared by sysfs_get_target_path(), maybe? FWIW, even rwlock might be sufficient...

Maneesh Soni

author

2004-04-23 08:49 UTC #28 lore raw

On Thu, Apr 22, 2004 at 10:37:37PM +0100, [email protected] wrote:
[..]

Quoted 6 lines

I would unhash before doing kobject_put() here. Otherwise you are risking ->follow_link() or ->readlink() coming between kobject_put() and unhashing, which will screw you when sysfs_get_kobject() tries to grab a reference to (already freed) ->d_fsdata.

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +5 -0

+			/* release the target kobject in case of 
+			 * a symlink
+			 */
+			if (S_ISLNK(victim->d_inode->i_mode))
+				kobject_put(victim->d_fsdata);
 			d_delete(victim);

Ditto.

Right.. I have corrected this.

[..]

Quoted 3 lines

Huh? Not to mention anything else, dput(d) is guaranteed to screw you if IS_ERR(d) is true.

great eyes.. I couldn't spot this even with four eyes 8-).. but for my relief I was not the culprit for this. I also corrected a couple of more uses of sysfs_get_dentry().

Quoted 7 lines

Review
Quoted 3 lines
(quoted diff) +3 -0
+	down(&target_parent->d_inode->i_sem);
+	error = sysfs_get_target_path(kobj, target_kobj, path);
+	up(&target_parent->d_inode->i_sem);
You need to be careful in sysfs_get_target_path() - this ->i_sem doesn't prevent renames of ancestors. rwsem held exclusive by renaming and shared by sysfs_get_target_path(), maybe? FWIW, even rwlock might be sufficient...

I used rwsem, considering the allocations done in kobject_set_name(). Updated patch appended.

Thanks
Maneesh

o The symlinks code in sysfs doesnot point to the correct target kobject whenever target kobject is renamed and suffers from dangling symlinks if target kobject is removed.

o This also pins the target kobject during link creation and the ref. is released when the link is removed.

o Apart from being correct this patch also saves some memory by not pinning a whole page for saving the target information.

o Used a rw_semaphor sysfs_rename_sem to avoid clashing with renaming of ancestors while the target path is generated.

o Also corrected a couple of sysfs_get_dentry() uses.

 fs/sysfs/dir.c     |   17 +++++-
 fs/sysfs/group.c   |   12 ++--
 fs/sysfs/inode.c   |    5 +
 fs/sysfs/symlink.c |  136 +++++++++++++++++++++++++++++++++++++----------------
 fs/sysfs/sysfs.h   |    3 +
 5 files changed, 124 insertions(+), 49 deletions(-)

fs/sysfs/sysfs.h renamed from fs/sysfs/sysfs.h~sysfs-symlinks-fix +3 -0

diff -puN fs/sysfs/sysfs.h~sysfs-symlinks-fix fs/sysfs/sysfs.h
@@ -12,6 +12,9 @@ extern void sysfs_hash_and_remove(struct
 extern int sysfs_create_subdir(struct kobject *, const char *, struct dentry **);
 extern void sysfs_remove_subdir(struct dentry *);
 
+extern int sysfs_readlink(struct dentry *, char __user *, int );
+extern int sysfs_follow_link(struct dentry *, struct nameidata *);
+extern struct rw_semaphore sysfs_rename_sem;
 
 static inline struct kobject *sysfs_get_kobject(struct dentry *dentry)
 {

fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs-symlinks-fix +14 -3

diff -puN fs/sysfs/dir.c~sysfs-symlinks-fix fs/sysfs/dir.c
@@ -10,6 +10,8 @@
 #include <linux/kobject.h>
 #include "sysfs.h"
 
+DECLARE_RWSEM(sysfs_rename_sem);
+
 static int init_dir(struct inode * inode)
 {
 	inode->i_op = &simple_dir_inode_operations;

@@ -136,6 +138,12 @@ restart:
 			 */
 			spin_unlock(&dcache_lock);
 			d_delete(d);
+			/* release the target kobject in case of 
+			 * a symlink
+			 */
+			if (S_ISLNK(d->d_inode->i_mode))
+				kobject_put(d->d_fsdata);
+			
 			simple_unlink(dentry->d_inode,d);
 			dput(d);
 			pr_debug(" done\n");

@@ -167,10 +175,13 @@ void sysfs_rename_dir(struct kobject * k
 	parent = kobj->parent->dentry;
 
 	down(&parent->d_inode->i_sem);
-
 	new_dentry = sysfs_get_dentry(parent, new_name);
-	d_move(kobj->dentry, new_dentry);
-	kobject_set_name(kobj,new_name);
+	if (!IS_ERR(new_dentry)) {
+		down_write(&sysfs_rename_sem);
+		d_move(kobj->dentry, new_dentry);
+		kobject_set_name(kobj,new_name);
+		up_write(&sysfs_rename_sem);
+	}
 	up(&parent->d_inode->i_sem);	
 }

fs/sysfs/inode.c renamed from fs/sysfs/inode.c~sysfs-symlinks-fix +5 -0

diff -puN fs/sysfs/inode.c~sysfs-symlinks-fix fs/sysfs/inode.c
@@ -97,6 +97,11 @@ void sysfs_hash_and_remove(struct dentry
 				 atomic_read(&victim->d_count));
 
 			d_delete(victim);
+			/* release the target kobject in case of 
+			 * a symlink
+			 */
+			if (S_ISLNK(victim->d_inode->i_mode))
+				kobject_put(victim->d_fsdata);
 			simple_unlink(dir->d_inode,victim);
 		}
 		/*

fs/sysfs/symlink.c renamed from fs/sysfs/symlink.c~sysfs-symlinks-fix +95 ~~-41~~

165 lines not on this page · raw message

fs/sysfs/group.c renamed from fs/sysfs/group.c~sysfs-symlinks-fix +7 -5

diff -puN fs/sysfs/group.c~sysfs-symlinks-fix fs/sysfs/group.c
@@ -70,11 +70,13 @@ void sysfs_remove_group(struct kobject *
 	else
 		dir = dget(kobj->dentry);
 
-	remove_files(dir,grp);
-	if (grp->name)
-		sysfs_remove_subdir(dir);
-	/* release the ref. taken in this routine */
-	dput(dir);
+	if (dir && !IS_ERR(dir)) {
+		remove_files(dir,grp);
+		if (grp->name)
+			sysfs_remove_subdir(dir);
+		/* release the ref. taken in this routine */
+		dput(dir);
+	}
 }

Signature · 6 lines

Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

[email protected]

2004-04-23 09:26 UTC #29 lore raw

fs/sysfs/dir.c restart: · line 140

Quoted 4 lines · Maneesh Soni

fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs-symlinks-fix +0 -0
@@ -136,6 +138,12 @@ restart:
 			 */
 			spin_unlock(&dcache_lock);
 			d_delete(d);

ITYM "d_drop(d)" here. Right now these are equivalent (and d_drop() is less work) since we are holding at least two references to d; if/when we stop pinning leaves of the tree in core, the check below will become unsafe with d_delete().

fs/sysfs/dir.c void sysfs_rename_dir(struct kobject * k · line 186

Quoted 11 lines

fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs-symlinks-fix +6 -2

 	new_dentry = sysfs_get_dentry(parent, new_name);
-	d_move(kobj->dentry, new_dentry);
-	kobject_set_name(kobj,new_name);
+	if (!IS_ERR(new_dentry)) {
+		down_write(&sysfs_rename_sem);
+		d_move(kobj->dentry, new_dentry);
+		kobject_set_name(kobj,new_name);
+		up_write(&sysfs_rename_sem);
+	}
 	up(&parent->d_inode->i_sem);	
 }

BTW, the above leaks because of unbalanced sysfs_get_dentry(). You need a dput() in there.

fs/sysfs/inode.c void sysfs_hash_and_remove(struct dentry · line 104

Quoted 12 lines

diff -puN fs/sysfs/inode.c~sysfs-symlinks-fix fs/sysfs/inode.c

fs/sysfs/inode.c renamed from fs/sysfs/inode.c~sysfs-symlinks-fix +5 -0

@@ -97,6 +97,11 @@ void sysfs_hash_and_remove(struct dentry
 				 atomic_read(&victim->d_count));
 
 			d_delete(victim);
+			/* release the target kobject in case of 
+			 * a symlink
+			 */
+			if (S_ISLNK(victim->d_inode->i_mode))
+				kobject_put(victim->d_fsdata);

Same s/d_delete/d_drop/ issue.

Quoted 10 lines

fs/sysfs/inode.c renamed from fs/sysfs/inode.c~sysfs-symlinks-fix +0 -3

 			simple_unlink(dir->d_inode,victim);
 		}

@@ -70,11 +70,13 @@ void sysfs_remove_group(struct kobject *
 	else
 		dir = dget(kobj->dentry);
 
-	remove_files(dir,grp);
-	if (grp->name)
-		sysfs_remove_subdir(dir);

fs/sysfs/inode.c void sysfs_remove_group(struct kobject * · line 80

Quoted 10 lines

fs/sysfs/inode.c renamed from fs/sysfs/inode.c~sysfs-symlinks-fix +7 -2

-	/* release the ref. taken in this routine */
-	dput(dir);
+	if (dir && !IS_ERR(dir)) {
+		remove_files(dir,grp);
+		if (grp->name)
+			sysfs_remove_subdir(dir);
+		/* release the ref. taken in this routine */
+		dput(dir);
+	}
 }

Hmm... I thought that's what

		if (error)
			return error;

several lines above had been about. Can we get there with NULL or ERR_PTR() in dir?

Maneesh Soni

author

2004-04-29 13:03 UTC #30 lore raw

Quoted 10 lines · On Fri, Apr 23, 2004 at 10:26:41AM +0100…

Review restart: · line 140
Quoted 4 lines · Maneesh Soni
(quoted diff) +0 -0
@@ -136,6 +138,12 @@ restart:
 			 */
 			spin_unlock(&dcache_lock);
 			d_delete(d);
ITYM "d_drop(d)" here. Right now these are equivalent (and d_drop() is less work) since we are holding at least two references to d; if/when we stop pinning leaves of the tree in core, the check below will become unsafe with d_delete().

You are right again, I do use d_drop() for sysfs backing store. Here also we can use d_drop(). I have corrected at both the places you pointed.

Quoted 15 lines

Review restart:

Quoted 12 lines

(quoted diff) +6 -2

 	new_dentry = sysfs_get_dentry(parent, new_name);
-	d_move(kobj->dentry, new_dentry);
-	kobject_set_name(kobj,new_name);
+	if (!IS_ERR(new_dentry)) {
+		down_write(&sysfs_rename_sem);
+		d_move(kobj->dentry, new_dentry);
+		kobject_set_name(kobj,new_name);
+		up_write(&sysfs_rename_sem);
+	}
 	up(&parent->d_inode->i_sem);	
 }

BTW, the above leaks because of unbalanced sysfs_get_dentry(). You need a dput() in there.

If I am not wrong I should add dput(new_dentry) after d_move() to avoid leak.

[..]

Quoted 20 lines

Quoted 8 lines

(quoted diff) +0 -3

@@ -70,11 +70,13 @@ void sysfs_remove_group(struct kobject *
 	else
 		dir = dget(kobj->dentry);
 
-	remove_files(dir,grp);
-	if (grp->name)
-		sysfs_remove_subdir(dir);

Review void sysfs_remove_group(struct kobject * · line 80

Quoted 10 lines

(quoted diff) +7 -2

-	/* release the ref. taken in this routine */
-	dput(dir);
+	if (dir && !IS_ERR(dir)) {
+		remove_files(dir,grp);
+		if (grp->name)
+			sysfs_remove_subdir(dir);
+		/* release the ref. taken in this routine */
+		dput(dir);
+	}
 }

Hmm... I thought that's what

Quoted 4 lines

		if (error)
			return error;
several lines above had been about. Can we get there with NULL or ERR_PTR() in dir?

No.. if sysfs_create_group() is successful, we will never get error in dir. And no one will be allowed to call sysfs_remove_group() if sysfs_create_group() has failed.

Patch appended with corrections.

Thanks
Maneesh

o The symlinks code in sysfs doesnot point to the correct target kobject whenever target kobject is renamed and suffers from dangling symlinks if target kobject is removed.

o This also pins the target kobject during link creation and the ref. is released when the link is removed.

o Apart from being correct this patch also saves some memory by not pinning a whole page for saving the target information.

o Used a rw_semaphor sysfs_rename_sem to avoid clashing with renaming of ancestors while the target path is generated.

o Used dcache_lock in fs/sysfs/sysfs.h:sysfs_get_kobject() because of using d_drop() while removing dentries.

 fs/sysfs/dir.c     |   20 ++++++-
 fs/sysfs/inode.c   |    7 ++
 fs/sysfs/symlink.c |  135 ++++++++++++++++++++++++++++++++++++-----------------
 fs/sysfs/sysfs.h   |    7 +-
 4 files changed, 121 insertions(+), 48 deletions(-)

fs/sysfs/sysfs.h renamed from fs/sysfs/sysfs.h~sysfs-symlinks-fix +5 -2

diff -puN fs/sysfs/sysfs.h~sysfs-symlinks-fix fs/sysfs/sysfs.h
@@ -12,15 +12,18 @@ extern void sysfs_hash_and_remove(struct
 extern int sysfs_create_subdir(struct kobject *, const char *, struct dentry **);
 extern void sysfs_remove_subdir(struct dentry *);
 
+extern int sysfs_readlink(struct dentry *, char __user *, int );
+extern int sysfs_follow_link(struct dentry *, struct nameidata *);
+extern struct rw_semaphore sysfs_rename_sem;
 
 static inline struct kobject *sysfs_get_kobject(struct dentry *dentry)
 {
 	struct kobject * kobj = NULL;
 
-	spin_lock(&dentry->d_lock);
+	spin_lock(&dcache_lock);
 	if (!d_unhashed(dentry))
 		kobj = kobject_get(dentry->d_fsdata);
-	spin_unlock(&dentry->d_lock);
+	spin_unlock(&dcache_lock);
 
 	return kobj;
 }

fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs-symlinks-fix +16 -4

diff -puN fs/sysfs/dir.c~sysfs-symlinks-fix fs/sysfs/dir.c
@@ -10,6 +10,8 @@
 #include <linux/kobject.h>
 #include "sysfs.h"
 
+DECLARE_RWSEM(sysfs_rename_sem);
+
 static int init_dir(struct inode * inode)
 {
 	inode->i_op = &simple_dir_inode_operations;

@@ -134,8 +136,14 @@ restart:
 			/**
 			 * Unlink and unhash.
 			 */
+			__d_drop(d);
 			spin_unlock(&dcache_lock);
-			d_delete(d);
+			/* release the target kobject in case of 
+			 * a symlink
+			 */
+			if (S_ISLNK(d->d_inode->i_mode))
+				kobject_put(d->d_fsdata);
+			
 			simple_unlink(dentry->d_inode,d);
 			dput(d);
 			pr_debug(" done\n");

@@ -167,10 +175,14 @@ void sysfs_rename_dir(struct kobject * k
 	parent = kobj->parent->dentry;
 
 	down(&parent->d_inode->i_sem);
-
 	new_dentry = sysfs_get_dentry(parent, new_name);
-	d_move(kobj->dentry, new_dentry);
-	kobject_set_name(kobj,new_name);
+	if (!IS_ERR(new_dentry)) {
+		down_write(&sysfs_rename_sem);
+		d_move(kobj->dentry, new_dentry);
+		kobject_set_name(kobj,new_name);
+		up_write(&sysfs_rename_sem);
+		dput(new_dentry);
+	}
 	up(&parent->d_inode->i_sem);	
 }

fs/sysfs/inode.c renamed from fs/sysfs/inode.c~sysfs-symlinks-fix +6 -1

diff -puN fs/sysfs/inode.c~sysfs-symlinks-fix fs/sysfs/inode.c
@@ -96,7 +96,12 @@ void sysfs_hash_and_remove(struct dentry
 			pr_debug("sysfs: Removing %s (%d)\n", victim->d_name.name,
 				 atomic_read(&victim->d_count));
 
-			d_delete(victim);
+			d_drop(victim);
+			/* release the target kobject in case of 
+			 * a symlink
+			 */
+			if (S_ISLNK(victim->d_inode->i_mode))
+				kobject_put(victim->d_fsdata);
 			simple_unlink(dir->d_inode,victim);
 		}
 		/*

fs/sysfs/symlink.c renamed from fs/sysfs/symlink.c~sysfs-symlinks-fix +94 ~~-41~~

164 lines not on this page · raw message

Signature · 6 lines

Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

[email protected]

2004-04-29 15:41 UTC #31 lore raw

On Thu, Apr 29, 2004 at 06:33:53PM +0530, Maneesh Soni wrote:

[snip]

Quoted 7 lines

fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs-symlinks-fix +0 -2

@@ -167,10 +175,14 @@ void sysfs_rename_dir(struct kobject * k
 	parent = kobj->parent->dentry;
 
 	down(&parent->d_inode->i_sem);
-
 	new_dentry = sysfs_get_dentry(parent, new_name);
-	d_move(kobj->dentry, new_dentry);

fs/sysfs/dir.c void sysfs_rename_dir(struct kobject * k · line 187

Quoted 10 lines

fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs-symlinks-fix +7 -1

-	kobject_set_name(kobj,new_name);
+	if (!IS_ERR(new_dentry)) {
+		down_write(&sysfs_rename_sem);
+		d_move(kobj->dentry, new_dentry);
+		kobject_set_name(kobj,new_name);
+		up_write(&sysfs_rename_sem);
+		dput(new_dentry);
+	}
 	up(&parent->d_inode->i_sem);	
 }

I would probably lift that rwsem all way up - in front of any other locks in sysfs_rename_dir(). Note that kobject_set_name() can very well lead to allocations, so just to make the lock hierarchy cleaner... Another thing:
please, check that new_dentry is negative here. Other than that, I've got no problems with the patch.

Maneesh Soni

author

2004-04-30 10:03 UTC #32 lore raw

Quoted 19 lines · On Thu, Apr 29, 2004 at 04:41:04PM +0100…

On Thu, Apr 29, 2004 at 06:33:53PM +0530, Maneesh Soni wrote:

[snip]

Quoted 15 lines

(quoted diff) +7 -3

@@ -167,10 +175,14 @@ void sysfs_rename_dir(struct kobject * k
 	parent = kobj->parent->dentry;
 
 	down(&parent->d_inode->i_sem);
-
 	new_dentry = sysfs_get_dentry(parent, new_name);
-	d_move(kobj->dentry, new_dentry);
-	kobject_set_name(kobj,new_name);
+	if (!IS_ERR(new_dentry)) {
+		down_write(&sysfs_rename_sem);
+		d_move(kobj->dentry, new_dentry);
+		kobject_set_name(kobj,new_name);
+		up_write(&sysfs_rename_sem);
+		dput(new_dentry);
+	}

Quoted 8 lines

Review void sysfs_rename_dir(struct kobject * k · line 187
Quoted 2 lines
(quoted diff) +0 -0
 	up(&parent->d_inode->i_sem);	
 }
I would probably lift that rwsem all way up - in front of any other locks in sysfs_rename_dir(). Note that kobject_set_name() can very well lead to allocations, so just to make the lock hierarchy cleaner... Another thing:
please, check that new_dentry is negative here. Other than that, I've got no problems with the patch.

Ok... the corrected patch is appended. But I see more work to be done here which if required can be done as a patch over this one which is not related to sysfs symlinks. Please see the next mail.

Thanks
Maneesh

o The symlinks code in sysfs doesnot point to the correct target kobject whenever target kobject is renamed and suffers from dangling symlinks if target kobject is removed.

o This also pins the target kobject during link creation and the ref. is released when the link is removed.

o Apart from being correct this patch also saves some memory by not pinning a whole page for saving the target information.

o Used a rw_semaphor sysfs_rename_sem to avoid clashing with renaming of ancestors while the target path is generated.

o Used dcache_lock in fs/sysfs/sysfs.h:sysfs_get_kobject() because of using d_drop() while removing dentries.

 fs/sysfs/dir.c     |   22 +++++++-
 fs/sysfs/inode.c   |    7 ++
 fs/sysfs/symlink.c |  135 ++++++++++++++++++++++++++++++++++++-----------------
 fs/sysfs/sysfs.h   |    7 +-
 4 files changed, 123 insertions(+), 48 deletions(-)

fs/sysfs/sysfs.h renamed from fs/sysfs/sysfs.h~sysfs-symlinks-fix +5 -2

diff -puN fs/sysfs/sysfs.h~sysfs-symlinks-fix fs/sysfs/sysfs.h
@@ -12,15 +12,18 @@ extern void sysfs_hash_and_remove(struct
 extern int sysfs_create_subdir(struct kobject *, const char *, struct dentry **);
 extern void sysfs_remove_subdir(struct dentry *);
 
+extern int sysfs_readlink(struct dentry *, char __user *, int );
+extern int sysfs_follow_link(struct dentry *, struct nameidata *);
+extern struct rw_semaphore sysfs_rename_sem;
 
 static inline struct kobject *sysfs_get_kobject(struct dentry *dentry)
 {
 	struct kobject * kobj = NULL;
 
-	spin_lock(&dentry->d_lock);
+	spin_lock(&dcache_lock);
 	if (!d_unhashed(dentry))
 		kobj = kobject_get(dentry->d_fsdata);
-	spin_unlock(&dentry->d_lock);
+	spin_unlock(&dcache_lock);
 
 	return kobj;
 }

fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs-symlinks-fix +18 -4

diff -puN fs/sysfs/dir.c~sysfs-symlinks-fix fs/sysfs/dir.c
@@ -10,6 +10,8 @@
 #include <linux/kobject.h>
 #include "sysfs.h"
 
+DECLARE_RWSEM(sysfs_rename_sem);
+
 static int init_dir(struct inode * inode)
 {
 	inode->i_op = &simple_dir_inode_operations;

@@ -134,8 +136,14 @@ restart:
 			/**
 			 * Unlink and unhash.
 			 */
+			__d_drop(d);
 			spin_unlock(&dcache_lock);
-			d_delete(d);
+			/* release the target kobject in case of 
+			 * a symlink
+			 */
+			if (S_ISLNK(d->d_inode->i_mode))
+				kobject_put(d->d_fsdata);
+			
 			simple_unlink(dentry->d_inode,d);
 			dput(d);
 			pr_debug(" done\n");

@@ -164,14 +172,20 @@ void sysfs_rename_dir(struct kobject * k
 	if (!kobj->parent)
 		return;
 
+	down_write(&sysfs_rename_sem);
 	parent = kobj->parent->dentry;
 
 	down(&parent->d_inode->i_sem);
-
 	new_dentry = sysfs_get_dentry(parent, new_name);
-	d_move(kobj->dentry, new_dentry);
-	kobject_set_name(kobj,new_name);
+	if (!IS_ERR(new_dentry)) {
+		if (!new_dentry->d_inode) {
+			d_move(kobj->dentry, new_dentry);
+			kobject_set_name(kobj,new_name);
+		}
+		dput(new_dentry);
+	}
 	up(&parent->d_inode->i_sem);	
+	up_write(&sysfs_rename_sem);
 }
 
 EXPORT_SYMBOL(sysfs_create_dir);

fs/sysfs/inode.c renamed from fs/sysfs/inode.c~sysfs-symlinks-fix +6 -1

diff -puN fs/sysfs/inode.c~sysfs-symlinks-fix fs/sysfs/inode.c
@@ -96,7 +96,12 @@ void sysfs_hash_and_remove(struct dentry
 			pr_debug("sysfs: Removing %s (%d)\n", victim->d_name.name,
 				 atomic_read(&victim->d_count));
 
-			d_delete(victim);
+			d_drop(victim);
+			/* release the target kobject in case of 
+			 * a symlink
+			 */
+			if (S_ISLNK(victim->d_inode->i_mode))
+				kobject_put(victim->d_fsdata);
 			simple_unlink(dir->d_inode,victim);
 		}
 		/*

fs/sysfs/symlink.c renamed from fs/sysfs/symlink.c~sysfs-symlinks-fix +94 ~~-41~~

164 lines not on this page · raw message

Signature · 6 lines

Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

Maneesh Soni

author

2004-04-30 10:10 UTC #33 lore raw

cover kobject_set_name - error handling

IMO error handling/propogation is incorrect or not there in most of the places in the users of kobject_set_name. sysfs_rename_dir is one of them.

1) sysfs_rename_dir ignores return code from kobject_set_name, and returns void
2) kobject_rename returns void
3) dev_change_name ignore return code from class_device_rename.

This can lead to mismatch in kobject name and the directory name. Now correcting this will involve changing the APIs at this point of time. Luckily we have just one user of sysfs_rename_dir as of now.

Like this there are 14 other users of kobject_set_name and only one checks the return code. Out of them atleast following are problematic IMO.

bus_add_driver()
bus_register()
sys_dev_register()
sysfs_rename_dir()

Others are not because we have name length less than KOBJ_NAME_LEN and there will be no allocation. I have corrected the above problems in the following two patches.

Please comment.

Thanks,
Maneesh

Signature · 6 lines

Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

Maneesh Soni

author

2004-04-30 10:11 UTC #34 lore raw

RFC 1/2 kobject_set_name - error handling

o The following patch cleans up the kobject_set_name() users. Basically checking return code from kobject_set_name(). There can be error returns like -ENOMEM or -EFAULT from kobject_set_name() if the name length exceeds KOBJ_NAME_LEN.

 drivers/base/bus.c |   12 +++++++++---
 drivers/base/sys.c |    5 ++++-
 2 files changed, 13 insertions(+), 4 deletions(-)

drivers/base/sys.c renamed from drivers/base/sys.c~kobject_set_name-cleanup-01 +4 -1

diff -puN drivers/base/sys.c~kobject_set_name-cleanup-01 drivers/base/sys.c
@@ -180,8 +180,11 @@ int sysdev_register(struct sys_device * 
 
 	/* But make sure we point to the right type for sysfs translation */
 	sysdev->kobj.ktype = &ktype_sysdev;
-	kobject_set_name(&sysdev->kobj,"%s%d",
+	error = kobject_set_name(&sysdev->kobj,"%s%d",
 			 kobject_name(&cls->kset.kobj),sysdev->id);
+	if (error)
+		return error;
+
 	pr_debug("Registering sys device '%s'\n",kobject_name(&sysdev->kobj));
 
 	/* Register the object */

drivers/base/bus.c renamed from drivers/base/bus.c~kobject_set_name-cleanup-01 +9 -3

diff -puN drivers/base/bus.c~kobject_set_name-cleanup-01 drivers/base/bus.c
@@ -451,7 +451,9 @@ int bus_add_driver(struct device_driver 
 
 	if (bus) {
 		pr_debug("bus %s: add driver %s\n",bus->name,drv->name);
-		kobject_set_name(&drv->kobj,drv->name);
+		error = kobject_set_name(&drv->kobj,drv->name);
+		if (error)
+			return error;
 		drv->kobj.kset = &bus->drivers;
 		if ((error = kobject_register(&drv->kobj))) {
 			put_bus(bus);

@@ -555,7 +557,11 @@ struct bus_type * find_bus(char * name)
  */
 int bus_register(struct bus_type * bus)
 {
-	kobject_set_name(&bus->subsys.kset.kobj,bus->name);
+	int error = 0;
+
+	error = kobject_set_name(&bus->subsys.kset.kobj,bus->name);
+	if (error)
+		return error;
 	subsys_set_kset(bus,bus_subsys);
 	subsystem_register(&bus->subsys);

@@ -569,7 +575,7 @@ int bus_register(struct bus_type * bus)
 	kset_register(&bus->drivers);
 
 	pr_debug("bus type '%s' registered\n",bus->name);
-	return 0;
+	return error;
 }

Signature · 6 lines

Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

Maneesh Soni

author

2004-04-30 10:14 UTC #35 lore raw

RFC 2/2 kobject_set_name - error handling

o The following patch cleans up sysfs_rename_dir(). It now checks the return code of kobject_set_name() and propagates the error code to its callers. Because of this there are changes in the following two APIs. Both return int instead of void.

int sysfs_rename_dir(struct kobject * kobj, const char *new_name) int kobject_rename(struct kobject * kobj, char *new_name)

o It needs the sysfs-symlinks-fix.patch to get apppied.

	http://marc.theaimsgroup.com/?l=linux-kernel&m=108331963219401&w=2

 drivers/base/class.c    |    6 ++++--
 fs/sysfs/dir.c          |   14 +++++++++-----
 include/linux/kobject.h |    2 +-
 include/linux/sysfs.h   |    2 +-
 lib/kobject.c           |   10 +++++++---
 net/core/dev.c          |   12 +++++++-----
 6 files changed, 29 insertions(+), 17 deletions(-)

fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs_rename_dir-cleanup +9 -5

diff -puN fs/sysfs/dir.c~sysfs_rename_dir-cleanup fs/sysfs/dir.c
@@ -162,15 +162,16 @@ restart:
 	dput(dentry);
 }
 
-void sysfs_rename_dir(struct kobject * kobj, const char *new_name)
+int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
 {
+	int error = 0;
 	struct dentry * new_dentry, * parent;
 
 	if (!strcmp(kobject_name(kobj), new_name))
-		return;
+		return -EINVAL;
 
 	if (!kobj->parent)
-		return;
+		return -EINVAL;
 
 	down_write(&sysfs_rename_sem);
 	parent = kobj->parent->dentry;

@@ -179,13 +180,16 @@ void sysfs_rename_dir(struct kobject * k
 	new_dentry = sysfs_get_dentry(parent, new_name);
 	if (!IS_ERR(new_dentry)) {
 		if (!new_dentry->d_inode) {
-			d_move(kobj->dentry, new_dentry);
-			kobject_set_name(kobj,new_name);
+			error = kobject_set_name(kobj,new_name);
+			if (!error)
+				d_move(kobj->dentry, new_dentry);
 		}
 		dput(new_dentry);
 	}
 	up(&parent->d_inode->i_sem);	
 	up_write(&sysfs_rename_sem);
+
+	return error;
 }
 
 EXPORT_SYMBOL(sysfs_create_dir);

include/linux/sysfs.h renamed from include/linux/sysfs.h~sysfs_rename_dir-cleanup +1 -1

diff -puN include/linux/sysfs.h~sysfs_rename_dir-cleanup include/linux/sysfs.h
@@ -44,7 +44,7 @@ sysfs_create_dir(struct kobject *);
 extern void
 sysfs_remove_dir(struct kobject *);
 
-extern void
+extern int
 sysfs_rename_dir(struct kobject *, const char *new_name);
 
 extern int

lib/kobject.c renamed from lib/kobject.c~sysfs_rename_dir-cleanup +7 -3

diff -puN lib/kobject.c~sysfs_rename_dir-cleanup lib/kobject.c
@@ -385,13 +385,17 @@ EXPORT_SYMBOL(kobject_set_name);
  *	@new_name: object's new name
  */
 
-void kobject_rename(struct kobject * kobj, char *new_name)
+int kobject_rename(struct kobject * kobj, char *new_name)
 {
+	int error = 0;
+
 	kobj = kobject_get(kobj);
 	if (!kobj)
-		return;
-	sysfs_rename_dir(kobj, new_name);
+		return -EINVAL;
+	error = sysfs_rename_dir(kobj, new_name);
 	kobject_put(kobj);
+
+	return error;
 }
 
 /**

include/linux/kobject.h renamed from include/linux/kobject.h~sysfs_rename_dir-cleanup +1 -1

diff -puN include/linux/kobject.h~sysfs_rename_dir-cleanup include/linux/kobject.h
@@ -48,7 +48,7 @@ extern void kobject_cleanup(struct kobje
 extern int kobject_add(struct kobject *);
 extern void kobject_del(struct kobject *);
 
-extern void kobject_rename(struct kobject *, char *new_name);
+extern int kobject_rename(struct kobject *, char *new_name);
 
 extern int kobject_register(struct kobject *);
 extern void kobject_unregister(struct kobject *);

drivers/base/class.c renamed from drivers/base/class.c~sysfs_rename_dir-cleanup +4 -2

diff -puN drivers/base/class.c~sysfs_rename_dir-cleanup drivers/base/class.c
@@ -361,6 +361,8 @@ void class_device_unregister(struct clas
 
 int class_device_rename(struct class_device *class_dev, char *new_name)
 {
+	int error = 0;
+
 	class_dev = class_device_get(class_dev);
 	if (!class_dev)
 		return -EINVAL;

@@ -370,11 +372,11 @@ int class_device_rename(struct class_dev
 
 	strlcpy(class_dev->class_id, new_name, KOBJ_NAME_LEN);
 
-	kobject_rename(&class_dev->kobj, new_name);
+	error = kobject_rename(&class_dev->kobj, new_name);
 
 	class_device_put(class_dev);
 
-	return 0;
+	return error;
 }
 
 struct class_device * class_device_get(struct class_device *class_dev)

net/core/dev.c renamed from net/core/dev.c~sysfs_rename_dir-cleanup +7 -5

diff -puN net/core/dev.c~sysfs_rename_dir-cleanup net/core/dev.c
@@ -811,12 +811,14 @@ int dev_change_name(struct net_device *d
 	else
 		strlcpy(dev->name, newname, IFNAMSIZ);
 
-	hlist_del(&dev->name_hlist);
-	hlist_add_head(&dev->name_hlist, dev_name_hash(dev->name));
+	err = class_device_rename(&dev->class_dev, dev->name);
+	if (!err) {
+		hlist_del(&dev->name_hlist);
+		hlist_add_head(&dev->name_hlist, dev_name_hash(dev->name));
+		notifier_call_chain(&netdev_chain, NETDEV_CHANGENAME, dev);
+	}
 
-	class_device_rename(&dev->class_dev, dev->name);
-	notifier_call_chain(&netdev_chain, NETDEV_CHANGENAME, dev);
-	return 0;
+	return err;
 }
 
 /**

Signature · 6 lines

Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

Maneesh Soni

author

2004-05-05 04:55 UTC #36 lore raw

Re: [RFC 2/2] kobject_set_name - error handling

Quoted 2 lines · Maneesh Soni

The previous one had compilation problems. Corrected now.

Thanks
Maneesh

int sysfs_rename_dir(struct kobject * kobj, const char *new_name) int kobject_rename(struct kobject * kobj, char *new_name)

 drivers/base/class.c    |    6 ++++--
 fs/sysfs/dir.c          |   14 +++++++++-----
 include/linux/kobject.h |    2 +-
 include/linux/sysfs.h   |    2 +-
 lib/kobject.c           |   10 +++++++---
 net/core/dev.c          |   16 ++++++++++------
 6 files changed, 32 insertions(+), 18 deletions(-)

fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs_rename_dir-cleanup +9 -5

diff -puN fs/sysfs/dir.c~sysfs_rename_dir-cleanup fs/sysfs/dir.c
@@ -162,15 +162,16 @@ restart:
 	dput(dentry);
 }
 
-void sysfs_rename_dir(struct kobject * kobj, const char *new_name)
+int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
 {
+	int error = 0;
 	struct dentry * new_dentry, * parent;
 
 	if (!strcmp(kobject_name(kobj), new_name))
-		return;
+		return -EINVAL;
 
 	if (!kobj->parent)
-		return;
+		return -EINVAL;
 
 	down_write(&sysfs_rename_sem);
 	parent = kobj->parent->dentry;

@@ -179,13 +180,16 @@ void sysfs_rename_dir(struct kobject * k
 	new_dentry = sysfs_get_dentry(parent, new_name);
 	if (!IS_ERR(new_dentry)) {
 		if (!new_dentry->d_inode) {
-			d_move(kobj->dentry, new_dentry);
-			kobject_set_name(kobj,new_name);
+			error = kobject_set_name(kobj,new_name);
+			if (!error)
+				d_move(kobj->dentry, new_dentry);
 		}
 		dput(new_dentry);
 	}
 	up(&parent->d_inode->i_sem);	
 	up_write(&sysfs_rename_sem);
+
+	return error;
 }
 
 EXPORT_SYMBOL(sysfs_create_dir);

include/linux/sysfs.h renamed from include/linux/sysfs.h~sysfs_rename_dir-cleanup +1 -1

diff -puN include/linux/sysfs.h~sysfs_rename_dir-cleanup include/linux/sysfs.h
@@ -44,7 +44,7 @@ sysfs_create_dir(struct kobject *);
 extern void
 sysfs_remove_dir(struct kobject *);
 
-extern void
+extern int
 sysfs_rename_dir(struct kobject *, const char *new_name);
 
 extern int

lib/kobject.c renamed from lib/kobject.c~sysfs_rename_dir-cleanup +7 -3

diff -puN lib/kobject.c~sysfs_rename_dir-cleanup lib/kobject.c
@@ -385,13 +385,17 @@ EXPORT_SYMBOL(kobject_set_name);
  *	@new_name: object's new name
  */
 
-void kobject_rename(struct kobject * kobj, char *new_name)
+int kobject_rename(struct kobject * kobj, char *new_name)
 {
+	int error = 0;
+
 	kobj = kobject_get(kobj);
 	if (!kobj)
-		return;
-	sysfs_rename_dir(kobj, new_name);
+		return -EINVAL;
+	error = sysfs_rename_dir(kobj, new_name);
 	kobject_put(kobj);
+
+	return error;
 }
 
 /**

include/linux/kobject.h renamed from include/linux/kobject.h~sysfs_rename_dir-cleanup +1 -1

diff -puN include/linux/kobject.h~sysfs_rename_dir-cleanup include/linux/kobject.h
@@ -48,7 +48,7 @@ extern void kobject_cleanup(struct kobje
 extern int kobject_add(struct kobject *);
 extern void kobject_del(struct kobject *);
 
-extern void kobject_rename(struct kobject *, char *new_name);
+extern int kobject_rename(struct kobject *, char *new_name);
 
 extern int kobject_register(struct kobject *);
 extern void kobject_unregister(struct kobject *);

drivers/base/class.c renamed from drivers/base/class.c~sysfs_rename_dir-cleanup +4 -2

diff -puN drivers/base/class.c~sysfs_rename_dir-cleanup drivers/base/class.c
@@ -361,6 +361,8 @@ void class_device_unregister(struct clas
 
 int class_device_rename(struct class_device *class_dev, char *new_name)
 {
+	int error = 0;
+
 	class_dev = class_device_get(class_dev);
 	if (!class_dev)
 		return -EINVAL;

@@ -370,11 +372,11 @@ int class_device_rename(struct class_dev
 
 	strlcpy(class_dev->class_id, new_name, KOBJ_NAME_LEN);
 
-	kobject_rename(&class_dev->kobj, new_name);
+	error = kobject_rename(&class_dev->kobj, new_name);
 
 	class_device_put(class_dev);
 
-	return 0;
+	return error;
 }
 
 struct class_device * class_device_get(struct class_device *class_dev)

net/core/dev.c renamed from net/core/dev.c~sysfs_rename_dir-cleanup +10 -6

diff -puN net/core/dev.c~sysfs_rename_dir-cleanup net/core/dev.c
@@ -792,6 +792,8 @@ int dev_alloc_name(struct net_device *de
  */
 int dev_change_name(struct net_device *dev, char *newname)
 {
+	int err = 0;
+
 	ASSERT_RTNL();
 
 	if (dev->flags & IFF_UP)

@@ -801,7 +803,7 @@ int dev_change_name(struct net_device *d
 		return -EINVAL;
 
 	if (strchr(newname, '%')) {
-		int err = dev_alloc_name(dev, newname);
+		err = dev_alloc_name(dev, newname);
 		if (err < 0)
 			return err;
 		strcpy(newname, dev->name);

@@ -811,12 +813,14 @@ int dev_change_name(struct net_device *d
 	else
 		strlcpy(dev->name, newname, IFNAMSIZ);
 
-	hlist_del(&dev->name_hlist);
-	hlist_add_head(&dev->name_hlist, dev_name_hash(dev->name));
+	err = class_device_rename(&dev->class_dev, dev->name);
+	if (!err) {
+		hlist_del(&dev->name_hlist);
+		hlist_add_head(&dev->name_hlist, dev_name_hash(dev->name));
+		notifier_call_chain(&netdev_chain, NETDEV_CHANGENAME, dev);
+	}
 
-	class_device_rename(&dev->class_dev, dev->name);
-	notifier_call_chain(&netdev_chain, NETDEV_CHANGENAME, dev);
-	return 0;
+	return err;
 }
 
 /**

Signature · 6 lines

Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

Dmitry Torokhov

2004-04-30 12:48 UTC #37 lore raw

Re: [RFC 1/2] kobject_set_name - error handling

In reply to Maneesh Soni

drivers/base/bus.c int bus_add_driver(struct device_driver · line 456

Quoted 11 lines · Maneesh Soni

diff -puN drivers/base/bus.c~kobject_set_name-cleanup-01 drivers/base/bus.c
drivers/base/bus.c renamed from drivers/base/bus.c~kobject_set_name-cleanup-01 +3 -1
@@ -451,7 +451,9 @@ int bus_add_driver(struct device_driver 
 
 	if (bus) {
 		pr_debug("bus %s: add driver %s\n",bus->name,drv->name);
-		kobject_set_name(&drv->kobj,drv->name);
+		error = kobject_set_name(&drv->kobj,drv->name);
+		if (error)
+			return error;

Hi, I think you are leaking a reference here, put_bus() is needed.

Quoted 3 lines

drivers/base/bus.c renamed from drivers/base/bus.c~kobject_set_name-cleanup-01 +0 -0
 		drv->kobj.kset = &bus->drivers;
 		if ((error = kobject_register(&drv->kobj))) {
 			put_bus(bus);

Signature · 1 line

Dmitry

Maneesh Soni

author

2004-05-04 05:35 UTC #38 lore raw

Re: [RFC 1/2] kobject_set_name - error handling

Quoted 7 lines · Dmitry Torokhov

Quoted 5 lines · Maneesh Soni
diff -puN drivers/base/bus.c~kobject_set_name-cleanup-01 drivers/base/bus.c
drivers/base/bus.c renamed from drivers/base/bus.c~kobject_set_name-cleanup-01 +0 -0
@@ -451,7 +451,9 @@ int bus_add_driver(struct device_driver 
 

Quoted 13 lines

drivers/base/bus.c int bus_add_driver(struct device_driver · line 456
Quoted 6 lines
drivers/base/bus.c renamed from drivers/base/bus.c~kobject_set_name-cleanup-01 +3 -1
 	if (bus) {
 		pr_debug("bus %s: add driver %s\n",bus->name,drv->name);
-		kobject_set_name(&drv->kobj,drv->name);
+		error = kobject_set_name(&drv->kobj,drv->name);
+		if (error)
+			return error;
Hi, I think you are leaking a reference here, put_bus() is needed.
Quoted 3 lines
drivers/base/bus.c renamed from drivers/base/bus.c~kobject_set_name-cleanup-01 +0 -0
 		drv->kobj.kset = &bus->drivers;
 		if ((error = kobject_register(&drv->kobj))) {
 			put_bus(bus);

Thanks for spotting it. Corrected patch is appended.

Greg, Are the patches fit for inclusion? I need to know this as my sysfs backing store patches are taking back seats because of these changes, particulary the one in second patch :-(.

Thanks
Maneesh

 drivers/base/bus.c |   14 +++++++++++---
 drivers/base/sys.c |    5 ++++-
 2 files changed, 15 insertions(+), 4 deletions(-)

drivers/base/sys.c renamed from drivers/base/sys.c~kobject_set_name-cleanup-01 +4 -1

diff -puN drivers/base/sys.c~kobject_set_name-cleanup-01 drivers/base/sys.c
@@ -180,8 +180,11 @@ int sysdev_register(struct sys_device * 
 
 	/* But make sure we point to the right type for sysfs translation */
 	sysdev->kobj.ktype = &ktype_sysdev;
-	kobject_set_name(&sysdev->kobj,"%s%d",
+	error = kobject_set_name(&sysdev->kobj,"%s%d",
 			 kobject_name(&cls->kset.kobj),sysdev->id);
+	if (error)
+		return error;
+
 	pr_debug("Registering sys device '%s'\n",kobject_name(&sysdev->kobj));
 
 	/* Register the object */

drivers/base/bus.c renamed from drivers/base/bus.c~kobject_set_name-cleanup-01 +11 -3

diff -puN drivers/base/bus.c~kobject_set_name-cleanup-01 drivers/base/bus.c
@@ -451,7 +451,11 @@ int bus_add_driver(struct device_driver 
 
 	if (bus) {
 		pr_debug("bus %s: add driver %s\n",bus->name,drv->name);
-		kobject_set_name(&drv->kobj,drv->name);
+		error = kobject_set_name(&drv->kobj,drv->name);
+		if (error) {
+			put_bus(bus)
+			return error;
+		}
 		drv->kobj.kset = &bus->drivers;
 		if ((error = kobject_register(&drv->kobj))) {
 			put_bus(bus);

@@ -555,7 +559,11 @@ struct bus_type * find_bus(char * name)
  */
 int bus_register(struct bus_type * bus)
 {
-	kobject_set_name(&bus->subsys.kset.kobj,bus->name);
+	int error = 0;
+
+	error = kobject_set_name(&bus->subsys.kset.kobj,bus->name);
+	if (error)
+		return error;
 	subsys_set_kset(bus,bus_subsys);
 	subsystem_register(&bus->subsys);

@@ -569,7 +577,7 @@ int bus_register(struct bus_type * bus)
 	kset_register(&bus->drivers);
 
 	pr_debug("bus type '%s' registered\n",bus->name);
-	return 0;
+	return error;
 }

Signature · 6 lines

Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

Maneesh Soni

author

2004-05-04 09:16 UTC #39 lore raw

Re: [RFC 1/2] kobject_set_name - error handling

Quoted 2 lines · Maneesh Soni

Quoted 1 line · Dmitry Torokhov
On Friday 30 April 2004 05:14 am, Maneesh Soni wrote:

I have to resend this. Sorry.

Maneesh

o The following patch cleansup the kobject_set_name() users. Basically checking return code from kobject_set_name(). There can be error returns like -ENOMEM or -EFAULT from kobject_set_name() if the name length exceeds KOBJ_NAME_LEN.

 drivers/base/bus.c |   14 +++++++++++---
 drivers/base/sys.c |    5 ++++-
 2 files changed, 15 insertions(+), 4 deletions(-)

drivers/base/sys.c renamed from drivers/base/sys.c~kobject_set_name-cleanup-01 +4 -1

diff -puN drivers/base/sys.c~kobject_set_name-cleanup-01 drivers/base/sys.c
@@ -180,8 +180,11 @@ int sysdev_register(struct sys_device * 
 
 	/* But make sure we point to the right type for sysfs translation */
 	sysdev->kobj.ktype = &ktype_sysdev;
-	kobject_set_name(&sysdev->kobj,"%s%d",
+	error = kobject_set_name(&sysdev->kobj,"%s%d",
 			 kobject_name(&cls->kset.kobj),sysdev->id);
+	if (error)
+		return error;
+
 	pr_debug("Registering sys device '%s'\n",kobject_name(&sysdev->kobj));
 
 	/* Register the object */

drivers/base/bus.c renamed from drivers/base/bus.c~kobject_set_name-cleanup-01 +11 -3

diff -puN drivers/base/bus.c~kobject_set_name-cleanup-01 drivers/base/bus.c
@@ -451,7 +451,11 @@ int bus_add_driver(struct device_driver 
 
 	if (bus) {
 		pr_debug("bus %s: add driver %s\n",bus->name,drv->name);
-		kobject_set_name(&drv->kobj,drv->name);
+		error = kobject_set_name(&drv->kobj,drv->name);
+		if (error) {
+			put_bus(bus);
+			return error;
+		}
 		drv->kobj.kset = &bus->drivers;
 		if ((error = kobject_register(&drv->kobj))) {
 			put_bus(bus);

@@ -555,7 +559,11 @@ struct bus_type * find_bus(char * name)
  */
 int bus_register(struct bus_type * bus)
 {
-	kobject_set_name(&bus->subsys.kset.kobj,bus->name);
+	int error = 0;
+
+	error = kobject_set_name(&bus->subsys.kset.kobj,bus->name);
+	if (error)
+		return error;
 	subsys_set_kset(bus,bus_subsys);
 	subsystem_register(&bus->subsys);

@@ -569,7 +577,7 @@ int bus_register(struct bus_type * bus)
 	kset_register(&bus->drivers);
 
 	pr_debug("bus type '%s' registered\n",bus->name);
-	return 0;
+	return error;
 }

Signature · 6 lines

Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

Greg KH

2004-05-08 00:22 UTC #40 lore raw

Re: [RFC 1/2] kobject_set_name - error handling

In reply to Maneesh Soni

Quoted 4 lines · Maneesh Soni

Greg, Are the patches fit for inclusion? I need to know this as my sysfs backing store patches are taking back seats because of these changes, particulary the one in second patch :-(.

I'm awash in different patches from you. Can you try sending me the ones you think are good enough for inclusion right now? We can work from there.

thanks,

greg k-h

Maneesh Soni

author

2004-05-08 09:57 UTC #41 lore raw

Re: [RFC 1/2] kobject_set_name - error handling

Quoted 10 lines · Greg KH

Quoted 4 lines · Maneesh Soni
Greg, Are the patches fit for inclusion? I need to know this as my sysfs backing store patches are taking back seats because of these changes, particulary the one in second patch :-(.
I'm awash in different patches from you. Can you try sending me the ones you think are good enough for inclusion right now? We can work from there.

Sorry Greg, for confusing you by sending multiple copies. Here we are talking about two patches which cleans up the kobject_set_name() usuage in the routines as mentioned below. The first one is appended here and the second one in the next mail. I have complied and tested (booting) both the patches and hope they are good for inclusion.

1) kobject_set_name-cleanup-01.patch

This patch corrects the following by checking the reutrn code from kobject_set_name().

bus_add_driver()
bus_register()
sys_dev_register()

o The following patch cleansup the kobject_set_name() users. Basically checking return code from kobject_set_name(). There can be error returns like -ENOMEM or -EFAULT from kobject_set_name() if the name length exceeds KOBJ_NAME_LEN.

 drivers/base/bus.c |   14 +++++++++++---
 drivers/base/sys.c |    5 ++++-
 2 files changed, 15 insertions(+), 4 deletions(-)

drivers/base/sys.c renamed from drivers/base/sys.c~kobject_set_name-cleanup-01 +4 -1

diff -puN drivers/base/sys.c~kobject_set_name-cleanup-01 drivers/base/sys.c
@@ -180,8 +180,11 @@ int sysdev_register(struct sys_device * 
 
 	/* But make sure we point to the right type for sysfs translation */
 	sysdev->kobj.ktype = &ktype_sysdev;
-	kobject_set_name(&sysdev->kobj,"%s%d",
+	error = kobject_set_name(&sysdev->kobj,"%s%d",
 			 kobject_name(&cls->kset.kobj),sysdev->id);
+	if (error)
+		return error;
+
 	pr_debug("Registering sys device '%s'\n",kobject_name(&sysdev->kobj));
 
 	/* Register the object */

drivers/base/bus.c renamed from drivers/base/bus.c~kobject_set_name-cleanup-01 +11 -3

diff -puN drivers/base/bus.c~kobject_set_name-cleanup-01 drivers/base/bus.c
@@ -451,7 +451,11 @@ int bus_add_driver(struct device_driver 
 
 	if (bus) {
 		pr_debug("bus %s: add driver %s\n",bus->name,drv->name);
-		kobject_set_name(&drv->kobj,drv->name);
+		error = kobject_set_name(&drv->kobj,drv->name);
+		if (error) {
+			put_bus(bus);
+			return error;
+		}
 		drv->kobj.kset = &bus->drivers;
 		if ((error = kobject_register(&drv->kobj))) {
 			put_bus(bus);

@@ -555,7 +559,11 @@ struct bus_type * find_bus(char * name)
  */
 int bus_register(struct bus_type * bus)
 {
-	kobject_set_name(&bus->subsys.kset.kobj,bus->name);
+	int error = 0;
+
+	error = kobject_set_name(&bus->subsys.kset.kobj,bus->name);
+	if (error)
+		return error;
 	subsys_set_kset(bus,bus_subsys);
 	subsystem_register(&bus->subsys);

@@ -569,7 +577,7 @@ int bus_register(struct bus_type * bus)
 	kset_register(&bus->drivers);
 
 	pr_debug("bus type '%s' registered\n",bus->name);
-	return 0;
+	return error;
 }

Signature · 5 lines

Maneesh Soni
IBM Linux Technology Center, 
IBM India Software Lab, Bangalore.
Phone: +91-80-5044999 email: [email protected]
http://lse.sourceforge.net/

Maneesh Soni

author

2004-05-08 10:02 UTC #42 lore raw

RFC 2/2 sysfs_rename_dir-cleanup

int sysfs_rename_dir(struct kobject * kobj, const char *new_name) int kobject_rename(struct kobject * kobj, char *new_name)

 drivers/base/class.c    |    6 ++++--
 fs/sysfs/dir.c          |   14 +++++++++-----
 include/linux/kobject.h |    2 +-
 include/linux/sysfs.h   |    2 +-
 lib/kobject.c           |   10 +++++++---
 net/core/dev.c          |   16 ++++++++++------
 6 files changed, 32 insertions(+), 18 deletions(-)

fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs_rename_dir-cleanup +9 -5

diff -puN fs/sysfs/dir.c~sysfs_rename_dir-cleanup fs/sysfs/dir.c
@@ -162,15 +162,16 @@ restart:
 	dput(dentry);
 }
 
-void sysfs_rename_dir(struct kobject * kobj, const char *new_name)
+int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
 {
+	int error = 0;
 	struct dentry * new_dentry, * parent;
 
 	if (!strcmp(kobject_name(kobj), new_name))
-		return;
+		return -EINVAL;
 
 	if (!kobj->parent)
-		return;
+		return -EINVAL;
 
 	down_write(&sysfs_rename_sem);
 	parent = kobj->parent->dentry;

@@ -179,13 +180,16 @@ void sysfs_rename_dir(struct kobject * k
 	new_dentry = sysfs_get_dentry(parent, new_name);
 	if (!IS_ERR(new_dentry)) {
 		if (!new_dentry->d_inode) {
-			d_move(kobj->dentry, new_dentry);
-			kobject_set_name(kobj,new_name);
+			error = kobject_set_name(kobj,new_name);
+			if (!error)
+				d_move(kobj->dentry, new_dentry);
 		}
 		dput(new_dentry);
 	}
 	up(&parent->d_inode->i_sem);	
 	up_write(&sysfs_rename_sem);
+
+	return error;
 }
 
 EXPORT_SYMBOL(sysfs_create_dir);

include/linux/sysfs.h renamed from include/linux/sysfs.h~sysfs_rename_dir-cleanup +1 -1

diff -puN include/linux/sysfs.h~sysfs_rename_dir-cleanup include/linux/sysfs.h
@@ -44,7 +44,7 @@ sysfs_create_dir(struct kobject *);
 extern void
 sysfs_remove_dir(struct kobject *);
 
-extern void
+extern int
 sysfs_rename_dir(struct kobject *, const char *new_name);
 
 extern int

lib/kobject.c renamed from lib/kobject.c~sysfs_rename_dir-cleanup +7 -3

diff -puN lib/kobject.c~sysfs_rename_dir-cleanup lib/kobject.c
@@ -385,13 +385,17 @@ EXPORT_SYMBOL(kobject_set_name);
  *	@new_name: object's new name
  */
 
-void kobject_rename(struct kobject * kobj, char *new_name)
+int kobject_rename(struct kobject * kobj, char *new_name)
 {
+	int error = 0;
+
 	kobj = kobject_get(kobj);
 	if (!kobj)
-		return;
-	sysfs_rename_dir(kobj, new_name);
+		return -EINVAL;
+	error = sysfs_rename_dir(kobj, new_name);
 	kobject_put(kobj);
+
+	return error;
 }
 
 /**

include/linux/kobject.h renamed from include/linux/kobject.h~sysfs_rename_dir-cleanup +1 -1

diff -puN include/linux/kobject.h~sysfs_rename_dir-cleanup include/linux/kobject.h
@@ -48,7 +48,7 @@ extern void kobject_cleanup(struct kobje
 extern int kobject_add(struct kobject *);
 extern void kobject_del(struct kobject *);
 
-extern void kobject_rename(struct kobject *, char *new_name);
+extern int kobject_rename(struct kobject *, char *new_name);
 
 extern int kobject_register(struct kobject *);
 extern void kobject_unregister(struct kobject *);

drivers/base/class.c renamed from drivers/base/class.c~sysfs_rename_dir-cleanup +4 -2

diff -puN drivers/base/class.c~sysfs_rename_dir-cleanup drivers/base/class.c
@@ -361,6 +361,8 @@ void class_device_unregister(struct clas
 
 int class_device_rename(struct class_device *class_dev, char *new_name)
 {
+	int error = 0;
+
 	class_dev = class_device_get(class_dev);
 	if (!class_dev)
 		return -EINVAL;

@@ -370,11 +372,11 @@ int class_device_rename(struct class_dev
 
 	strlcpy(class_dev->class_id, new_name, KOBJ_NAME_LEN);
 
-	kobject_rename(&class_dev->kobj, new_name);
+	error = kobject_rename(&class_dev->kobj, new_name);
 
 	class_device_put(class_dev);
 
-	return 0;
+	return error;
 }
 
 struct class_device * class_device_get(struct class_device *class_dev)

net/core/dev.c renamed from net/core/dev.c~sysfs_rename_dir-cleanup +10 -6

diff -puN net/core/dev.c~sysfs_rename_dir-cleanup net/core/dev.c
@@ -792,6 +792,8 @@ int dev_alloc_name(struct net_device *de
  */
 int dev_change_name(struct net_device *dev, char *newname)
 {
+	int err = 0;
+
 	ASSERT_RTNL();
 
 	if (dev->flags & IFF_UP)

@@ -801,7 +803,7 @@ int dev_change_name(struct net_device *d
 		return -EINVAL;
 
 	if (strchr(newname, '%')) {
-		int err = dev_alloc_name(dev, newname);
+		err = dev_alloc_name(dev, newname);
 		if (err < 0)
 			return err;
 		strcpy(newname, dev->name);

@@ -811,12 +813,14 @@ int dev_change_name(struct net_device *d
 	else
 		strlcpy(dev->name, newname, IFNAMSIZ);
 
-	hlist_del(&dev->name_hlist);
-	hlist_add_head(&dev->name_hlist, dev_name_hash(dev->name));
+	err = class_device_rename(&dev->class_dev, dev->name);
+	if (!err) {
+		hlist_del(&dev->name_hlist);
+		hlist_add_head(&dev->name_hlist, dev_name_hash(dev->name));
+		notifier_call_chain(&netdev_chain, NETDEV_CHANGENAME, dev);
+	}
 
-	class_device_rename(&dev->class_dev, dev->name);
-	notifier_call_chain(&netdev_chain, NETDEV_CHANGENAME, dev);
-	return 0;
+	return err;
 }
 
 /**

Signature · 5 lines

Maneesh Soni
IBM Linux Technology Center, 
IBM India Software Lab, Bangalore.
Phone: +91-80-5044999 email: [email protected]
http://lse.sourceforge.net/

Greg KH

2004-05-11 23:45 UTC #43 lore raw

Re: [RFC 2/2] sysfs_rename_dir-cleanup

Quoted 32 lines · Maneesh Soni

diff -puN fs/sysfs/dir.c~sysfs_rename_dir-cleanup fs/sysfs/dir.c

fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs_rename_dir-cleanup +7 -5

@@ -162,15 +162,16 @@ restart:
 	dput(dentry);
 }
 
-void sysfs_rename_dir(struct kobject * kobj, const char *new_name)
+int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
 {
+	int error = 0;
 	struct dentry * new_dentry, * parent;
 
 	if (!strcmp(kobject_name(kobj), new_name))
-		return;
+		return -EINVAL;
 
 	if (!kobj->parent)
-		return;
+		return -EINVAL;
 
 	down_write(&sysfs_rename_sem);
 	parent = kobj->parent->dentry;

@@ -179,13 +180,16 @@ void sysfs_rename_dir(struct kobject * k
 	new_dentry = sysfs_get_dentry(parent, new_name);
 	if (!IS_ERR(new_dentry)) {
 		if (!new_dentry->d_inode) {
-			d_move(kobj->dentry, new_dentry);
-			kobject_set_name(kobj,new_name);
+			error = kobject_set_name(kobj,new_name);
+			if (!error)
+				d_move(kobj->dentry, new_dentry);

fs/sysfs/dir.c void sysfs_rename_dir(struct kobject * k · line 195

Quoted 10 lines

fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs_rename_dir-cleanup +2 -0

 		}
 		dput(new_dentry);
 	}
 	up(&parent->d_inode->i_sem);	
 	up_write(&sysfs_rename_sem);
+
+	return error;
 }
 
 EXPORT_SYMBOL(sysfs_create_dir);

This second chunk fails miserably. I don't know what you diffed it against, as it doesn't look like anything that I currently have in that function...

Anyway, can you grab the next -mm release and rediff this patch against the bk-driver-2.6 tree, or even against a clean 2.6.6 tree so that I can apply it?

thanks,

greg k-h

Maneesh Soni

author

2004-05-12 05:19 UTC #44 lore raw

Re: [RFC 2/2] sysfs_rename_dir-cleanup

On Tue, May 11, 2004 at 04:33:51PM -0700, Greg KH wrote:
[snip]

Quoted 4 lines

This second chunk fails miserably. I don't know what you diffed it against, as it doesn't look like anything that I currently have in that function...

The patch was made against 2.6.6-rc3-mm2 and it needed the symlinks fix also.

Quoted 3 lines

Anyway, can you grab the next -mm release and rediff this patch against the bk-driver-2.6 tree, or even against a clean 2.6.6 tree so that I can apply it?

There will be integration problems as there are multiple changes in this area because of sysfs backing store and others. -mm has sysfs backing store patch set and the symlinks fix, so I diffed it against clean 2.6.6. Hope you will not get any rejects now. Please let me know if you want to integrate it against any other tree.

Thanks
Maneesh

int sysfs_rename_dir(struct kobject * kobj, const char *new_name) int kobject_rename(struct kobject * kobj, char *new_name)

 drivers/base/class.c    |    6 ++++--
 fs/sysfs/dir.c          |   19 ++++++++++++++-----
 include/linux/kobject.h |    2 +-
 include/linux/sysfs.h   |    2 +-
 lib/kobject.c           |   10 +++++++---
 net/core/dev.c          |   16 ++++++++++------
 6 files changed, 37 insertions(+), 18 deletions(-)

fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs_rename_dir-cleanup +14 -5

diff -puN fs/sysfs/dir.c~sysfs_rename_dir-cleanup fs/sysfs/dir.c
@@ -154,24 +154,33 @@ restart:
 	dput(dentry);
 }
 
-void sysfs_rename_dir(struct kobject * kobj, const char *new_name)
+int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
 {
+	int error = 0;
 	struct dentry * new_dentry, * parent;
 
 	if (!strcmp(kobject_name(kobj), new_name))
-		return;
+		return -EINVAL;
 
 	if (!kobj->parent)
-		return;
+		return -EINVAL;
 
 	parent = kobj->parent->dentry;
 
 	down(&parent->d_inode->i_sem);
 
 	new_dentry = sysfs_get_dentry(parent, new_name);
-	d_move(kobj->dentry, new_dentry);
-	kobject_set_name(kobj,new_name);
+	if (!IS_ERR(new_dentrty)) {
+  		if (!new_dentry->d_inode) {
+			error = kobject_set_name(kobj,new_name);
+			if (!error)
+				d_move(kobj->dentry, new_dentry);
+		}
+		dput(new_dentry);
+	}
 	up(&parent->d_inode->i_sem);	
+
+	return error;
 }
 
 EXPORT_SYMBOL(sysfs_create_dir);

include/linux/kobject.h renamed from include/linux/kobject.h~sysfs_rename_dir-cleanup +1 -1

diff -puN include/linux/kobject.h~sysfs_rename_dir-cleanup include/linux/kobject.h
@@ -48,7 +48,7 @@ extern void kobject_cleanup(struct kobje
 extern int kobject_add(struct kobject *);
 extern void kobject_del(struct kobject *);
 
-extern void kobject_rename(struct kobject *, char *new_name);
+extern int kobject_rename(struct kobject *, char *new_name);
 
 extern int kobject_register(struct kobject *);
 extern void kobject_unregister(struct kobject *);

drivers/base/class.c renamed from drivers/base/class.c~sysfs_rename_dir-cleanup +4 -2

diff -puN drivers/base/class.c~sysfs_rename_dir-cleanup drivers/base/class.c
@@ -361,6 +361,8 @@ void class_device_unregister(struct clas
 
 int class_device_rename(struct class_device *class_dev, char *new_name)
 {
+	int error = 0;
+
 	class_dev = class_device_get(class_dev);
 	if (!class_dev)
 		return -EINVAL;

@@ -370,11 +372,11 @@ int class_device_rename(struct class_dev
 
 	strlcpy(class_dev->class_id, new_name, KOBJ_NAME_LEN);
 
-	kobject_rename(&class_dev->kobj, new_name);
+	error = kobject_rename(&class_dev->kobj, new_name);
 
 	class_device_put(class_dev);
 
-	return 0;
+	return error;
 }
 
 struct class_device * class_device_get(struct class_device *class_dev)

lib/kobject.c renamed from lib/kobject.c~sysfs_rename_dir-cleanup +7 -3

diff -puN lib/kobject.c~sysfs_rename_dir-cleanup lib/kobject.c
@@ -385,13 +385,17 @@ EXPORT_SYMBOL(kobject_set_name);
  *	@new_name: object's new name
  */
 
-void kobject_rename(struct kobject * kobj, char *new_name)
+int kobject_rename(struct kobject * kobj, char *new_name)
 {
+	int error = 0;
+
 	kobj = kobject_get(kobj);
 	if (!kobj)
-		return;
-	sysfs_rename_dir(kobj, new_name);
+		return -EINVAL;
+	error = sysfs_rename_dir(kobj, new_name);
 	kobject_put(kobj);
+
+	return error;
 }
 
 /**

include/linux/sysfs.h renamed from include/linux/sysfs.h~sysfs_rename_dir-cleanup +1 -1

diff -puN include/linux/sysfs.h~sysfs_rename_dir-cleanup include/linux/sysfs.h
@@ -44,7 +44,7 @@ sysfs_create_dir(struct kobject *);
 extern void
 sysfs_remove_dir(struct kobject *);
 
-extern void
+extern int
 sysfs_rename_dir(struct kobject *, const char *new_name);
 
 extern int

net/core/dev.c renamed from net/core/dev.c~sysfs_rename_dir-cleanup +10 -6

diff -puN net/core/dev.c~sysfs_rename_dir-cleanup net/core/dev.c
@@ -792,6 +792,8 @@ int dev_alloc_name(struct net_device *de
  */
 int dev_change_name(struct net_device *dev, char *newname)
 {
+	int err = 0;
+
 	ASSERT_RTNL();
 
 	if (dev->flags & IFF_UP)

@@ -801,7 +803,7 @@ int dev_change_name(struct net_device *d
 		return -EINVAL;
 
 	if (strchr(newname, '%')) {
-		int err = dev_alloc_name(dev, newname);
+		err = dev_alloc_name(dev, newname);
 		if (err < 0)
 			return err;
 		strcpy(newname, dev->name);

@@ -811,12 +813,14 @@ int dev_change_name(struct net_device *d
 	else
 		strlcpy(dev->name, newname, IFNAMSIZ);
 
-	hlist_del(&dev->name_hlist);
-	hlist_add_head(&dev->name_hlist, dev_name_hash(dev->name));
+	err = class_device_rename(&dev->class_dev, dev->name);
+	if (!err) {
+		hlist_del(&dev->name_hlist);
+		hlist_add_head(&dev->name_hlist, dev_name_hash(dev->name));
+		notifier_call_chain(&netdev_chain, NETDEV_CHANGENAME, dev);
+	}
 
-	class_device_rename(&dev->class_dev, dev->name);
-	notifier_call_chain(&netdev_chain, NETDEV_CHANGENAME, dev);
-	return 0;
+	return err;
 }
 
 /**

Signature · 6 lines

Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

Maneesh Soni

author

2004-05-12 05:41 UTC #45 lore raw

Re: [RFC 2/2] sysfs_rename_dir-cleanup

Quoted 13 lines · Maneesh Soni

On Tue, May 11, 2004 at 04:33:51PM -0700, Greg KH wrote:
[snip]
Quoted 4 lines
This second chunk fails miserably. I don't know what you diffed it against, as it doesn't look like anything that I currently have in that function...
The patch was made against 2.6.6-rc3-mm2 and it needed the symlinks fix also.
Quoted 3 lines
Anyway, can you grab the next -mm release and rediff this patch against the bk-driver-2.6 tree, or even against a clean 2.6.6 tree so that I can apply it?

Quoted 7 lines

There will be integration problems as there are multiple changes in this area because of sysfs backing store and others. -mm has sysfs backing store patch set and the symlinks fix, so I diffed it against clean 2.6.6. Hope you will not get any rejects now. Please let me know if you want to integrate it against any other tree.

Bad me.. can't wait for compilation to over. There was one typo. Very sorry for the bad patch. Just make "new_dentrty" to "new_dentry" or use this correct one.

int sysfs_rename_dir(struct kobject * kobj, const char *new_name) int kobject_rename(struct kobject * kobj, char *new_name)

 drivers/base/class.c    |    6 ++++--
 fs/sysfs/dir.c          |   19 ++++++++++++++-----
 include/linux/kobject.h |    2 +-
 include/linux/sysfs.h   |    2 +-
 lib/kobject.c           |   10 +++++++---
 net/core/dev.c          |   16 ++++++++++------
 6 files changed, 37 insertions(+), 18 deletions(-)

fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs_rename_dir-cleanup +14 -5

diff -puN fs/sysfs/dir.c~sysfs_rename_dir-cleanup fs/sysfs/dir.c
@@ -154,24 +154,33 @@ restart:
 	dput(dentry);
 }
 
-void sysfs_rename_dir(struct kobject * kobj, const char *new_name)
+int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
 {
+	int error = 0;
 	struct dentry * new_dentry, * parent;
 
 	if (!strcmp(kobject_name(kobj), new_name))
-		return;
+		return -EINVAL;
 
 	if (!kobj->parent)
-		return;
+		return -EINVAL;
 
 	parent = kobj->parent->dentry;
 
 	down(&parent->d_inode->i_sem);
 
 	new_dentry = sysfs_get_dentry(parent, new_name);
-	d_move(kobj->dentry, new_dentry);
-	kobject_set_name(kobj,new_name);
+	if (!IS_ERR(new_dentry)) {
+  		if (!new_dentry->d_inode) {
+			error = kobject_set_name(kobj,new_name);
+			if (!error)
+				d_move(kobj->dentry, new_dentry);
+		}
+		dput(new_dentry);
+	}
 	up(&parent->d_inode->i_sem);	
+
+	return error;
 }
 
 EXPORT_SYMBOL(sysfs_create_dir);

include/linux/kobject.h renamed from include/linux/kobject.h~sysfs_rename_dir-cleanup +1 -1

diff -puN include/linux/kobject.h~sysfs_rename_dir-cleanup include/linux/kobject.h
@@ -48,7 +48,7 @@ extern void kobject_cleanup(struct kobje
 extern int kobject_add(struct kobject *);
 extern void kobject_del(struct kobject *);
 
-extern void kobject_rename(struct kobject *, char *new_name);
+extern int kobject_rename(struct kobject *, char *new_name);
 
 extern int kobject_register(struct kobject *);
 extern void kobject_unregister(struct kobject *);

drivers/base/class.c renamed from drivers/base/class.c~sysfs_rename_dir-cleanup +4 -2

diff -puN drivers/base/class.c~sysfs_rename_dir-cleanup drivers/base/class.c
@@ -361,6 +361,8 @@ void class_device_unregister(struct clas
 
 int class_device_rename(struct class_device *class_dev, char *new_name)
 {
+	int error = 0;
+
 	class_dev = class_device_get(class_dev);
 	if (!class_dev)
 		return -EINVAL;

@@ -370,11 +372,11 @@ int class_device_rename(struct class_dev
 
 	strlcpy(class_dev->class_id, new_name, KOBJ_NAME_LEN);
 
-	kobject_rename(&class_dev->kobj, new_name);
+	error = kobject_rename(&class_dev->kobj, new_name);
 
 	class_device_put(class_dev);
 
-	return 0;
+	return error;
 }
 
 struct class_device * class_device_get(struct class_device *class_dev)

lib/kobject.c renamed from lib/kobject.c~sysfs_rename_dir-cleanup +7 -3

diff -puN lib/kobject.c~sysfs_rename_dir-cleanup lib/kobject.c
@@ -385,13 +385,17 @@ EXPORT_SYMBOL(kobject_set_name);
  *	@new_name: object's new name
  */
 
-void kobject_rename(struct kobject * kobj, char *new_name)
+int kobject_rename(struct kobject * kobj, char *new_name)
 {
+	int error = 0;
+
 	kobj = kobject_get(kobj);
 	if (!kobj)
-		return;
-	sysfs_rename_dir(kobj, new_name);
+		return -EINVAL;
+	error = sysfs_rename_dir(kobj, new_name);
 	kobject_put(kobj);
+
+	return error;
 }
 
 /**

include/linux/sysfs.h renamed from include/linux/sysfs.h~sysfs_rename_dir-cleanup +1 -1

diff -puN include/linux/sysfs.h~sysfs_rename_dir-cleanup include/linux/sysfs.h
@@ -44,7 +44,7 @@ sysfs_create_dir(struct kobject *);
 extern void
 sysfs_remove_dir(struct kobject *);
 
-extern void
+extern int
 sysfs_rename_dir(struct kobject *, const char *new_name);
 
 extern int

net/core/dev.c renamed from net/core/dev.c~sysfs_rename_dir-cleanup +10 -6

diff -puN net/core/dev.c~sysfs_rename_dir-cleanup net/core/dev.c
@@ -792,6 +792,8 @@ int dev_alloc_name(struct net_device *de
  */
 int dev_change_name(struct net_device *dev, char *newname)
 {
+	int err = 0;
+
 	ASSERT_RTNL();
 
 	if (dev->flags & IFF_UP)

@@ -801,7 +803,7 @@ int dev_change_name(struct net_device *d
 		return -EINVAL;
 
 	if (strchr(newname, '%')) {
-		int err = dev_alloc_name(dev, newname);
+		err = dev_alloc_name(dev, newname);
 		if (err < 0)
 			return err;
 		strcpy(newname, dev->name);

@@ -811,12 +813,14 @@ int dev_change_name(struct net_device *d
 	else
 		strlcpy(dev->name, newname, IFNAMSIZ);
 
-	hlist_del(&dev->name_hlist);
-	hlist_add_head(&dev->name_hlist, dev_name_hash(dev->name));
+	err = class_device_rename(&dev->class_dev, dev->name);
+	if (!err) {
+		hlist_del(&dev->name_hlist);
+		hlist_add_head(&dev->name_hlist, dev_name_hash(dev->name));
+		notifier_call_chain(&netdev_chain, NETDEV_CHANGENAME, dev);
+	}
 
-	class_device_rename(&dev->class_dev, dev->name);
-	notifier_call_chain(&netdev_chain, NETDEV_CHANGENAME, dev);
-	return 0;
+	return err;
 }
 
 /**

Signature · 6 lines

Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

Greg KH

2004-05-14 19:12 UTC #46 lore raw

Re: [RFC 2/2] sysfs_rename_dir-cleanup

Quoted 4 lines · Maneesh Soni

Bad me.. can't wait for compilation to over. There was one typo. Very sorry for the bad patch. Just make "new_dentrty" to "new_dentry" or use this correct one.

Thanks, this one worked, I've applied it.

greg k-h

Greg KH

2004-05-11 23:47 UTC #47 lore raw

Re: [RFC 1/2] kobject_set_name - error handling

In reply to Maneesh Soni

Quoted 19 lines · Maneesh Soni

Quoted 10 lines · Greg KH
Quoted 4 lines · Maneesh Soni
Greg, Are the patches fit for inclusion? I need to know this as my sysfs backing store patches are taking back seats because of these changes, particulary the one in second patch :-(.
I'm awash in different patches from you. Can you try sending me the ones you think are good enough for inclusion right now? We can work from there.
Sorry Greg, for confusing you by sending multiple copies. Here we are talking about two patches which cleans up the kobject_set_name() usuage in the routines as mentioned below. The first one is appended here and the second one in the next mail. I have complied and tested (booting) both the patches and hope they are good for inclusion.

Quoted 8 lines

1) kobject_set_name-cleanup-01.patch
This patch corrects the following by checking the reutrn code from kobject_set_name().
bus_add_driver()
bus_register()
sys_dev_register()

Ok, I applied this patch (it needed some manual messing with due to some other patches in this area by others.)

thanks,

greg k-h

Greg KH

2004-04-17 00:17 UTC #48 lore raw

In reply to [email protected]

Quoted 9 lines · On Sat, Apr 17, 2004 at 12:46:02AM +0100…

Quoted 3 lines · Greg KH
Since when did we ever assume that renaming a kobject would rename the symlinks that might point to it? Renaming kobjects are a hack that way, if you use them, you need to be aware of this limitation.
Since we assume that these symlinks actually reflect some relationship between the objects and are really needed for something. If they are not - why the hell do we keep them at all?

They show a relationship, yes. And it would be nice to be able to try to keep that link around if possible.

But what devices currently have this problem? I only thought network devices renamed themselves, and there are no symlinks to those devices, only out from it. In grepping the tree, they seem like the only users of this functionality, and they would never need the symlink "reaname".

thanks,

greg k-h