Hi,
I spent some time tonight trying to track down how to fix the issue of
duplicate sysfs files and/or directories. This happens when you try to
create a kobject with the same name in the same directory. The creation
of the second kobject will fail, but the directory will remain in sysfs.
Now I know this isn't a normal operation, but it would be good to fix
this eventually. I traced the issue down to fs/sysfs/dir.c:create_dir()
and the check for:
if (error && (error != -EEXIST)) {
Problem is, error is set to -EEXIST, so we don't clean up properly. Now
I know we can't just not check for this, as if you do that error
cleanup, the original kobject's sysfs entry gets very messed up (ls -l
does not like it at all...)
But I can't seem to figure out what exactly we need to do to clean up
properly here.
Do you, or anyone else, have any pointers or ideas?
thanks,
greg k-h
Greg KH <[email protected]> wrote:
>
> Hi,
>
> I spent some time tonight trying to track down how to fix the issue of
> duplicate sysfs files and/or directories. This happens when you try to
> create a kobject with the same name in the same directory. The creation
> of the second kobject will fail, but the directory will remain in sysfs.
>
> Now I know this isn't a normal operation, but it would be good to fix
> this eventually. I traced the issue down to fs/sysfs/dir.c:create_dir()
> and the check for:
> if (error && (error != -EEXIST)) {
>
> Problem is, error is set to -EEXIST, so we don't clean up properly. Now
> I know we can't just not check for this, as if you do that error
> cleanup, the original kobject's sysfs entry gets very messed up (ls -l
> does not like it at all...)
>
> But I can't seem to figure out what exactly we need to do to clean up
> properly here.
>
> Do you, or anyone else, have any pointers or ideas?
>
Emit a loud warning and don't bother cleaning up - leave the current
behaviour as-is. Whatever takes the least amount code and has the minimum
end-user impact, IMO.
On Wed, Mar 08, 2006 at 01:02:05AM -0800, Andrew Morton wrote:
> Greg KH <[email protected]> wrote:
> >
> > Hi,
> >
> > I spent some time tonight trying to track down how to fix the issue of
> > duplicate sysfs files and/or directories. This happens when you try to
> > create a kobject with the same name in the same directory. The creation
> > of the second kobject will fail, but the directory will remain in sysfs.
> >
> > Now I know this isn't a normal operation, but it would be good to fix
> > this eventually. I traced the issue down to fs/sysfs/dir.c:create_dir()
> > and the check for:
> > if (error && (error != -EEXIST)) {
> >
> > Problem is, error is set to -EEXIST, so we don't clean up properly. Now
> > I know we can't just not check for this, as if you do that error
> > cleanup, the original kobject's sysfs entry gets very messed up (ls -l
> > does not like it at all...)
> >
> > But I can't seem to figure out what exactly we need to do to clean up
> > properly here.
> >
> > Do you, or anyone else, have any pointers or ideas?
> >
>
> Emit a loud warning and don't bother cleaning up - leave the current
> behaviour as-is. Whatever takes the least amount code and has the minimum
> end-user impact, IMO.
Yeah, we do give a warning. Well, we used to always do it, but now that
more people are not using kobject_register() but only kobject_add(), it
doesn't always show up...
Here's a patch that I've added to my queue that fixes the warn issue,
but it still would be nice to fix up sysfs someday.
thanks,
greg k-h
---
lib/kobject.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
--- gregkh-2.6.orig/lib/kobject.c
+++ gregkh-2.6/lib/kobject.c
@@ -194,6 +194,17 @@ int kobject_add(struct kobject * kobj)
unlink(kobj);
if (parent)
kobject_put(parent);
+
+ /* be noisy on error issues */
+ if (error == -EEXIST)
+ printk("kobject_add failed for %s with -EEXIST, "
+ "don't try to register things with the "
+ "same name in the same directory.\n",
+ kobject_name(kobj));
+ else
+ printk("kobject_add failed for %s (%d)\n",
+ kobject_name(kobj), error);
+ dump_stack();
}
return error;
@@ -207,18 +218,13 @@ int kobject_add(struct kobject * kobj)
int kobject_register(struct kobject * kobj)
{
- int error = 0;
+ int error = -EINVAL;
if (kobj) {
kobject_init(kobj);
error = kobject_add(kobj);
- if (error) {
- printk("kobject_register failed for %s (%d)\n",
- kobject_name(kobj),error);
- dump_stack();
- } else
+ if (!error)
kobject_uevent(kobj, KOBJ_ADD);
- } else
- error = -EINVAL;
+ }
return error;
}
On Tue, Mar 07, 2006 at 11:53:42PM -0800, Greg KH wrote:
> Hi,
>
> I spent some time tonight trying to track down how to fix the issue of
> duplicate sysfs files and/or directories. This happens when you try to
> create a kobject with the same name in the same directory. The creation
> of the second kobject will fail, but the directory will remain in sysfs.
>
Let me understand this. Lets say we have sysfs directory tree like
/sys/a/b/c
and someone is trying to create one more kobject with name "c" for the
parent kobject "b" ?
And are you saying that though the new creation fails but the existing
directory remains in sysfs? I think failing the new creation and leaving
the exisiting directoy is ok. But there is sysfs_dirent leakage which
does need fixing.
> Now I know this isn't a normal operation, but it would be good to fix
> this eventually. I traced the issue down to fs/sysfs/dir.c:create_dir()
> and the check for:
> if (error && (error != -EEXIST)) {
>
> Problem is, error is set to -EEXIST, so we don't clean up properly. Now
> I know we can't just not check for this, as if you do that error
> cleanup, the original kobject's sysfs entry gets very messed up (ls -l
> does not like it at all...)
>
> But I can't seem to figure out what exactly we need to do to clean up
> properly here.
>
> Do you, or anyone else, have any pointers or ideas?
>
If you are talking about the example above, to me it appears that except
a possible sysfs_dirent leakage, we are some what ok, else there would have
been more catastrophic results because of the duplicate directory dentry/inode.
As per the current code
static int create_dir(struct kobject * k, struct dentry * p,
const char * n, struct dentry ** d)
{
int error;
umode_t mode = S_IFDIR| S_IRWXU | S_IRUGO | S_IXUGO;
mutex_lock(&p->d_inode->i_mutex);
*d = lookup_one_len(n, p, strlen(n));
^^^^ lookup_one_len() will return the existing dentry corresponding to
the last component "c" in "/sys/a/b/c" without any error. Just note
that VFS is not going to allocate a new dentry for it. The existing
dentry's ref count will be increased by one.
if (!IS_ERR(*d)) {
error = sysfs_make_dirent(p->d_fsdata, *d, k, mode, SYSFS_DIR);
^^^^ we do have problem here, a new sysfs_dirent is allocated which
replaces the existing dentry->d_fsdata and yuk... the old sysfs_dirent
is no more linked with the existing directory, there by leaking
one sysfs_dirent.
Not only for sysfs_create_dir(), I think the problem of existing sysfs_dirent
is also there with sysfs_add_file() and sysfs_add_link(). I am working on a
patch to plug this leak.
Thanks
Maneesh
--
Maneesh Soni
Linux Technology Center,
IBM India Software Labs,
Bangalore, India
email: [email protected]
Phone: 91-80-51776416
o Following patch checks for existing sysfs_dirent before allocation new one.
Signed-off-by: Maneesh Soni <[email protected]>
---
linux-2.6.16-rc5-git10-maneesh/fs/sysfs/dir.c | 17 +++++++++++++++++
1 files changed, 17 insertions(+)
diff -puN fs/sysfs/dir.c~sysfs-check-existing-dirent fs/sysfs/dir.c
--- linux-2.6.16-rc5-git10/fs/sysfs/dir.c~sysfs-check-existing-dirent 2006-03-08 17:50:00.857712216 +0530
+++ linux-2.6.16-rc5-git10-maneesh/fs/sysfs/dir.c 2006-03-08 17:50:00.864711152 +0530
@@ -50,11 +50,28 @@ static struct sysfs_dirent * sysfs_new_d
return sd;
}
+/**
+ * Initialise a newly allocated sysfs_dirent and attach it to
+ * the corresponding dentry if present.
+ *
+ * Return -EEXIST if there is already a sysfs element with the same name for
+ * the same parent.
+ *
+ * called with parent inode's i_mutex held
+ */
int sysfs_make_dirent(struct sysfs_dirent * parent_sd, struct dentry * dentry,
void * element, umode_t mode, int type)
{
struct sysfs_dirent * sd;
+ list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
+ const unsigned char * existing = sysfs_get_name(sd);
+ if (strcmp(existing, new))
+ continue;
+ else
+ return -EEXIST;
+ }
+
sd = sysfs_new_dirent(parent_sd, element);
if (!sd)
return -ENOMEM;
_
On Thu, Mar 09, 2006 at 08:28:24AM +0530, Maneesh Soni wrote:
> On Tue, Mar 07, 2006 at 11:53:42PM -0800, Greg KH wrote:
> > Hi,
> >
> > I spent some time tonight trying to track down how to fix the issue of
> > duplicate sysfs files and/or directories. This happens when you try to
> > create a kobject with the same name in the same directory. The creation
> > of the second kobject will fail, but the directory will remain in sysfs.
> >
>
> Let me understand this. Lets say we have sysfs directory tree like
> /sys/a/b/c
> and someone is trying to create one more kobject with name "c" for the
> parent kobject "b" ?
Yes.
> And are you saying that though the new creation fails but the existing
> directory remains in sysfs?
No. The new creation fails, but we end up with two "c" directories
under "b".
> I think failing the new creation and leaving the exisiting directoy is
> ok.
I agree, but that's not what happens :)
> But there is sysfs_dirent leakage which does need fixing.
Yes.
> > Now I know this isn't a normal operation, but it would be good to fix
> > this eventually. I traced the issue down to fs/sysfs/dir.c:create_dir()
> > and the check for:
> > if (error && (error != -EEXIST)) {
> >
> > Problem is, error is set to -EEXIST, so we don't clean up properly. Now
> > I know we can't just not check for this, as if you do that error
> > cleanup, the original kobject's sysfs entry gets very messed up (ls -l
> > does not like it at all...)
> >
> > But I can't seem to figure out what exactly we need to do to clean up
> > properly here.
> >
> > Do you, or anyone else, have any pointers or ideas?
> >
>
> If you are talking about the example above, to me it appears that except
> a possible sysfs_dirent leakage, we are some what ok, else there would have
> been more catastrophic results because of the duplicate directory dentry/inode.
>
> As per the current code
>
> static int create_dir(struct kobject * k, struct dentry * p,
> const char * n, struct dentry ** d)
> {
> int error;
> umode_t mode = S_IFDIR| S_IRWXU | S_IRUGO | S_IXUGO;
>
> mutex_lock(&p->d_inode->i_mutex);
>
> *d = lookup_one_len(n, p, strlen(n));
>
> ^^^^ lookup_one_len() will return the existing dentry corresponding to
> the last component "c" in "/sys/a/b/c" without any error. Just note
> that VFS is not going to allocate a new dentry for it. The existing
> dentry's ref count will be increased by one.
>
> if (!IS_ERR(*d)) {
> error = sysfs_make_dirent(p->d_fsdata, *d, k, mode, SYSFS_DIR);
>
> ^^^^ we do have problem here, a new sysfs_dirent is allocated which
> replaces the existing dentry->d_fsdata and yuk... the old sysfs_dirent
> is no more linked with the existing directory, there by leaking
> one sysfs_dirent.
Yeah, I just couldn't figure out how to clean it up properly :)
> Not only for sysfs_create_dir(), I think the problem of existing sysfs_dirent
> is also there with sysfs_add_file() and sysfs_add_link(). I am working on a
> patch to plug this leak.
That's great, thanks.
I have a test module that shows this at:
http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/gregkh/sysfs-test.patch
just load the 'gregkh' module and look in /sys/class/gregkh/ to see the
duplicate "gregkh1" directories.
thanks,
greg k-h
On Wed, Mar 08, 2006 at 09:58:16PM -0800, Greg KH wrote:
>
> I have a test module that shows this at:
> http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/gregkh/sysfs-test.patch
> just load the 'gregkh' module and look in /sys/class/gregkh/ to see the
> duplicate "gregkh1" directories.
>
Thnaks for the testcase, I could verify the following patch using this. I
don't see the duplicate "gregkh1" directories. Please queue it for post 2.6.16
if you are ok with it.
Thanks
Maneesh
o The following patch checks for existing sysfs_dirent before
preparing new one while creating sysfs directories and files.
Signed-off-by: Maneesh Soni <[email protected]>
---
linux-2.6.16-rc5-git12-maneesh/fs/sysfs/dir.c | 30 +++++++++++++++++++++-
linux-2.6.16-rc5-git12-maneesh/fs/sysfs/file.c | 4 ++
linux-2.6.16-rc5-git12-maneesh/fs/sysfs/symlink.c | 3 +-
linux-2.6.16-rc5-git12-maneesh/fs/sysfs/sysfs.h | 1
4 files changed, 35 insertions(+), 3 deletions(-)
diff -puN fs/sysfs/dir.c~sysfs-check-existing-sysfs_dirent fs/sysfs/dir.c
--- linux-2.6.16-rc5-git12/fs/sysfs/dir.c~sysfs-check-existing-sysfs_dirent 2006-03-09 19:19:22.655241008 +0530
+++ linux-2.6.16-rc5-git12-maneesh/fs/sysfs/dir.c 2006-03-09 19:38:52.735361792 +0530
@@ -50,6 +50,30 @@ static struct sysfs_dirent * sysfs_new_d
return sd;
}
+/**
+ *
+ * Return -EEXIST if there is already a sysfs element with the same name for
+ * the same parent.
+ *
+ * called with parent inode's i_mutex held
+ */
+int is_sysfs_dirent_exist(struct sysfs_dirent * parent_sd,
+ const unsigned char * new)
+{
+ struct sysfs_dirent * sd;
+
+ list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
+ const unsigned char * existing = sysfs_get_name(sd);
+ if (strcmp(existing, new))
+ continue;
+ else
+ return -EEXIST;
+ }
+
+ return 0;
+}
+
+
int sysfs_make_dirent(struct sysfs_dirent * parent_sd, struct dentry * dentry,
void * element, umode_t mode, int type)
{
@@ -102,7 +126,11 @@ static int create_dir(struct kobject * k
mutex_lock(&p->d_inode->i_mutex);
*d = lookup_one_len(n, p, strlen(n));
if (!IS_ERR(*d)) {
- error = sysfs_make_dirent(p->d_fsdata, *d, k, mode, SYSFS_DIR);
+ if (is_sysfs_dirent_exist(p->d_fsdata, n))
+ error = -EEXIST;
+ else
+ error = sysfs_make_dirent(p->d_fsdata, *d, k, mode,
+ SYSFS_DIR);
if (!error) {
error = sysfs_create(*d, mode, init_dir);
if (!error) {
diff -puN fs/sysfs/file.c~sysfs-check-existing-sysfs_dirent fs/sysfs/file.c
--- linux-2.6.16-rc5-git12/fs/sysfs/file.c~sysfs-check-existing-sysfs_dirent 2006-03-09 19:19:22.659240400 +0530
+++ linux-2.6.16-rc5-git12-maneesh/fs/sysfs/file.c 2006-03-09 19:19:22.676237816 +0530
@@ -365,7 +365,9 @@ int sysfs_add_file(struct dentry * dir,
int error = 0;
mutex_lock(&dir->d_inode->i_mutex);
- error = sysfs_make_dirent(parent_sd, NULL, (void *) attr, mode, type);
+ if (!is_sysfs_dirent_exist(parent_sd, attr->name))
+ error = sysfs_make_dirent(parent_sd, NULL, (void *) attr,
+ mode, type);
mutex_unlock(&dir->d_inode->i_mutex);
return error;
diff -puN fs/sysfs/symlink.c~sysfs-check-existing-sysfs_dirent fs/sysfs/symlink.c
--- linux-2.6.16-rc5-git12/fs/sysfs/symlink.c~sysfs-check-existing-sysfs_dirent 2006-03-09 19:19:22.663239792 +0530
+++ linux-2.6.16-rc5-git12-maneesh/fs/sysfs/symlink.c 2006-03-09 19:19:22.677237664 +0530
@@ -87,7 +87,8 @@ int sysfs_create_link(struct kobject * k
BUG_ON(!kobj || !kobj->dentry || !name);
mutex_lock(&dentry->d_inode->i_mutex);
- error = sysfs_add_link(dentry, name, target);
+ if (!is_sysfs_dirent_exist(dentry->d_fsdata, name))
+ error = sysfs_add_link(dentry, name, target);
mutex_unlock(&dentry->d_inode->i_mutex);
return error;
}
diff -puN fs/sysfs/sysfs.h~sysfs-check-existing-sysfs_dirent fs/sysfs/sysfs.h
--- linux-2.6.16-rc5-git12/fs/sysfs/sysfs.h~sysfs-check-existing-sysfs_dirent 2006-03-09 19:19:22.666239336 +0530
+++ linux-2.6.16-rc5-git12-maneesh/fs/sysfs/sysfs.h 2006-03-09 19:28:52.404625936 +0530
@@ -5,6 +5,7 @@ extern kmem_cache_t *sysfs_dir_cachep;
extern struct inode * sysfs_new_inode(mode_t mode, struct sysfs_dirent *);
extern int sysfs_create(struct dentry *, int mode, int (*init)(struct inode *));
+extern int is_sysfs_dirent_exist(struct sysfs_dirent *, const unsigned char * );
extern int sysfs_make_dirent(struct sysfs_dirent *, struct dentry *, void *,
umode_t, int);
_
--
Maneesh Soni
Linux Technology Center,
IBM India Software Labs,
Bangalore, India
email: [email protected]
Phone: 91-80-51776416
On Thu, Mar 09, 2006 at 07:40:14PM +0530, Maneesh Soni wrote:
> On Wed, Mar 08, 2006 at 09:58:16PM -0800, Greg KH wrote:
>
> >
> > I have a test module that shows this at:
> > http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/gregkh/sysfs-test.patch
> > just load the 'gregkh' module and look in /sys/class/gregkh/ to see the
> > duplicate "gregkh1" directories.
> >
>
> Thnaks for the testcase, I could verify the following patch using this. I
> don't see the duplicate "gregkh1" directories. Please queue it for post 2.6.16
> if you are ok with it.
Looks good. I tweaked it a bit (returned -EEXIST from functions if the
file already existed), and renamed the function to play nicer in the
global namespace. Updated patch is below
Thanks a lot for doing this, I appreciate it.
greg k-h
-------------
>From [email protected] Thu Mar 9 06:13:04 2006
Date: Thu, 9 Mar 2006 19:40:14 +0530
From: Maneesh Soni <[email protected]>
To: Greg KH <[email protected]>
Cc: Patrick Mochel <[email protected]>
Subject: sysfs: fix problem with duplicate sysfs directories and files
Message-ID: <[email protected]>
Content-Disposition: inline
The following patch checks for existing sysfs_dirent before
preparing new one while creating sysfs directories and files.
Signed-off-by: Maneesh Soni <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
fs/sysfs/dir.c | 30 +++++++++++++++++++++++++++++-
fs/sysfs/file.c | 6 ++++--
fs/sysfs/symlink.c | 5 +++--
fs/sysfs/sysfs.h | 1 +
4 files changed, 37 insertions(+), 5 deletions(-)
--- gregkh-2.6.orig/fs/sysfs/dir.c
+++ gregkh-2.6/fs/sysfs/dir.c
@@ -51,6 +51,30 @@ static struct sysfs_dirent * sysfs_new_d
return sd;
}
+/**
+ *
+ * Return -EEXIST if there is already a sysfs element with the same name for
+ * the same parent.
+ *
+ * called with parent inode's i_mutex held
+ */
+int sysfs_dirent_exist(struct sysfs_dirent *parent_sd,
+ const unsigned char *new)
+{
+ struct sysfs_dirent * sd;
+
+ list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
+ const unsigned char * existing = sysfs_get_name(sd);
+ if (strcmp(existing, new))
+ continue;
+ else
+ return -EEXIST;
+ }
+
+ return 0;
+}
+
+
int sysfs_make_dirent(struct sysfs_dirent * parent_sd, struct dentry * dentry,
void * element, umode_t mode, int type)
{
@@ -117,7 +141,11 @@ static int create_dir(struct kobject * k
mutex_lock(&p->d_inode->i_mutex);
*d = lookup_one_len(n, p, strlen(n));
if (!IS_ERR(*d)) {
- error = sysfs_make_dirent(p->d_fsdata, *d, k, mode, SYSFS_DIR);
+ if (sysfs_dirent_exist(p->d_fsdata, n))
+ error = -EEXIST;
+ else
+ error = sysfs_make_dirent(p->d_fsdata, *d, k, mode,
+ SYSFS_DIR);
if (!error) {
error = sysfs_create(*d, mode, init_dir);
if (!error) {
--- gregkh-2.6.orig/fs/sysfs/file.c
+++ gregkh-2.6/fs/sysfs/file.c
@@ -450,10 +450,12 @@ int sysfs_add_file(struct dentry * dir,
{
struct sysfs_dirent * parent_sd = dir->d_fsdata;
umode_t mode = (attr->mode & S_IALLUGO) | S_IFREG;
- int error = 0;
+ int error = -EEXIST;
mutex_lock(&dir->d_inode->i_mutex);
- error = sysfs_make_dirent(parent_sd, NULL, (void *) attr, mode, type);
+ if (!sysfs_dirent_exist(parent_sd, attr->name))
+ error = sysfs_make_dirent(parent_sd, NULL, (void *)attr,
+ mode, type);
mutex_unlock(&dir->d_inode->i_mutex);
return error;
--- gregkh-2.6.orig/fs/sysfs/symlink.c
+++ gregkh-2.6/fs/sysfs/symlink.c
@@ -82,12 +82,13 @@ exit1:
int sysfs_create_link(struct kobject * kobj, struct kobject * target, const char * name)
{
struct dentry * dentry = kobj->dentry;
- int error = 0;
+ int error = -EEXIST;
BUG_ON(!kobj || !kobj->dentry || !name);
mutex_lock(&dentry->d_inode->i_mutex);
- error = sysfs_add_link(dentry, name, target);
+ if (!sysfs_dirent_exist(dentry->d_fsdata, name))
+ error = sysfs_add_link(dentry, name, target);
mutex_unlock(&dentry->d_inode->i_mutex);
return error;
}
--- gregkh-2.6.orig/fs/sysfs/sysfs.h
+++ gregkh-2.6/fs/sysfs/sysfs.h
@@ -5,6 +5,7 @@ extern kmem_cache_t *sysfs_dir_cachep;
extern struct inode * sysfs_new_inode(mode_t mode, struct sysfs_dirent *);
extern int sysfs_create(struct dentry *, int mode, int (*init)(struct inode *));
+extern int sysfs_dirent_exist(struct sysfs_dirent *, const unsigned char *);
extern int sysfs_make_dirent(struct sysfs_dirent *, struct dentry *, void *,
umode_t, int);