2003-09-11 12:00:41

by Martin Schwidefsky

Subject: [PATCH] sysfs & dput.

Hi Pat,
there is another, small bug in sysfs. In sysfs_create_bin_file
dentry gets assigned the error value of the call to sysfs_create
if the call failed. The subsequent call to dput will crash. The
solution is to remove the assignment of the error to dentry.

blue skies,
Martin.

diffstat:
fs/sysfs/file.c | 2 --
1 files changed, 2 deletions(-)

diff -urN linux-2.6/fs/sysfs/file.c linux-2.6-s390/fs/sysfs/file.c
--- linux-2.6/fs/sysfs/file.c Mon Sep 8 21:49:52 2003
+++ linux-2.6-s390/fs/sysfs/file.c Thu Sep 11 13:38:57 2003
@@ -356,8 +356,6 @@
error = sysfs_create(dentry,(attr->mode & S_IALLUGO) | S_IFREG,init_file);
if (!error)
dentry->d_fsdata = (void *)attr;
- else
- dentry = ERR_PTR(error);
dput(dentry);
} else
error = PTR_ERR(dentry);

2003-09-11 16:38:59

by Patrick Mochel

Subject: Re: [PATCH] sysfs & dput.

> there is another, small bug in sysfs. In sysfs_create_bin_file
> dentry gets assigned the error value of the call to sysfs_create
> if the call failed. The subsequent call to dput will crash. The
> solution is to remove the assignment of the error to dentry.

Thanks for the catch; Andrew Morton also posted a patch yesterday.
However, your patch drops the error value, and his adds another variable.
The patch below should be equivalent (and fixes the same problem for
directories, too).

Please test it, if you get a chance.

> blue skies,

Heh, not today. :)


Thanks,


Pat


===== fs/sysfs/dir.c 1.11 vs edited =====
--- 1.11/fs/sysfs/dir.c Fri Aug 29 14:17:37 2003
+++ edited/fs/sysfs/dir.c Thu Sep 11 09:32:12 2003
@@ -32,12 +32,12 @@
int error = sysfs_create(dentry,
S_IFDIR| S_IRWXU | S_IRUGO | S_IXUGO,
init_dir);
+ dput(dentry);
if (!error) {
dentry->d_fsdata = k;
p->d_inode->i_nlink++;
} else
dentry = ERR_PTR(error);
- dput(dentry);
}
up(&p->d_inode->i_sem);
return dentry;
===== fs/sysfs/file.c 1.11 vs edited =====
--- 1.11/fs/sysfs/file.c Fri Aug 29 09:40:51 2003
+++ edited/fs/sysfs/file.c Thu Sep 11 09:32:19 2003
@@ -353,12 +353,14 @@
down(&dir->d_inode->i_sem);
dentry = sysfs_get_dentry(dir,attr->name);
if (!IS_ERR(dentry)) {
- error = sysfs_create(dentry,(attr->mode & S_IALLUGO) | S_IFREG,init_file);
+ error = sysfs_create(dentry,
+ (attr->mode & S_IALLUGO) | S_IFREG,
+ init_file);
+ dput(dentry);
if (!error)
dentry->d_fsdata = (void *)attr;
else
dentry = ERR_PTR(error);
- dput(dentry);
} else
error = PTR_ERR(dentry);
up(&dir->d_inode->i_sem);

2003-09-12 08:23:43

by Martin Schwidefsky

Subject: Re: [PATCH] sysfs & dput.

Hi Pat,

> > there is another, small bug in sysfs. In sysfs_create_bin_file
> > dentry gets assigned the error value of the call to sysfs_create
> > if the call failed. The subsequent call to dput will crash. The
> > solution is to remove the assignment of the error to dentry.
>
> Thanks for the catch; Andrew Morton also posted a patch yesterday.
> However, your patch drops the error value, and his adds another variable.
> The patch below should be equivalent (and fixes the same problem for
> directories, too).
>
> Please test it, if you get a chance.

My patch fixes sysfs_create_bin_file which returns error and not the dentry,
the assignment to dentry is a dead assignment. But anyway I indeed missed
the second place where the dput is problematic. What I don't like about
the new patch is that dentry is accessed AFTER the dput. The chances are
slim but the dentry could have been deleted and if the dput drops the
reference count to zero you access an already freed structure.

blue skies,
Martin