[RFC] fix sysfs symlinks

[RFC] fix sysfs symlinks

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


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(-)

diff -puN fs/sysfs/symlink.c~sysfs-symlinks-fix fs/sysfs/symlink.c
--- linux-2.6.5/fs/sysfs/symlink.c~sysfs-symlinks-fix	2004-04-12 18:43:15.000000000 +0530
+++ linux-2.6.5-maneesh/fs/sysfs/symlink.c	2004-04-13 17:58:15.000000000 +0530
@@ -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);
diff -puN fs/sysfs/sysfs.h~sysfs-symlinks-fix fs/sysfs/sysfs.h
--- linux-2.6.5/fs/sysfs/sysfs.h~sysfs-symlinks-fix	2004-04-13 12:26:45.000000000 +0530
+++ linux-2.6.5-maneesh/fs/sysfs/sysfs.h	2004-04-13 17:58:24.000000000 +0530
@@ -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;
+}
+

_
-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

Re: [RFC] fix sysfs symlinks

[viro]

On Tue, Apr 13, 2004 at 06:10:37PM +0530, Maneesh Soni wrote:
> 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().

Re: [RFC] fix sysfs symlinks

[Maneesh Soni]

On Tue, Apr 13, 2004 at 02:36:15PM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Tue, Apr 13, 2004 at 06:10:37PM +0530, Maneesh Soni wrote:
> > 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().

> 
> 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 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 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(-)

diff -puN fs/sysfs/symlink.c~sysfs-symlinks-fix fs/sysfs/symlink.c
--- linux-2.6.5/fs/sysfs/symlink.c~sysfs-symlinks-fix	2004-04-13 22:19:15.000000000 +0530
+++ linux-2.6.5-maneesh/fs/sysfs/symlink.c	2004-04-14 12:08:54.000000000 +0530
@@ -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);
diff -puN fs/sysfs/sysfs.h~sysfs-symlinks-fix fs/sysfs/sysfs.h
--- linux-2.6.5/fs/sysfs/sysfs.h~sysfs-symlinks-fix	2004-04-13 22:19:15.000000000 +0530
+++ linux-2.6.5-maneesh/fs/sysfs/sysfs.h	2004-04-13 22:19:15.000000000 +0530
@@ -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;
+}
+

_


-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

Re: [RFC] fix sysfs symlinks

[viro]

On Wed, Apr 14, 2004 at 12:10:16PM +0530, Maneesh Soni wrote:
> 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?
 
> +	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.

Re: [RFC] fix sysfs symlinks

[Maneesh Soni]

On Wed, Apr 14, 2004 at 08:02:27AM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Wed, Apr 14, 2004 at 12:10:16PM +0530, Maneesh Soni wrote:
> > 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

>  
> > +	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 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 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(-)

diff -puN fs/sysfs/symlink.c~sysfs-symlinks-fix fs/sysfs/symlink.c
--- linux-2.6.5/fs/sysfs/symlink.c~sysfs-symlinks-fix	2004-04-13 22:19:15.000000000 +0530
+++ linux-2.6.5-maneesh/fs/sysfs/symlink.c	2004-04-14 12:43:23.000000000 +0530
@@ -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);
diff -puN fs/sysfs/sysfs.h~sysfs-symlinks-fix fs/sysfs/sysfs.h
--- linux-2.6.5/fs/sysfs/sysfs.h~sysfs-symlinks-fix	2004-04-13 22:19:15.000000000 +0530
+++ linux-2.6.5-maneesh/fs/sysfs/sysfs.h	2004-04-13 22:19:15.000000000 +0530
@@ -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;
+}
+

_

-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

Re: [RFC] fix sysfs symlinks

[viro]

On Wed, Apr 14, 2004 at 12:47:56PM +0530, Maneesh Soni wrote:
> On Wed, Apr 14, 2004 at 08:02:27AM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
> > On Wed, Apr 14, 2004 at 12:10:16PM +0530, Maneesh Soni wrote:
> > > 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

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

Re: [RFC] fix sysfs symlinks

[Russell King]

On Wed, Apr 14, 2004 at 08:02:27AM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Wed, Apr 14, 2004 at 12:10:16PM +0530, Maneesh Soni wrote:
> > 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.

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

Re: [RFC] fix sysfs symlinks

[viro]

On Thu, Apr 15, 2004 at 09:17:52AM +0100, Russell King wrote:
> > 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...

Re: [RFC] fix sysfs symlinks

[Russell King]

On Thu, Apr 15, 2004 at 11:38:49AM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
> 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>. ;(

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

Re: [RFC] fix sysfs symlinks

[Greg KH]

On Thu, Apr 15, 2004 at 04:19:42PM +0100, Russell King wrote:
> 
> 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>. ;(

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

Re: [RFC] fix sysfs symlinks

[viro]

On Thu, Apr 15, 2004 at 09:10:11AM -0700, Greg KH wrote:
> 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".

Re: [RFC] fix sysfs symlinks

[viro]

On Thu, Apr 15, 2004 at 05:13:32PM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Thu, Apr 15, 2004 at 09:10:11AM -0700, Greg KH wrote:
> > 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?

Re: [RFC] fix sysfs symlinks

[Greg KH]

On Thu, Apr 15, 2004 at 08:14:47PM +0100, viro@parcelfarce.linux.theplanet.co.uk 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.
> 
> 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

Re: [RFC] fix sysfs symlinks

[Rusty Russell]

On Fri, 2004-04-16 at 05:14, viro@parcelfarce.linux.theplanet.co.uk
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.
> 
> 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.
-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell

Re: [RFC] fix sysfs symlinks

[viro]

On Sat, Apr 17, 2004 at 04:15:34PM +1000, Rusty Russell wrote:
> > 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.

Re: [RFC] fix sysfs symlinks

[Rusty Russell]

On Sun, 2004-04-18 at 05:39, viro@parcelfarce.linux.theplanet.co.uk
wrote:
> On Sat, Apr 17, 2004 at 04:15:34PM +1000, Rusty Russell wrote:
> > > 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.

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.
-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell

Re: [RFC] fix sysfs symlinks

[Greg KH]

On Tue, Apr 13, 2004 at 02:36:15PM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Tue, Apr 13, 2004 at 06:10:37PM +0530, Maneesh Soni wrote:
> > 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

Re: [RFC] fix sysfs symlinks

[viro]

On Thu, Apr 15, 2004 at 03:02:32PM -0700, Greg KH wrote:
> 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.

Re: [RFC] fix sysfs symlinks

[Horst von Brand]

viro@parcelfarce.linux.theplanet.co.uk said:
> On Thu, Apr 15, 2004 at 03:02:32PM -0700, Greg KH wrote:
> > 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.

Sounds an awful lot like ordinary hard links...
-- 
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

Re: [RFC] fix sysfs symlinks

[viro]

On Fri, Apr 16, 2004 at 02:03:21PM -0400, Horst von Brand wrote:
> > 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.

Re: [RFC] fix sysfs symlinks

[Greg KH]

On Fri, Apr 16, 2004 at 04:24:48PM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Thu, Apr 15, 2004 at 03:02:32PM -0700, Greg KH wrote:
> > 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.

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

Re: [RFC] fix sysfs symlinks

[viro]

On Fri, Apr 16, 2004 at 03:37:32PM -0700, Greg KH wrote:

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

Re: [RFC] fix sysfs symlinks

[Jeff Garzik]

viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Fri, Apr 16, 2004 at 03:37:32PM -0700, Greg KH wrote:
> 
> 
>>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

Re: [RFC] fix sysfs symlinks

[Russell King]

On Fri, Apr 16, 2004 at 08:03:50PM -0400, Jeff Garzik wrote:
> 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.

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

Re: [RFC] fix sysfs symlinks

[viro]

On Sat, Apr 17, 2004 at 09:07:12AM +0100, Russell King wrote:
> 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?

Re: [RFC] fix sysfs symlinks

[Greg KH]

On Sat, Apr 17, 2004 at 09:22:06AM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Sat, Apr 17, 2004 at 09:07:12AM +0100, Russell King wrote:
> > 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?

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

Re: [RFC] fix sysfs symlinks

[Maneesh Soni]

On Tue, Apr 20, 2004 at 09:16:02AM -0700, Greg KH wrote:
> On Sat, Apr 17, 2004 at 09:22:06AM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
> > On Sat, Apr 17, 2004 at 09:07:12AM +0100, Russell King wrote:
> > > 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?
> 
> 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(-)

diff -puN fs/sysfs/sysfs.h~sysfs-symlinks-fix fs/sysfs/sysfs.h
--- linux-2.6.6-rc2-mm1/fs/sysfs/sysfs.h~sysfs-symlinks-fix	2004-04-21 14:59:13.000000000 +0530
+++ linux-2.6.6-rc2-mm1-maneesh/fs/sysfs/sysfs.h	2004-04-21 15:00:08.000000000 +0530
@@ -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)
 {
diff -puN fs/sysfs/dir.c~sysfs-symlinks-fix fs/sysfs/dir.c
--- linux-2.6.6-rc2-mm1/fs/sysfs/dir.c~sysfs-symlinks-fix	2004-04-21 14:59:22.000000000 +0530
+++ linux-2.6.6-rc2-mm1-maneesh/fs/sysfs/dir.c	2004-04-21 15:39:30.000000000 +0530
@@ -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);
diff -puN fs/sysfs/inode.c~sysfs-symlinks-fix fs/sysfs/inode.c
--- linux-2.6.6-rc2-mm1/fs/sysfs/inode.c~sysfs-symlinks-fix	2004-04-21 14:59:25.000000000 +0530
+++ linux-2.6.6-rc2-mm1-maneesh/fs/sysfs/inode.c	2004-04-21 15:39:20.000000000 +0530
@@ -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);
 		}
diff -puN fs/sysfs/symlink.c~sysfs-symlinks-fix fs/sysfs/symlink.c
--- linux-2.6.6-rc2-mm1/fs/sysfs/symlink.c~sysfs-symlinks-fix	2004-04-21 14:59:28.000000000 +0530
+++ linux-2.6.6-rc2-mm1-maneesh/fs/sysfs/symlink.c	2004-04-21 15:39:51.000000000 +0530
@@ -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,20 @@ 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)
+			/* 
+			 * associate the link dentry with the target kobject 
+			 */
+			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 +93,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);

_
-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

Re: [RFC] fix sysfs symlinks

[viro]

On Wed, Apr 21, 2004 at 03:41:04PM +0530, Maneesh Soni wrote:
> +			/* 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.

> +			/* 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.

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

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

Re: [RFC] fix sysfs symlinks

[Maneesh Soni]

On Thu, Apr 22, 2004 at 10:37:37PM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
[..]
> 
> 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.
> 
> > +			/* 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.

[..]
> 
> 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().

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

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(-)

diff -puN fs/sysfs/sysfs.h~sysfs-symlinks-fix fs/sysfs/sysfs.h
--- linux-2.6.6-rc2-mm1/fs/sysfs/sysfs.h~sysfs-symlinks-fix	2004-04-23 10:22:41.000000000 +0530
+++ linux-2.6.6-rc2-mm1-maneesh/fs/sysfs/sysfs.h	2004-04-23 12:11:52.000000000 +0530
@@ -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)
 {
diff -puN fs/sysfs/dir.c~sysfs-symlinks-fix fs/sysfs/dir.c
--- linux-2.6.6-rc2-mm1/fs/sysfs/dir.c~sysfs-symlinks-fix	2004-04-23 10:22:41.000000000 +0530
+++ linux-2.6.6-rc2-mm1-maneesh/fs/sysfs/dir.c	2004-04-23 11:30:17.000000000 +0530
@@ -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);	
 }
 
diff -puN fs/sysfs/inode.c~sysfs-symlinks-fix fs/sysfs/inode.c
--- linux-2.6.6-rc2-mm1/fs/sysfs/inode.c~sysfs-symlinks-fix	2004-04-23 10:22:41.000000000 +0530
+++ linux-2.6.6-rc2-mm1-maneesh/fs/sysfs/inode.c	2004-04-23 11:03:38.000000000 +0530
@@ -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);
 		}
 		/*
diff -puN fs/sysfs/symlink.c~sysfs-symlinks-fix fs/sysfs/symlink.c
--- linux-2.6.6-rc2-mm1/fs/sysfs/symlink.c~sysfs-symlinks-fix	2004-04-23 10:22:41.000000000 +0530
+++ linux-2.6.6-rc2-mm1-maneesh/fs/sysfs/symlink.c	2004-04-23 11:40:49.000000000 +0530
@@ -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,20 @@ 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)
+			/* 
+			 * associate the link dentry with the target kobject 
+			 */
+			d->d_fsdata = kobject_get(target);
+		dput(d);
+	} else 
 		error = PTR_ERR(d);
-	dput(d);
 	up(&dentry->d_inode->i_sem);
-	kfree(path);
 	return error;
 }
 
@@ -120,6 +93,87 @@ 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;
+	}
+
+	down_read(&sysfs_rename_sem);
+	error = sysfs_get_target_path(kobj, target_kobj, path);
+	up_read(&sysfs_rename_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);
diff -puN fs/sysfs/group.c~sysfs-symlinks-fix fs/sysfs/group.c
--- linux-2.6.6-rc2-mm1/fs/sysfs/group.c~sysfs-symlinks-fix	2004-04-23 11:20:40.000000000 +0530
+++ linux-2.6.6-rc2-mm1-maneesh/fs/sysfs/group.c	2004-04-23 11:22:12.000000000 +0530
@@ -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);
+	}
 }
 
 

_

-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

Re: [RFC] fix sysfs symlinks

[viro]

On Fri, Apr 23, 2004 at 02:22:18PM +0530, Maneesh Soni wrote:
> @@ -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().

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

> diff -puN fs/sysfs/inode.c~sysfs-symlinks-fix fs/sysfs/inode.c
> --- linux-2.6.6-rc2-mm1/fs/sysfs/inode.c~sysfs-symlinks-fix	2004-04-23 10:22:41.000000000 +0530
> +++ linux-2.6.6-rc2-mm1-maneesh/fs/sysfs/inode.c	2004-04-23 11:03:38.000000000 +0530
> @@ -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.

>  			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);
> -	/* 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?

Re: [RFC] fix sysfs symlinks

[Maneesh Soni]

On Fri, Apr 23, 2004 at 10:26:41AM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Fri, Apr 23, 2004 at 02:22:18PM +0530, Maneesh Soni wrote:
> > @@ -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.

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

[..]

> 
> > @@ -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);
> > +	}
> >  }
> 
> 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?

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

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(-)

diff -puN fs/sysfs/sysfs.h~sysfs-symlinks-fix fs/sysfs/sysfs.h
--- linux-2.6.6-rc2-mm2/fs/sysfs/sysfs.h~sysfs-symlinks-fix	2004-04-29 16:15:13.000000000 +0530
+++ linux-2.6.6-rc2-mm2-maneesh/fs/sysfs/sysfs.h	2004-04-29 18:03:58.000000000 +0530
@@ -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;
 }
diff -puN fs/sysfs/dir.c~sysfs-symlinks-fix fs/sysfs/dir.c
--- linux-2.6.6-rc2-mm2/fs/sysfs/dir.c~sysfs-symlinks-fix	2004-04-29 16:15:13.000000000 +0530
+++ linux-2.6.6-rc2-mm2-maneesh/fs/sysfs/dir.c	2004-04-29 18:31:53.000000000 +0530
@@ -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);	
 }
 
diff -puN fs/sysfs/inode.c~sysfs-symlinks-fix fs/sysfs/inode.c
--- linux-2.6.6-rc2-mm2/fs/sysfs/inode.c~sysfs-symlinks-fix	2004-04-29 16:15:13.000000000 +0530
+++ linux-2.6.6-rc2-mm2-maneesh/fs/sysfs/inode.c	2004-04-29 16:15:13.000000000 +0530
@@ -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);
 		}
 		/*
diff -puN fs/sysfs/symlink.c~sysfs-symlinks-fix fs/sysfs/symlink.c
--- linux-2.6.6-rc2-mm2/fs/sysfs/symlink.c~sysfs-symlinks-fix	2004-04-29 16:15:13.000000000 +0530
+++ linux-2.6.6-rc2-mm2-maneesh/fs/sysfs/symlink.c	2004-04-29 18:04:07.000000000 +0530
@@ -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,20 @@ 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)
+			/* 
+			 * associate the link dentry with the target kobject 
+			 */
+			d->d_fsdata = kobject_get(target);
+		dput(d);
+	} else 
 		error = PTR_ERR(d);
-	dput(d);
 	up(&dentry->d_inode->i_sem);
-	kfree(path);
 	return error;
 }
 
@@ -120,6 +93,86 @@ 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;
+	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;
+	}
+
+	down_read(&sysfs_rename_sem);
+	error = sysfs_get_target_path(kobj, target_kobj, path);
+	up_read(&sysfs_rename_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);

_

-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

Re: [RFC] fix sysfs symlinks

[viro]

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

[snip]

> @@ -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);	
>  }

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.

Re: [RFC] fix sysfs symlinks

[Maneesh Soni]

On Thu, Apr 29, 2004 at 04:41:04PM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Thu, Apr 29, 2004 at 06:33:53PM +0530, Maneesh Soni wrote:
> 
> [snip]
> 
> > @@ -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);	
> >  }
> 
> 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 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.

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(-)

diff -puN fs/sysfs/sysfs.h~sysfs-symlinks-fix fs/sysfs/sysfs.h
--- linux-2.6.6-rc2-mm2/fs/sysfs/sysfs.h~sysfs-symlinks-fix	2004-04-30 11:10:09.000000000 +0530
+++ linux-2.6.6-rc2-mm2-maneesh/fs/sysfs/sysfs.h	2004-04-30 11:10:09.000000000 +0530
@@ -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;
 }
diff -puN fs/sysfs/dir.c~sysfs-symlinks-fix fs/sysfs/dir.c
--- linux-2.6.6-rc2-mm2/fs/sysfs/dir.c~sysfs-symlinks-fix	2004-04-30 11:10:09.000000000 +0530
+++ linux-2.6.6-rc2-mm2-maneesh/fs/sysfs/dir.c	2004-04-30 11:39:16.000000000 +0530
@@ -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);
diff -puN fs/sysfs/inode.c~sysfs-symlinks-fix fs/sysfs/inode.c
--- linux-2.6.6-rc2-mm2/fs/sysfs/inode.c~sysfs-symlinks-fix	2004-04-30 11:10:09.000000000 +0530
+++ linux-2.6.6-rc2-mm2-maneesh/fs/sysfs/inode.c	2004-04-30 11:10:09.000000000 +0530
@@ -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);
 		}
 		/*
diff -puN fs/sysfs/symlink.c~sysfs-symlinks-fix fs/sysfs/symlink.c
--- linux-2.6.6-rc2-mm2/fs/sysfs/symlink.c~sysfs-symlinks-fix	2004-04-30 11:10:09.000000000 +0530
+++ linux-2.6.6-rc2-mm2-maneesh/fs/sysfs/symlink.c	2004-04-30 11:10:09.000000000 +0530
@@ -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,20 @@ 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)
+			/* 
+			 * associate the link dentry with the target kobject 
+			 */
+			d->d_fsdata = kobject_get(target);
+		dput(d);
+	} else 
 		error = PTR_ERR(d);
-	dput(d);
 	up(&dentry->d_inode->i_sem);
-	kfree(path);
 	return error;
 }
 
@@ -120,6 +93,86 @@ 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;
+	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;
+	}
+
+	down_read(&sysfs_rename_sem);
+	error = sysfs_get_target_path(kobj, target_kobj, path);
+	up_read(&sysfs_rename_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);

_


-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

[RFC 0/2] kobject_set_name - error handling

[Maneesh Soni]

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
                                                                                

-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

[RFC 1/2] kobject_set_name - error handling

[Maneesh Soni]

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(-)

diff -puN drivers/base/sys.c~kobject_set_name-cleanup-01 drivers/base/sys.c
--- linux-2.6.6-rc2-mm2/drivers/base/sys.c~kobject_set_name-cleanup-01	2004-04-30 15:14:03.000000000 +0530
+++ linux-2.6.6-rc2-mm2-maneesh/drivers/base/sys.c	2004-04-30 15:14:03.000000000 +0530
@@ -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 */
diff -puN drivers/base/bus.c~kobject_set_name-cleanup-01 drivers/base/bus.c
--- linux-2.6.6-rc2-mm2/drivers/base/bus.c~kobject_set_name-cleanup-01	2004-04-30 15:14:03.000000000 +0530
+++ linux-2.6.6-rc2-mm2-maneesh/drivers/base/bus.c	2004-04-30 15:14:03.000000000 +0530
@@ -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;
 }
 
 

_
-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

[RFC 2/2] kobject_set_name - error handling

[Maneesh Soni]

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(-)

diff -puN fs/sysfs/dir.c~sysfs_rename_dir-cleanup fs/sysfs/dir.c
--- linux-2.6.6-rc2-mm2/fs/sysfs/dir.c~sysfs_rename_dir-cleanup	2004-04-30 15:00:41.000000000 +0530
+++ linux-2.6.6-rc2-mm2-maneesh/fs/sysfs/dir.c	2004-04-30 15:12:13.000000000 +0530
@@ -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);
diff -puN include/linux/sysfs.h~sysfs_rename_dir-cleanup include/linux/sysfs.h
--- linux-2.6.6-rc2-mm2/include/linux/sysfs.h~sysfs_rename_dir-cleanup	2004-04-30 15:00:48.000000000 +0530
+++ linux-2.6.6-rc2-mm2-maneesh/include/linux/sysfs.h	2004-04-30 15:07:23.000000000 +0530
@@ -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
diff -puN lib/kobject.c~sysfs_rename_dir-cleanup lib/kobject.c
--- linux-2.6.6-rc2-mm2/lib/kobject.c~sysfs_rename_dir-cleanup	2004-04-30 15:00:54.000000000 +0530
+++ linux-2.6.6-rc2-mm2-maneesh/lib/kobject.c	2004-04-30 15:08:21.000000000 +0530
@@ -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;
 }
 
 /**
diff -puN include/linux/kobject.h~sysfs_rename_dir-cleanup include/linux/kobject.h
--- linux-2.6.6-rc2-mm2/include/linux/kobject.h~sysfs_rename_dir-cleanup	2004-04-30 15:03:32.000000000 +0530
+++ linux-2.6.6-rc2-mm2-maneesh/include/linux/kobject.h	2004-04-30 15:07:08.000000000 +0530
@@ -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 *);
diff -puN drivers/base/class.c~sysfs_rename_dir-cleanup drivers/base/class.c
--- linux-2.6.6-rc2-mm2/drivers/base/class.c~sysfs_rename_dir-cleanup	2004-04-30 15:04:06.000000000 +0530
+++ linux-2.6.6-rc2-mm2-maneesh/drivers/base/class.c	2004-04-30 15:06:51.000000000 +0530
@@ -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)
diff -puN net/core/dev.c~sysfs_rename_dir-cleanup net/core/dev.c
--- linux-2.6.6-rc2-mm2/net/core/dev.c~sysfs_rename_dir-cleanup	2004-04-30 15:08:36.000000000 +0530
+++ linux-2.6.6-rc2-mm2-maneesh/net/core/dev.c	2004-04-30 15:09:34.000000000 +0530
@@ -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;
 }
 
 /**

_
-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

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

[Maneesh Soni]

On Fri, Apr 30, 2004 at 03:47:18PM +0530, Maneesh Soni wrote:
> 
> 
The previous one had compilation problems. Corrected now. 

Thanks
Maneesh



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)


 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(-)

diff -puN fs/sysfs/dir.c~sysfs_rename_dir-cleanup fs/sysfs/dir.c
--- linux-2.6.6-rc3-mm1/fs/sysfs/dir.c~sysfs_rename_dir-cleanup	2004-04-30 16:07:28.000000000 +0530
+++ linux-2.6.6-rc3-mm1-maneesh/fs/sysfs/dir.c	2004-05-04 14:55:43.000000000 +0530
@@ -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);
diff -puN include/linux/sysfs.h~sysfs_rename_dir-cleanup include/linux/sysfs.h
--- linux-2.6.6-rc3-mm1/include/linux/sysfs.h~sysfs_rename_dir-cleanup	2004-04-30 16:07:28.000000000 +0530
+++ linux-2.6.6-rc3-mm1-maneesh/include/linux/sysfs.h	2004-05-04 14:55:44.000000000 +0530
@@ -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
diff -puN lib/kobject.c~sysfs_rename_dir-cleanup lib/kobject.c
--- linux-2.6.6-rc3-mm1/lib/kobject.c~sysfs_rename_dir-cleanup	2004-04-30 16:07:28.000000000 +0530
+++ linux-2.6.6-rc3-mm1-maneesh/lib/kobject.c	2004-04-30 16:07:28.000000000 +0530
@@ -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;
 }
 
 /**
diff -puN include/linux/kobject.h~sysfs_rename_dir-cleanup include/linux/kobject.h
--- linux-2.6.6-rc3-mm1/include/linux/kobject.h~sysfs_rename_dir-cleanup	2004-04-30 16:07:28.000000000 +0530
+++ linux-2.6.6-rc3-mm1-maneesh/include/linux/kobject.h	2004-04-30 16:07:28.000000000 +0530
@@ -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 *);
diff -puN drivers/base/class.c~sysfs_rename_dir-cleanup drivers/base/class.c
--- linux-2.6.6-rc3-mm1/drivers/base/class.c~sysfs_rename_dir-cleanup	2004-04-30 16:07:28.000000000 +0530
+++ linux-2.6.6-rc3-mm1-maneesh/drivers/base/class.c	2004-04-30 16:07:28.000000000 +0530
@@ -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)
diff -puN net/core/dev.c~sysfs_rename_dir-cleanup net/core/dev.c
--- linux-2.6.6-rc3-mm1/net/core/dev.c~sysfs_rename_dir-cleanup	2004-04-30 16:07:28.000000000 +0530
+++ linux-2.6.6-rc3-mm1-maneesh/net/core/dev.c	2004-05-04 14:56:17.000000000 +0530
@@ -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;
 }
 
 /**

_
-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

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

[Dmitry Torokhov]

On Friday 30 April 2004 05:14 am, Maneesh Soni wrote:

> diff -puN drivers/base/bus.c~kobject_set_name-cleanup-01 drivers/base/bus.c
> --- linux-2.6.6-rc2-mm2/drivers/base/bus.c~kobject_set_name-cleanup-01	2004-04-30 15:14:03.000000000 +0530
> +++ linux-2.6.6-rc2-mm2-maneesh/drivers/base/bus.c	2004-04-30 15:14:03.000000000 +0530
> @@ -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.

>  		drv->kobj.kset = &bus->drivers;
>  		if ((error = kobject_register(&drv->kobj))) {
>  			put_bus(bus);

-- 
Dmitry

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

[Maneesh Soni]

On Fri, Apr 30, 2004 at 07:48:13AM -0500, Dmitry Torokhov wrote:
> On Friday 30 April 2004 05:14 am, Maneesh Soni wrote:
> 
> > diff -puN drivers/base/bus.c~kobject_set_name-cleanup-01 drivers/base/bus.c
> > --- linux-2.6.6-rc2-mm2/drivers/base/bus.c~kobject_set_name-cleanup-01	2004-04-30 15:14:03.000000000 +0530
> > +++ linux-2.6.6-rc2-mm2-maneesh/drivers/base/bus.c	2004-04-30 15:14:03.000000000 +0530
> > @@ -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.
> 
> >  		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


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 |   14 +++++++++++---
 drivers/base/sys.c |    5 ++++-
 2 files changed, 15 insertions(+), 4 deletions(-)

diff -puN drivers/base/sys.c~kobject_set_name-cleanup-01 drivers/base/sys.c
--- linux-2.6.6-rc3-mm1/drivers/base/sys.c~kobject_set_name-cleanup-01	2004-04-30 16:07:32.000000000 +0530
+++ linux-2.6.6-rc3-mm1-maneesh/drivers/base/sys.c	2004-04-30 16:07:32.000000000 +0530
@@ -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 */
diff -puN drivers/base/bus.c~kobject_set_name-cleanup-01 drivers/base/bus.c
--- linux-2.6.6-rc3-mm1/drivers/base/bus.c~kobject_set_name-cleanup-01	2004-04-30 16:07:32.000000000 +0530
+++ linux-2.6.6-rc3-mm1-maneesh/drivers/base/bus.c	2004-05-04 11:01:52.000000000 +0530
@@ -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;
 }
 
 

_





-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

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

[Maneesh Soni]

On Tue, May 04, 2004 at 11:09:08AM +0530, Maneesh Soni wrote:
> On Fri, Apr 30, 2004 at 07:48:13AM -0500, Dmitry Torokhov wrote:
> > 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(-)

diff -puN drivers/base/sys.c~kobject_set_name-cleanup-01 drivers/base/sys.c
--- linux-2.6.6-rc3-mm1/drivers/base/sys.c~kobject_set_name-cleanup-01	2004-04-30 16:07:32.000000000 +0530
+++ linux-2.6.6-rc3-mm1-maneesh/drivers/base/sys.c	2004-04-30 16:07:32.000000000 +0530
@@ -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 */
diff -puN drivers/base/bus.c~kobject_set_name-cleanup-01 drivers/base/bus.c
--- linux-2.6.6-rc3-mm1/drivers/base/bus.c~kobject_set_name-cleanup-01	2004-04-30 16:07:32.000000000 +0530
+++ linux-2.6.6-rc3-mm1-maneesh/drivers/base/bus.c	2004-05-04 11:01:52.000000000 +0530
@@ -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;
 }
 
 

_

-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

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

[Greg KH]

On Tue, May 04, 2004 at 11:09:08AM +0530, Maneesh Soni wrote:
> 
> 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

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

[Maneesh Soni]

On Fri, May 07, 2004 at 03:25:49PM -0700, Greg KH wrote:
> On Tue, May 04, 2004 at 11:09:08AM +0530, Maneesh Soni wrote:
> > 
> > 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(-)

diff -puN drivers/base/sys.c~kobject_set_name-cleanup-01 drivers/base/sys.c
--- linux-2.6.6-rc3-mm2/drivers/base/sys.c~kobject_set_name-cleanup-01	2004-05-06 12:13:22.000000000 +0530
+++ linux-2.6.6-rc3-mm2-maneesh/drivers/base/sys.c	2004-05-06 12:13:22.000000000 +0530
@@ -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 */
diff -puN drivers/base/bus.c~kobject_set_name-cleanup-01 drivers/base/bus.c
--- linux-2.6.6-rc3-mm2/drivers/base/bus.c~kobject_set_name-cleanup-01	2004-05-06 12:13:22.000000000 +0530
+++ linux-2.6.6-rc3-mm2-maneesh/drivers/base/bus.c	2004-05-06 12:13:42.000000000 +0530
@@ -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;
 }
 
 

_


-- 
Maneesh Soni
IBM Linux Technology Center, 
IBM India Software Lab, Bangalore.
Phone: +91-80-5044999 email: maneesh@in.ibm.com
http://lse.sourceforge.net/

[RFC 2/2] sysfs_rename_dir-cleanup

[Maneesh Soni]

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)


 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(-)

diff -puN fs/sysfs/dir.c~sysfs_rename_dir-cleanup fs/sysfs/dir.c
--- linux-2.6.6-rc3-mm2/fs/sysfs/dir.c~sysfs_rename_dir-cleanup	2004-05-05 18:22:39.000000000 +0530
+++ linux-2.6.6-rc3-mm2-maneesh/fs/sysfs/dir.c	2004-05-05 18:33:54.000000000 +0530
@@ -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);
diff -puN include/linux/sysfs.h~sysfs_rename_dir-cleanup include/linux/sysfs.h
--- linux-2.6.6-rc3-mm2/include/linux/sysfs.h~sysfs_rename_dir-cleanup	2004-05-05 18:22:39.000000000 +0530
+++ linux-2.6.6-rc3-mm2-maneesh/include/linux/sysfs.h	2004-05-05 18:33:58.000000000 +0530
@@ -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
diff -puN lib/kobject.c~sysfs_rename_dir-cleanup lib/kobject.c
--- linux-2.6.6-rc3-mm2/lib/kobject.c~sysfs_rename_dir-cleanup	2004-05-05 18:22:39.000000000 +0530
+++ linux-2.6.6-rc3-mm2-maneesh/lib/kobject.c	2004-05-05 18:22:39.000000000 +0530
@@ -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;
 }
 
 /**
diff -puN include/linux/kobject.h~sysfs_rename_dir-cleanup include/linux/kobject.h
--- linux-2.6.6-rc3-mm2/include/linux/kobject.h~sysfs_rename_dir-cleanup	2004-05-05 18:22:39.000000000 +0530
+++ linux-2.6.6-rc3-mm2-maneesh/include/linux/kobject.h	2004-05-05 18:22:39.000000000 +0530
@@ -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 *);
diff -puN drivers/base/class.c~sysfs_rename_dir-cleanup drivers/base/class.c
--- linux-2.6.6-rc3-mm2/drivers/base/class.c~sysfs_rename_dir-cleanup	2004-05-05 18:22:39.000000000 +0530
+++ linux-2.6.6-rc3-mm2-maneesh/drivers/base/class.c	2004-05-05 18:22:39.000000000 +0530
@@ -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)
diff -puN net/core/dev.c~sysfs_rename_dir-cleanup net/core/dev.c
--- linux-2.6.6-rc3-mm2/net/core/dev.c~sysfs_rename_dir-cleanup	2004-05-05 18:22:39.000000000 +0530
+++ linux-2.6.6-rc3-mm2-maneesh/net/core/dev.c	2004-05-05 18:22:39.000000000 +0530
@@ -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;
 }
 
 /**

_
-- 
Maneesh Soni
IBM Linux Technology Center, 
IBM India Software Lab, Bangalore.
Phone: +91-80-5044999 email: maneesh@in.ibm.com
http://lse.sourceforge.net/

Re: [RFC 2/2] sysfs_rename_dir-cleanup

[Greg KH]

On Fri, May 09, 2003 at 03:39:57PM +0530, Maneesh Soni wrote:
> diff -puN fs/sysfs/dir.c~sysfs_rename_dir-cleanup fs/sysfs/dir.c
> --- linux-2.6.6-rc3-mm2/fs/sysfs/dir.c~sysfs_rename_dir-cleanup	2004-05-05 18:22:39.000000000 +0530
> +++ linux-2.6.6-rc3-mm2-maneesh/fs/sysfs/dir.c	2004-05-05 18:33:54.000000000 +0530
> @@ -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);

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

Re: [RFC 2/2] sysfs_rename_dir-cleanup

[Maneesh Soni]

On Tue, May 11, 2004 at 04:33:51PM -0700, Greg KH wrote:
[snip]
> 
> 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.

> 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

 


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)


 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(-)

diff -puN fs/sysfs/dir.c~sysfs_rename_dir-cleanup fs/sysfs/dir.c
--- linux-2.6.6/fs/sysfs/dir.c~sysfs_rename_dir-cleanup	2004-10-07 10:33:47.000000000 +0530
+++ linux-2.6.6-maneesh/fs/sysfs/dir.c	2004-10-07 10:38:09.000000000 +0530
@@ -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);
diff -puN include/linux/kobject.h~sysfs_rename_dir-cleanup include/linux/kobject.h
--- linux-2.6.6/include/linux/kobject.h~sysfs_rename_dir-cleanup	2004-10-07 10:33:55.000000000 +0530
+++ linux-2.6.6-maneesh/include/linux/kobject.h	2004-10-07 10:35:11.000000000 +0530
@@ -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 *);
diff -puN drivers/base/class.c~sysfs_rename_dir-cleanup drivers/base/class.c
--- linux-2.6.6/drivers/base/class.c~sysfs_rename_dir-cleanup	2004-10-07 10:34:02.000000000 +0530
+++ linux-2.6.6-maneesh/drivers/base/class.c	2004-10-07 10:35:11.000000000 +0530
@@ -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)
diff -puN lib/kobject.c~sysfs_rename_dir-cleanup lib/kobject.c
--- linux-2.6.6/lib/kobject.c~sysfs_rename_dir-cleanup	2004-10-07 10:34:17.000000000 +0530
+++ linux-2.6.6-maneesh/lib/kobject.c	2004-10-07 10:35:11.000000000 +0530
@@ -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;
 }
 
 /**
diff -puN include/linux/sysfs.h~sysfs_rename_dir-cleanup include/linux/sysfs.h
--- linux-2.6.6/include/linux/sysfs.h~sysfs_rename_dir-cleanup	2004-10-07 10:34:25.000000000 +0530
+++ linux-2.6.6-maneesh/include/linux/sysfs.h	2004-10-07 10:35:11.000000000 +0530
@@ -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
diff -puN net/core/dev.c~sysfs_rename_dir-cleanup net/core/dev.c
--- linux-2.6.6/net/core/dev.c~sysfs_rename_dir-cleanup	2004-10-07 10:34:43.000000000 +0530
+++ linux-2.6.6-maneesh/net/core/dev.c	2004-10-07 10:35:11.000000000 +0530
@@ -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;
 }
 
 /**

_

-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

Re: [RFC 2/2] sysfs_rename_dir-cleanup

[Maneesh Soni]

On Thu, Oct 07, 2004 at 10:46:23AM +0530, Maneesh Soni wrote:
> On Tue, May 11, 2004 at 04:33:51PM -0700, Greg KH wrote:
> [snip]
> > 
> > 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.
> 
> > 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.
> 
> 

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.




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)


 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(-)

diff -puN fs/sysfs/dir.c~sysfs_rename_dir-cleanup fs/sysfs/dir.c
--- linux-2.6.6/fs/sysfs/dir.c~sysfs_rename_dir-cleanup	2004-10-07 10:33:47.000000000 +0530
+++ linux-2.6.6-maneesh/fs/sysfs/dir.c	2004-10-07 10:47:22.000000000 +0530
@@ -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);
diff -puN include/linux/kobject.h~sysfs_rename_dir-cleanup include/linux/kobject.h
--- linux-2.6.6/include/linux/kobject.h~sysfs_rename_dir-cleanup	2004-10-07 10:33:55.000000000 +0530
+++ linux-2.6.6-maneesh/include/linux/kobject.h	2004-10-07 10:35:11.000000000 +0530
@@ -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 *);
diff -puN drivers/base/class.c~sysfs_rename_dir-cleanup drivers/base/class.c
--- linux-2.6.6/drivers/base/class.c~sysfs_rename_dir-cleanup	2004-10-07 10:34:02.000000000 +0530
+++ linux-2.6.6-maneesh/drivers/base/class.c	2004-10-07 10:35:11.000000000 +0530
@@ -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)
diff -puN lib/kobject.c~sysfs_rename_dir-cleanup lib/kobject.c
--- linux-2.6.6/lib/kobject.c~sysfs_rename_dir-cleanup	2004-10-07 10:34:17.000000000 +0530
+++ linux-2.6.6-maneesh/lib/kobject.c	2004-10-07 10:35:11.000000000 +0530
@@ -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;
 }
 
 /**
diff -puN include/linux/sysfs.h~sysfs_rename_dir-cleanup include/linux/sysfs.h
--- linux-2.6.6/include/linux/sysfs.h~sysfs_rename_dir-cleanup	2004-10-07 10:34:25.000000000 +0530
+++ linux-2.6.6-maneesh/include/linux/sysfs.h	2004-10-07 10:35:11.000000000 +0530
@@ -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
diff -puN net/core/dev.c~sysfs_rename_dir-cleanup net/core/dev.c
--- linux-2.6.6/net/core/dev.c~sysfs_rename_dir-cleanup	2004-10-07 10:34:43.000000000 +0530
+++ linux-2.6.6-maneesh/net/core/dev.c	2004-10-07 10:35:11.000000000 +0530
@@ -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;
 }
 
 /**

_

-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

Re: [RFC 2/2] sysfs_rename_dir-cleanup

[Greg KH]

On Thu, Oct 07, 2004 at 11:08:45AM +0530, Maneesh Soni wrote:
> 
> 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

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

[Greg KH]

On Fri, May 09, 2003 at 03:35:23PM +0530, Maneesh Soni wrote:
> On Fri, May 07, 2004 at 03:25:49PM -0700, Greg KH wrote:
> > On Tue, May 04, 2004 at 11:09:08AM +0530, Maneesh Soni wrote:
> > > 
> > > 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()

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

Re: [RFC] fix sysfs symlinks

[Greg KH]

On Sat, Apr 17, 2004 at 12:46:02AM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Fri, Apr 16, 2004 at 03:37:32PM -0700, Greg KH wrote:
> 
> > 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