Re: [PATCH 4/4] Btrfs: do not set subvolume flags in readonly mode

[Tsutomu Itoh <t-itoh@jp.fujitsu.com>]

(2012/06/21 20:48), Liu Bo wrote:
> $ mkfs.btrfs /dev/sdb7
> $ btrfstune -S1 /dev/sdb7
> $ mount /dev/sdb7 /mnt/btrfs
> mount: block device /dev/sdb7 is write-protected, mounting read-only
> $ btrfs dev add /dev/sdb8 /mnt/btrfs/
> 
> Now we get a btrfs in which mnt flags has readonly but sb flags does
> not.  So for those ioctls that only check sb flags with MS_RDONLY, it
> is going to be a problem.
> Setting subvolume flags is such an ioctl, we should use mnt_want_write_file()
> to check RO flags.
> 
> Signed-off-by: Liu Bo <liubo2009@cn.fujitsu.com>
> ---
>   fs/btrfs/ioctl.c |   27 ++++++++++++++++++---------
>   1 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index df4c04d..ae29737 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1519,29 +1519,35 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
>   	u64 flags;
>   	int ret = 0;
>   
> -	if (root->fs_info->sb->s_flags & MS_RDONLY)
> -		return -EROFS;
> +	ret = mnt_want_write_file(file);
> +	if (ret)
> +		goto out;
>   
> +	ret = -EINVAL;
>   	if (btrfs_ino(inode) != BTRFS_FIRST_FREE_OBJECTID)
> -		return -EINVAL;
> +		goto out_drop_write;
>   
> +	ret = -EFAULT;
>   	if (copy_from_user(&flags, arg, sizeof(flags)))
> -		return -EFAULT;
> +		goto out_drop_write;
>   
> +	ret = -EINVAL;
>   	if (flags & BTRFS_SUBVOL_CREATE_ASYNC)
> -		return -EINVAL;
> +		goto out_drop_write;
>   
> +	ret = -EOPNOTSUPP;
>   	if (flags & ~BTRFS_SUBVOL_RDONLY)
> -		return -EOPNOTSUPP;
> +		goto out_drop_write;
>   
> +	ret = -EACCES;
>   	if (!inode_owner_or_capable(inode))
> -		return -EACCES;
> +		goto out_drop_write;

I think that

	if (!inode_owner_or_capable(inode)) {
		ret = -EACCES;
		goto out_drop_write;
	}

is better than

	ret = -EACCES;
	if (!inode_owner_or_capable(inode))
		goto out_drop_write;

Thanks,
Tsutomu

>   
>   	down_write(&root->fs_info->subvol_sem);
>   
>   	/* nothing to do */
>   	if (!!(flags & BTRFS_SUBVOL_RDONLY) == btrfs_root_readonly(root))
> -		goto out;
> +		goto out_drop_sem;
>   
>   	root_flags = btrfs_root_flags(&root->root_item);
>   	if (flags & BTRFS_SUBVOL_RDONLY)
> @@ -1564,8 +1570,11 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
>   out_reset:
>   	if (ret)
>   		btrfs_set_root_flags(&root->root_item, root_flags);
> -out:
> +out_drop_sem:
>   	up_write(&root->fs_info->subvol_sem);
> +out_drop_write:
> +	mnt_drop_write_file(file);
> +out:
>   	return ret;
>   }
>   
>