Re: [PATCH 03/13] sysfs: Use dentry_ops instead of directlyplaying with the dcache

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebiederm@xxxxxxxxxxxx):
> From: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
> 
> Calling d_drop unconditionally when a sysfs_dirent is deleted has
> the potential to leak mounts, so instead implement dentry delete
> and revalidate operations that cause sysfs dentries to be removed
> at the appropriate time.
> 
> Acked-by: Tejun Heo <tj@xxxxxxxxxx>
> Signed-off-by: Eric W. Biederman <ebiederm@xxxxxxxxxxxxxxxxxx>

I like where it's heading (and spent some time staring at the
related code, looks kosher).

Acked-by: Serge Hallyn <serue@xxxxxxxxxx>

> ---
>  fs/sysfs/dir.c |   73 +++++++++++++++++++++++++++++++++++--------------------
>  1 files changed, 46 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> index 130dfc3..b5e8499 100644
> --- a/fs/sysfs/dir.c
> +++ b/fs/sysfs/dir.c
> @@ -298,6 +298,46 @@ void release_sysfs_dirent(struct sysfs_dirent * sd)
>  		goto repeat;
>  }
> 
> +static int sysfs_dentry_delete(struct dentry *dentry)
> +{
> +	struct sysfs_dirent *sd = dentry->d_fsdata;
> +	return !!(sd->s_flags & SYSFS_FLAG_REMOVED);
> +}
> +
> +static int sysfs_dentry_revalidate(struct dentry *dentry, struct nameidata *nd)
> +{
> +	struct sysfs_dirent *sd = dentry->d_fsdata;
> +	int is_dir;
> +
> +	mutex_lock(&sysfs_mutex);
> +
> +	/* The sysfs dirent has been deleted */
> +	if (sd->s_flags & SYSFS_FLAG_REMOVED)
> +		goto out_bad;
> +
> +	mutex_unlock(&sysfs_mutex);
> +out_valid:
> +	return 1;
> +out_bad:
> +	/* Remove the dentry from the dcache hashes.
> +	 * If this is a deleted dentry we use d_drop instead of d_delete
> +	 * so sysfs doesn't need to cope with negative dentries.
> +	 */
> +	is_dir = (sysfs_type(sd) == SYSFS_DIR);
> +	mutex_unlock(&sysfs_mutex);
> +	if (is_dir) {
> +		/* If we have submounts we must allow the vfs caches
> +		 * to lie about the state of the filesystem to prevent
> +		 * leaks and other nasty things.
> +		 */
> +		if (have_submounts(dentry))
> +			goto out_valid;
> +		shrink_dcache_parent(dentry);
> +	}
> +	d_drop(dentry);
> +	return 0;
> +}
> +
>  static void sysfs_dentry_iput(struct dentry * dentry, struct inode * inode)
>  {
>  	struct sysfs_dirent * sd = dentry->d_fsdata;
> @@ -307,6 +347,8 @@ static void sysfs_dentry_iput(struct dentry * dentry, struct inode * inode)
>  }
> 
>  static const struct dentry_operations sysfs_dentry_ops = {
> +	.d_revalidate	= sysfs_dentry_revalidate,
> +	.d_delete	= sysfs_dentry_delete,
>  	.d_iput		= sysfs_dentry_iput,
>  };
> 
> @@ -527,44 +569,21 @@ void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
>  }
> 
>  /**
> - *	sysfs_drop_dentry - drop dentry for the specified sysfs_dirent
> + *	sysfs_dec_nlink - Decrement link count for the specified sysfs_dirent
>   *	@sd: target sysfs_dirent
>   *
> - *	Drop dentry for @sd.  @sd must have been unlinked from its
> + *	Decrement nlink for @sd.  @sd must have been unlinked from its
>   *	parent on entry to this function such that it can't be looked
>   *	up anymore.
>   */
> -static void sysfs_drop_dentry(struct sysfs_dirent *sd)
> +static void sysfs_dec_nlink(struct sysfs_dirent *sd)
>  {
>  	struct inode *inode;
> -	struct dentry *dentry;
> 
>  	inode = ilookup(sysfs_sb, sd->s_ino);
>  	if (!inode)
>  		return;
> 
> -	/* Drop any existing dentries associated with sd.
> -	 *
> -	 * For the dentry to be properly freed we need to grab a
> -	 * reference to the dentry under the dcache lock,  unhash it,
> -	 * and then put it.  The playing with the dentry count allows
> -	 * dput to immediately free the dentry  if it is not in use.
> -	 */
> -repeat:
> -	spin_lock(&dcache_lock);
> -	list_for_each_entry(dentry, &inode->i_dentry, d_alias) {
> -		if (d_unhashed(dentry))
> -			continue;
> -		dget_locked(dentry);
> -		spin_lock(&dentry->d_lock);
> -		__d_drop(dentry);
> -		spin_unlock(&dentry->d_lock);
> -		spin_unlock(&dcache_lock);
> -		dput(dentry);
> -		goto repeat;
> -	}
> -	spin_unlock(&dcache_lock);
> -
>  	/* adjust nlink and update timestamp */
>  	mutex_lock(&inode->i_mutex);
> 
> @@ -611,7 +630,7 @@ void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt)
>  		acxt->removed = sd->s_sibling;
>  		sd->s_sibling = NULL;
> 
> -		sysfs_drop_dentry(sd);
> +		sysfs_dec_nlink(sd);
>  		sysfs_deactivate(sd);
>  		unmap_bin_file(sd);
>  		sysfs_put(sd);
> -- 
> 1.6.5.2.143.g8cc62
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/