Re: [RFC] fix sysfs symlinks

viro
2004-04-22T21:38:59Z

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