Re: [PATCH 3.2 058/221] debugfs: leave freeing a symlink body until inode eviction

[Luis Henriques]

On Tue, May 05, 2015 at 02:16:39AM +0100, Ben Hutchings wrote:
> 3.2.69-rc1 review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> 
> commit 0db59e59299f0b67450c5db21f7f316c8fb04e84 upstream.
> 
> As it is, we have debugfs_remove() racing with symlink traversals.
> Supply ->evict_inode() and do freeing there - inode will remain
> pinned until we are done with the symlink body.
> 
> And rip the idiocy with checking if dentry is positive right after
> we'd verified debugfs_positive(), which is a stronger check...
> 
> Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> [bwh: Backported to 3.2:
>  - Plumb in debugfs_super_operations, which we didn't previously define

It looks like this is introducing a regression[1].  Basically, simply
running df shows an error:

  df: `/sys/kernel/debug': Function not implemented

Doing 'strace df' shows the following:

  statfs64("/sys/kernel/debug", 84, 0xbfddc6bc) = -1 ENOSYS (Function not implemented)

A quick test shows that adding '.statfs = simple_statfs' in the
debugfs_super_operations struct fixes the problem, but I'm not sure
that's the right thing to do.

[1] http://bugs.launchpad.net/bugs/1465322

Cheers,
--
Luís

>  - Call truncate_inode_pages() instead of truncate_inode_pages_final()
>  - Call end_writeback() instead of clear_inode()]
> Signed-off-by: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
> ---
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -125,11 +125,30 @@ static inline int debugfs_positive(struc
>  	return dentry->d_inode && !d_unhashed(dentry);
>  }
>  
> +static void debugfs_evict_inode(struct inode *inode)
> +{
> +	truncate_inode_pages(&inode->i_data, 0);
> +	end_writeback(inode);
> +	if (S_ISLNK(inode->i_mode))
> +		kfree(inode->i_private);
> +}
> +
> +static const struct super_operations debugfs_super_operations = {
> +	.evict_inode	= debugfs_evict_inode,
> +};
> +
>  static int debug_fill_super(struct super_block *sb, void *data, int silent)
>  {
>  	static struct tree_descr debug_files[] = {{""}};
> +	int err;
> +
> +	err = simple_fill_super(sb, DEBUGFS_MAGIC, debug_files);
> +	if (err)
> +		return err;
>  
> -	return simple_fill_super(sb, DEBUGFS_MAGIC, debug_files);
> +	sb->s_op = &debugfs_super_operations;
> +
> +	return 0;
>  }
>  
>  static struct dentry *debug_mount(struct file_system_type *fs_type,
> @@ -312,23 +331,14 @@ static int __debugfs_remove(struct dentr
>  	int ret = 0;
>  
>  	if (debugfs_positive(dentry)) {
> -		if (dentry->d_inode) {
> -			dget(dentry);
> -			switch (dentry->d_inode->i_mode & S_IFMT) {
> -			case S_IFDIR:
> -				ret = simple_rmdir(parent->d_inode, dentry);
> -				break;
> -			case S_IFLNK:
> -				kfree(dentry->d_inode->i_private);
> -				/* fall through */
> -			default:
> -				simple_unlink(parent->d_inode, dentry);
> -				break;
> -			}
> -			if (!ret)
> -				d_delete(dentry);
> -			dput(dentry);
> -		}
> +		dget(dentry);
> +		if (S_ISDIR(dentry->d_inode->i_mode))
> +			ret = simple_rmdir(parent->d_inode, dentry);
> +		else
> +			simple_unlink(parent->d_inode, dentry);
> +		if (!ret)
> +			d_delete(dentry);
> +		dput(dentry);
>  	}
>  	return ret;
>  }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe stable" 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/