Re: [RFC] fix sysfs symlinks

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

On Fri, Apr 23, 2004 at 02:22:18PM +0530, Maneesh Soni wrote:
> @@ -136,6 +138,12 @@ restart:
>  			 */
>  			spin_unlock(&dcache_lock);
>  			d_delete(d);

ITYM "d_drop(d)" here.  Right now these are equivalent (and d_drop() is
less work) since we are holding at least two references to d; if/when
we stop pinning leaves of the tree in core, the check below will become
unsafe with d_delete().

>  	new_dentry = sysfs_get_dentry(parent, new_name);
> -	d_move(kobj->dentry, new_dentry);
> -	kobject_set_name(kobj,new_name);
> +	if (!IS_ERR(new_dentry)) {
> +		down_write(&sysfs_rename_sem);
> +		d_move(kobj->dentry, new_dentry);
> +		kobject_set_name(kobj,new_name);
> +		up_write(&sysfs_rename_sem);
> +	}
>  	up(&parent->d_inode->i_sem);	
>  }

BTW, the above leaks because of unbalanced sysfs_get_dentry().  You need
a dput() in there.

> diff -puN fs/sysfs/inode.c~sysfs-symlinks-fix fs/sysfs/inode.c
> --- linux-2.6.6-rc2-mm1/fs/sysfs/inode.c~sysfs-symlinks-fix	2004-04-23 10:22:41.000000000 +0530
> +++ linux-2.6.6-rc2-mm1-maneesh/fs/sysfs/inode.c	2004-04-23 11:03:38.000000000 +0530
> @@ -97,6 +97,11 @@ void sysfs_hash_and_remove(struct dentry
>  				 atomic_read(&victim->d_count));
>  
>  			d_delete(victim);
> +			/* release the target kobject in case of 
> +			 * a symlink
> +			 */
> +			if (S_ISLNK(victim->d_inode->i_mode))
> +				kobject_put(victim->d_fsdata);

Same s/d_delete/d_drop/ issue.

>  			simple_unlink(dir->d_inode,victim);
>  		}

> @@ -70,11 +70,13 @@ void sysfs_remove_group(struct kobject *
>  	else
>  		dir = dget(kobj->dentry);
>  
> -	remove_files(dir,grp);
> -	if (grp->name)
> -		sysfs_remove_subdir(dir);
> -	/* release the ref. taken in this routine */
> -	dput(dir);
> +	if (dir && !IS_ERR(dir)) {
> +		remove_files(dir,grp);
> +		if (grp->name)
> +			sysfs_remove_subdir(dir);
> +		/* release the ref. taken in this routine */
> +		dput(dir);
> +	}
>  }

Hmm...  I thought that's what
		if (error)
			return error;
several lines above had been about.  Can we get there with NULL or ERR_PTR()
in dir?