btrfs: check options during subsequent mount
diff mbox

Message ID 20170428091409.19621-1-anand.jain@oracle.com
State New
Headers show

Commit Message

Anand Jain April 28, 2017, 9:14 a.m. UTC
We allow recursive mounts with subvol options such as [1]

[1]
 mount -o rw,compress=lzo /dev/sdc /btrfs1
 mount -o ro,subvol=sv2 /dev/sdc /btrfs2

And except for the btrfs-specific subvol and subvolid options
all-other options are just ignored in the subsequent mounts.

In the below example [2] the effect compression is only zlib,
even though there was no error for the option -o compress=lzo.

[2]
----
 # mount -o compress=zlib /dev/sdc /btrfs1
 #echo $?
 0

 # mount -o compress=lzo /dev/sdc /btrfs
 #echo $?
 0

 #cat /proc/self/mounts
 ::
 /dev/sdc /btrfs1 btrfs rw,relatime,compress=zlib,space_cache,subvolid=5,subvol=/ 0 0
 /dev/sdc /btrfs btrfs rw,relatime,compress=zlib,space_cache,subvolid=5,subvol=/ 0 0
----

Further, random string .. has no error as well.
-----
 # mount -o compress=zlib /dev/sdc /btrfs1
 #echo $?
 0

 # mount -o something /dev/sdc /btrfs
 #echo $?
 0
-----

This patch fixes the above issue, by checking the if the passed
options are only subvol or subvolid in the subsequent mount.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/super.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

Comments

Andrei Borzenkov April 29, 2017, 5:26 a.m. UTC | #1
28.04.2017 12:14, Anand Jain пишет:
> We allow recursive mounts with subvol options such as [1]
> 
> [1]
>  mount -o rw,compress=lzo /dev/sdc /btrfs1
>  mount -o ro,subvol=sv2 /dev/sdc /btrfs2
> 
> And except for the btrfs-specific subvol and subvolid options
> all-other options are just ignored in the subsequent mounts.
> 
> In the below example [2] the effect compression is only zlib,
> even though there was no error for the option -o compress=lzo.
> 
> [2]
> ----
>  # mount -o compress=zlib /dev/sdc /btrfs1
>  #echo $?
>  0
> 
>  # mount -o compress=lzo /dev/sdc /btrfs
>  #echo $?
>  0
> 
>  #cat /proc/self/mounts
>  ::
>  /dev/sdc /btrfs1 btrfs rw,relatime,compress=zlib,space_cache,subvolid=5,subvol=/ 0 0
>  /dev/sdc /btrfs btrfs rw,relatime,compress=zlib,space_cache,subvolid=5,subvol=/ 0 0
> ----
> 
> Further, random string .. has no error as well.
> -----
>  # mount -o compress=zlib /dev/sdc /btrfs1
>  #echo $?
>  0
> 
>  # mount -o something /dev/sdc /btrfs
>  #echo $?
>  0
> -----
> 
> This patch fixes the above issue, by checking the if the passed
> options are only subvol or subvolid in the subsequent mount.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/super.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 9530a333d302..e0e542345c38 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -389,6 +389,44 @@ static const match_table_t tokens = {
>  	{Opt_err, NULL},
>  };
>  
> +static int parse_recursive_mount_options(char *data)
> +{
> +	substring_t args[MAX_OPT_ARGS];
> +	char *options, *orig;
> +	char *p;
> +	int ret = 0;
> +
> +	/*
> +	 * This is not a remount thread, but we allow recursive mounts
> +	 * with varying RO/RW flag to support subvol-mounts. So error-out
> +	 * if any other option being passed in here.
> +	 */
> +
> +	options = kstrdup(data, GFP_NOFS);
> +	if (!options)
> +		return -ENOMEM;
> +
> +	orig = options;
> +
> +	while ((p = strsep(&options, ",")) != NULL) {
> +		int token;
> +		if (!*p)
> +			continue;
> +
> +		token = match_token(p, tokens, args);
> +		switch(token) {
> +		case Opt_subvol:
> +		case Opt_subvolid:
> +			break;
> +		default:
> +			ret = -EBUSY;
> +		}
> +	}
> +
> +	kfree(orig);
> +	return ret;
> +}
> +
>  /*
>   * Regular mount options parser.  Everything that is needed only when
>   * reading in a new superblock is parsed here.
> @@ -1611,6 +1649,8 @@ static struct dentry *btrfs_mount(struct file_system_type *fs_type, int flags,
>  		free_fs_info(fs_info);
>  		if ((flags ^ s->s_flags) & MS_RDONLY)
>  			error = -EBUSY;
> +		if (parse_recursive_mount_options(data))
> +			error = -EBUSY;

But if subvol= was passed, it should not reach this place at all -
btrfs_mount() returns earlier in

        if (subvol_name || subvol_objectid != BTRFS_FS_TREE_OBJECTID) {
                /* mount_subvol() will free subvol_name. */
                return mount_subvol(subvol_name, subvol_objectid, flags,
                                    device_name, data);
        }

So check for subvol here seems redundant.

>  	} else {
>  		snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev);
>  		btrfs_sb(s)->bdev_holder = fs_type;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Anand Jain April 29, 2017, 8:27 a.m. UTC | #2
On 04/29/2017 01:26 PM, Andrei Borzenkov wrote:
> 28.04.2017 12:14, Anand Jain пишет:
>> We allow recursive mounts with subvol options such as [1]
>>
>> [1]
>>  mount -o rw,compress=lzo /dev/sdc /btrfs1
>>  mount -o ro,subvol=sv2 /dev/sdc /btrfs2
>>
>> And except for the btrfs-specific subvol and subvolid options
>> all-other options are just ignored in the subsequent mounts.
>>
>> In the below example [2] the effect compression is only zlib,
>> even though there was no error for the option -o compress=lzo.
>>
>> [2]
>> ----
>>  # mount -o compress=zlib /dev/sdc /btrfs1
>>  #echo $?
>>  0
>>
>>  # mount -o compress=lzo /dev/sdc /btrfs
>>  #echo $?
>>  0
>>
>>  #cat /proc/self/mounts
>>  ::
>>  /dev/sdc /btrfs1 btrfs rw,relatime,compress=zlib,space_cache,subvolid=5,subvol=/ 0 0
>>  /dev/sdc /btrfs btrfs rw,relatime,compress=zlib,space_cache,subvolid=5,subvol=/ 0 0
>> ----
>>
>> Further, random string .. has no error as well.
>> -----
>>  # mount -o compress=zlib /dev/sdc /btrfs1
>>  #echo $?
>>  0
>>
>>  # mount -o something /dev/sdc /btrfs
>>  #echo $?
>>  0
>> -----
>>
>> This patch fixes the above issue, by checking the if the passed
>> options are only subvol or subvolid in the subsequent mount.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>  fs/btrfs/super.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 40 insertions(+)
>>
>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>> index 9530a333d302..e0e542345c38 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -389,6 +389,44 @@ static const match_table_t tokens = {
>>  	{Opt_err, NULL},
>>  };
>>
>> +static int parse_recursive_mount_options(char *data)
>> +{
>> +	substring_t args[MAX_OPT_ARGS];
>> +	char *options, *orig;
>> +	char *p;
>> +	int ret = 0;
>> +
>> +	/*
>> +	 * This is not a remount thread, but we allow recursive mounts
>> +	 * with varying RO/RW flag to support subvol-mounts. So error-out
>> +	 * if any other option being passed in here.
>> +	 */
>> +
>> +	options = kstrdup(data, GFP_NOFS);
>> +	if (!options)
>> +		return -ENOMEM;
>> +
>> +	orig = options;
>> +
>> +	while ((p = strsep(&options, ",")) != NULL) {
>> +		int token;
>> +		if (!*p)
>> +			continue;
>> +
>> +		token = match_token(p, tokens, args);
>> +		switch(token) {
>> +		case Opt_subvol:
>> +		case Opt_subvolid:
>> +			break;
>> +		default:
>> +			ret = -EBUSY;
>> +		}
>> +	}
>> +
>> +	kfree(orig);
>> +	return ret;
>> +}
>> +
>>  /*
>>   * Regular mount options parser.  Everything that is needed only when
>>   * reading in a new superblock is parsed here.
>> @@ -1611,6 +1649,8 @@ static struct dentry *btrfs_mount(struct file_system_type *fs_type, int flags,
>>  		free_fs_info(fs_info);
>>  		if ((flags ^ s->s_flags) & MS_RDONLY)
>>  			error = -EBUSY;
>> +		if (parse_recursive_mount_options(data))
>> +			error = -EBUSY;
>
> But if subvol= was passed, it should not reach this place at all -
> btrfs_mount() returns earlier in
>
>         if (subvol_name || subvol_objectid != BTRFS_FS_TREE_OBJECTID) {
>                 /* mount_subvol() will free subvol_name. */
>                 return mount_subvol(subvol_name, subvol_objectid, flags,
>                                     device_name, data);
>         }
>
> So check for subvol here seems redundant.

Thanks for the review.

No its not, actually mount_subvol() will call vfs_kern_mount() which
will call out our mount entry point btrfs_mount() with subvolid=0,
and btrfs_parse_early_options() will set subvolid=5, which then
fails the check and goes to the rest of the code in btrfs_mount().

HTH.

Thanks, Anand



>>  	} else {
>>  		snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev);
>>  		btrfs_sb(s)->bdev_holder = fs_type;
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Anand Jain May 24, 2017, 9:13 a.m. UTC | #3
David,

  Can I ping you on this patch ? Wonder if there is any concern.

Thanks, Anand


On 04/28/2017 05:14 PM, Anand Jain wrote:
> We allow recursive mounts with subvol options such as [1]
>
> [1]
>  mount -o rw,compress=lzo /dev/sdc /btrfs1
>  mount -o ro,subvol=sv2 /dev/sdc /btrfs2
>
> And except for the btrfs-specific subvol and subvolid options
> all-other options are just ignored in the subsequent mounts.
>
> In the below example [2] the effect compression is only zlib,
> even though there was no error for the option -o compress=lzo.
>
> [2]
> ----
>  # mount -o compress=zlib /dev/sdc /btrfs1
>  #echo $?
>  0
>
>  # mount -o compress=lzo /dev/sdc /btrfs
>  #echo $?
>  0
>
>  #cat /proc/self/mounts
>  ::
>  /dev/sdc /btrfs1 btrfs rw,relatime,compress=zlib,space_cache,subvolid=5,subvol=/ 0 0
>  /dev/sdc /btrfs btrfs rw,relatime,compress=zlib,space_cache,subvolid=5,subvol=/ 0 0
> ----
>
> Further, random string .. has no error as well.
> -----
>  # mount -o compress=zlib /dev/sdc /btrfs1
>  #echo $?
>  0
>
>  # mount -o something /dev/sdc /btrfs
>  #echo $?
>  0
> -----
>
> This patch fixes the above issue, by checking the if the passed
> options are only subvol or subvolid in the subsequent mount.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/super.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 9530a333d302..e0e542345c38 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -389,6 +389,44 @@ static const match_table_t tokens = {
>  	{Opt_err, NULL},
>  };
>
> +static int parse_recursive_mount_options(char *data)
> +{
> +	substring_t args[MAX_OPT_ARGS];
> +	char *options, *orig;
> +	char *p;
> +	int ret = 0;
> +
> +	/*
> +	 * This is not a remount thread, but we allow recursive mounts
> +	 * with varying RO/RW flag to support subvol-mounts. So error-out
> +	 * if any other option being passed in here.
> +	 */
> +
> +	options = kstrdup(data, GFP_NOFS);
> +	if (!options)
> +		return -ENOMEM;
> +
> +	orig = options;
> +
> +	while ((p = strsep(&options, ",")) != NULL) {
> +		int token;
> +		if (!*p)
> +			continue;
> +
> +		token = match_token(p, tokens, args);
> +		switch(token) {
> +		case Opt_subvol:
> +		case Opt_subvolid:
> +			break;
> +		default:
> +			ret = -EBUSY;
> +		}
> +	}
> +
> +	kfree(orig);
> +	return ret;
> +}
> +
>  /*
>   * Regular mount options parser.  Everything that is needed only when
>   * reading in a new superblock is parsed here.
> @@ -1611,6 +1649,8 @@ static struct dentry *btrfs_mount(struct file_system_type *fs_type, int flags,
>  		free_fs_info(fs_info);
>  		if ((flags ^ s->s_flags) & MS_RDONLY)
>  			error = -EBUSY;
> +		if (parse_recursive_mount_options(data))
> +			error = -EBUSY;
>  	} else {
>  		snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev);
>  		btrfs_sb(s)->bdev_holder = fs_type;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba July 4, 2017, 4:31 p.m. UTC | #4
On Wed, May 24, 2017 at 05:13:24PM +0800, Anand Jain wrote:
>   Can I ping you on this patch ? Wonder if there is any concern.

Well, there's my feeling that touching mount code always leads to some
surprise. The specific options apply to the whole filesystem, this is a
known limitation. Your patch seems to change the behaviour, when there
are extra mount options found, this could break existing setups. The
patch could be right after all, but I don't have time to look closely
riht now.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 9530a333d302..e0e542345c38 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -389,6 +389,44 @@  static const match_table_t tokens = {
 	{Opt_err, NULL},
 };
 
+static int parse_recursive_mount_options(char *data)
+{
+	substring_t args[MAX_OPT_ARGS];
+	char *options, *orig;
+	char *p;
+	int ret = 0;
+
+	/*
+	 * This is not a remount thread, but we allow recursive mounts
+	 * with varying RO/RW flag to support subvol-mounts. So error-out
+	 * if any other option being passed in here.
+	 */
+
+	options = kstrdup(data, GFP_NOFS);
+	if (!options)
+		return -ENOMEM;
+
+	orig = options;
+
+	while ((p = strsep(&options, ",")) != NULL) {
+		int token;
+		if (!*p)
+			continue;
+
+		token = match_token(p, tokens, args);
+		switch(token) {
+		case Opt_subvol:
+		case Opt_subvolid:
+			break;
+		default:
+			ret = -EBUSY;
+		}
+	}
+
+	kfree(orig);
+	return ret;
+}
+
 /*
  * Regular mount options parser.  Everything that is needed only when
  * reading in a new superblock is parsed here.
@@ -1611,6 +1649,8 @@  static struct dentry *btrfs_mount(struct file_system_type *fs_type, int flags,
 		free_fs_info(fs_info);
 		if ((flags ^ s->s_flags) & MS_RDONLY)
 			error = -EBUSY;
+		if (parse_recursive_mount_options(data))
+			error = -EBUSY;
 	} else {
 		snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev);
 		btrfs_sb(s)->bdev_holder = fs_type;