LinuxLists.cc - [RFC] fix sysfs symlinks

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

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

2004-04-14 06:36:01

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

On Tue, Apr 13, 2004 at 02:36:15PM +0100, [email protected] 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: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

2004-04-14 07:02:30

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

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.

2004-04-14 07:13:53

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

On Wed, Apr 14, 2004 at 08:02:27AM +0100, [email protected] 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: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

2004-04-14 07:27:14

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

On Wed, Apr 14, 2004 at 12:47:56PM +0530, Maneesh Soni wrote:
> On Wed, Apr 14, 2004 at 08:02:27AM +0100, [email protected] 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...

2004-04-15 08:17:58

by Russell King

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

2004-04-15 10:38:56

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

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

2004-04-15 15:19:52

by Russell King

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

On Thu, Apr 15, 2004 at 11:38:49AM +0100, [email protected] 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

2004-04-15 16:13:37

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

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

2004-04-15 16:10:45

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

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

2004-04-15 19:14:52

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

On Thu, Apr 15, 2004 at 05:13:32PM +0100, [email protected] 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?

2004-04-15 21:28:14

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

On Thu, Apr 15, 2004 at 08:14:47PM +0100, [email protected] wrote:
>
> BTW, how about a new section that would
> a) be allocated separately at module load time
> b) contain a kobject with ->release() freeing that section
> c) be populated with structures containing kobjects and having
> no ->release(); main kobject would be pinned down by them. Original
> refcount in each of those guys would be 1.
>
> 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

2004-04-15 22:19:50

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

2004-04-16 15:28:36

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

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.

2004-04-16 18:04:03

by Horst H. von Brand

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

[email protected] 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

2004-04-16 18:11:32

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

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.

2004-04-16 23:43:52

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

On Fri, Apr 16, 2004 at 04:24:48PM +0100, [email protected] 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

2004-04-16 23:47:18

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

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?

2004-04-17 00:04:10

by Jeff Garzik

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

[email protected] 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

2004-04-17 00:17:58

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

On Sat, Apr 17, 2004 at 12:46:02AM +0100, [email protected] 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

2004-04-17 06:15:50

by Rusty Russell

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

On Fri, 2004-04-16 at 05:14, [email protected]
wrote:
> BTW, how about a new section that would
> a) be allocated separately at module load time
> b) contain a kobject with ->release() freeing that section
> c) be populated with structures containing kobjects and having
> no ->release(); main kobject would be pinned down by them. Original
> refcount in each of those guys would be 1.
>
> 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

2004-04-17 08:07:36

by Russell King

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

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

2004-04-17 08:22:17

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

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?

2004-04-17 19:39:45

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

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.

2004-04-17 23:45:34

by Rusty Russell

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

On Sun, 2004-04-18 at 05:39, [email protected]
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

2004-04-20 16:23:52

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

On Sat, Apr 17, 2004 at 09:22:06AM +0100, [email protected] 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

2004-04-21 10:09:52

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

On Tue, Apr 20, 2004 at 09:16:02AM -0700, Greg KH wrote:
> On Sat, Apr 17, 2004 at 09:22:06AM +0100, [email protected] 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: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

2004-04-22 21:38:59

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

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

2004-04-23 08:49:24

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

On Thu, Apr 22, 2004 at 10:37:37PM +0100, [email protected] 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: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

2004-04-23 09:26:53

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

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?

2004-04-29 13:03:24

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

On Fri, Apr 23, 2004 at 10:26:41AM +0100, [email protected] 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: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

2004-04-29 15:41:09

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

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.

2004-04-30 10:03:23

[permalink] [raw]

Subject: Re: [RFC] fix sysfs symlinks

On Thu, Apr 29, 2004 at 04:41:04PM +0100, [email protected] 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: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

2004-04-30 10:10:52

[permalink] [raw]

Subject: [RFC 0/2] kobject_set_name - error handling

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

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

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

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

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

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

Please comment.

Thanks,
Maneesh

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

2004-04-30 10:11:24

[permalink] [raw]

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

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

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

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: [email protected]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696

2004-04-30 10:14:52