Re: [RFC] fix sysfs symlinks

[viro@parcelfarce.linux.theplanet.co.uk]

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