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

Luis Henriques
2015-06-16T16:33:25Z

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@zeniv.linux.org.uk>
>
> 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@zeniv.linux.org.uk>
> [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@decadent.org.uk>
> ---
> --- 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html