[PATCH 1/4] Btrfs: check write access to mount earlier while creating snapshots

[PATCH 1/4] Btrfs: check write access to mount earlier while creating snapshots

[Liu Bo]

Move check of write access to mount into upper functions so that we can
use mnt_want_write_file instead, which is faster than mnt_want_write.

Signed-off-by: Liu Bo <liubo2009@cn.fujitsu.com>
---
 fs/btrfs/ioctl.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 0e92e57..b5e946e 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -652,13 +652,9 @@ static noinline int btrfs_mksubvol(struct path *parent,
 	if (dentry->d_inode)
 		goto out_dput;
 
-	error = mnt_want_write(parent->mnt);
-	if (error)
-		goto out_dput;
-
 	error = btrfs_may_create(dir, dentry);
 	if (error)
-		goto out_drop_write;
+		goto out_dput;
 
 	down_read(&BTRFS_I(dir)->root->fs_info->subvol_sem);
 
@@ -676,8 +672,6 @@ static noinline int btrfs_mksubvol(struct path *parent,
 		fsnotify_mkdir(dir, dentry);
 out_up_read:
 	up_read(&BTRFS_I(dir)->root->fs_info->subvol_sem);
-out_drop_write:
-	mnt_drop_write(parent->mnt);
 out_dput:
 	dput(dentry);
 out_unlock:
@@ -1393,16 +1387,20 @@ static noinline int btrfs_ioctl_snap_create_transid(struct file *file,
 	if (root->fs_info->sb->s_flags & MS_RDONLY)
 		return -EROFS;
 
+	ret = mnt_want_write_file(file);
+	if (ret)
+		goto out;
+
 	namelen = strlen(name);
 	if (strchr(name, '/')) {
 		ret = -EINVAL;
-		goto out;
+		goto out_drop_write;
 	}
 
 	if (name[0] == '.' &&
 	   (namelen == 1 || (name[1] == '.' && namelen == 2))) {
 		ret = -EEXIST;
-		goto out;
+		goto out_drop_write;
 	}
 
 	if (subvol) {
@@ -1413,7 +1411,7 @@ static noinline int btrfs_ioctl_snap_create_transid(struct file *file,
 		src_file = fget(fd);
 		if (!src_file) {
 			ret = -EINVAL;
-			goto out;
+			goto out_drop_write;
 		}
 
 		src_inode = src_file->f_path.dentry->d_inode;
@@ -1422,13 +1420,15 @@ static noinline int btrfs_ioctl_snap_create_transid(struct file *file,
 			       "another FS\n");
 			ret = -EINVAL;
 			fput(src_file);
-			goto out;
+			goto out_drop_write;
 		}
 		ret = btrfs_mksubvol(&file->f_path, name, namelen,
 				     BTRFS_I(src_inode)->root,
 				     transid, readonly);
 		fput(src_file);
 	}
+out_drop_write:
+	mnt_drop_write_file(file);
 out:
 	return ret;
 }
-- 
1.6.5.2

[PATCH 2/4] Btrfs: remove redundant r/o check for superblock

[Liu Bo]

mnt_want_write() and mnt_want_write_file() will check sb->s_flags with
MS_RDONLY, and we don't need to do it ourselves.

Signed-off-by: Liu Bo <liubo2009@cn.fujitsu.com>
---
 fs/btrfs/ioctl.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index b5e946e..069cd85 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1379,14 +1379,10 @@ static noinline int btrfs_ioctl_snap_create_transid(struct file *file,
 						    u64 *transid,
 						    bool readonly)
 {
-	struct btrfs_root *root = BTRFS_I(fdentry(file)->d_inode)->root;
 	struct file *src_file;
 	int namelen;
 	int ret = 0;
 
-	if (root->fs_info->sb->s_flags & MS_RDONLY)
-		return -EROFS;
-
 	ret = mnt_want_write_file(file);
 	if (ret)
 		goto out;
@@ -3265,9 +3261,6 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg)
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	if (fs_info->sb->s_flags & MS_RDONLY)
-		return -EROFS;
-
 	ret = mnt_want_write(file->f_path.mnt);
 	if (ret)
 		return ret;
-- 
1.6.5.2

[PATCH 3/4] Btrfs: use mnt_want_write_file instead of mnt_want_write

[Liu Bo]

mnt_want_write_file is faster when file has been opened for write.

Signed-off-by: Liu Bo <liubo2009@cn.fujitsu.com>
---
 fs/btrfs/ioctl.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 069cd85..df4c04d 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3261,7 +3261,7 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg)
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	ret = mnt_want_write(file->f_path.mnt);
+	ret = mnt_want_write_file(file);
 	if (ret)
 		return ret;
 
@@ -3331,7 +3331,7 @@ out_bargs:
 out:
 	mutex_unlock(&fs_info->balance_mutex);
 	mutex_unlock(&fs_info->volume_mutex);
-	mnt_drop_write(file->f_path.mnt);
+	mnt_drop_write_file(file);
 	return ret;
 }
 
-- 
1.6.5.2

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

[Liu Bo]

$ 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;
 
 	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;
 }
 
-- 
1.6.5.2

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

[Tsutomu Itoh]

(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;
>   }
>   
>