lkml.org 
[lkml]   [2006]   [Mar]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: problem with duplicate sysfs directories and files
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: maneesh@in.ibm.com
Phone: 91-80-51776416





o Following patch checks for existing sysfs_dirent before allocation new one.

Signed-off-by: Maneesh Soni <maneesh@in.ibm.com>
---

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;
_
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
\
 
 \ /
  Last update: 2006-03-09 04:03    [W:0.092 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site