[PATCH v2 1/2] btrfs: declare max_inline as u32

[PATCH v2 1/2] btrfs: declare max_inline as u32

[Anand Jain]

As of now btrfs_fs_info::max_line is u64, which can't be
larger than btrfs_fs_info::sectorsize which is defined as
u32, so make btrfs_fs_info::max_line u32,

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v1->v2: Born in v2.
 fs/btrfs/ctree.h | 2 +-
 fs/btrfs/super.c | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 4759e988b0df..0b738f0a9a23 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -795,7 +795,7 @@ struct btrfs_fs_info {
 	 * extent. The write side(mount/remount) is under ->s_umount lock,
 	 * so it is also safe.
 	 */
-	u64 max_inline;
+	u32 max_inline;
 
 	struct btrfs_transaction *running_transaction;
 	wait_queue_head_t transaction_throttle;
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 383be3609cc9..b13d68506f15 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -610,11 +610,11 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 				kfree(num);
 
 				if (info->max_inline) {
-					info->max_inline = min_t(u64,
+					info->max_inline = min_t(u32,
 						info->max_inline,
 						info->sectorsize);
 				}
-				btrfs_info(info, "max_inline at %llu",
+				btrfs_info(info, "max_inline at %u",
 					   info->max_inline);
 			} else {
 				ret = -ENOMEM;
@@ -1325,7 +1325,7 @@ static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry)
 	if (btrfs_test_opt(info, NOBARRIER))
 		seq_puts(seq, ",nobarrier");
 	if (info->max_inline != BTRFS_DEFAULT_MAX_INLINE)
-		seq_printf(seq, ",max_inline=%llu", info->max_inline);
+		seq_printf(seq, ",max_inline=%u", info->max_inline);
 	if (info->thread_pool_size !=  min_t(unsigned long,
 					     num_online_cpus() + 2, 8))
 		seq_printf(seq, ",thread_pool=%d", info->thread_pool_size);
@@ -1801,7 +1801,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
 	unsigned old_flags = sb->s_flags;
 	unsigned long old_opts = fs_info->mount_opt;
 	unsigned long old_compress_type = fs_info->compress_type;
-	u64 old_max_inline = fs_info->max_inline;
+	u32 old_max_inline = fs_info->max_inline;
 	int old_thread_pool_size = fs_info->thread_pool_size;
 	unsigned int old_metadata_ratio = fs_info->metadata_ratio;
 	int ret;
-- 
2.7.0

[PATCH v2 2/2] btrfs: verify max_inline mount parameter

[Anand Jain]

We aren't verifying the parameter passed to the max_inline mount option,
so we won't report and fail the mount if a junk value is specified for
example, -o max_inline=abc.
This patch converts the max_inline option to %d and checks if it's a
number >= 0.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v1->v2: use match_int ret value if error
        use %u instead of %d for parser

 fs/btrfs/super.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index b13d68506f15..02c7766e6849 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -344,7 +344,7 @@ static const match_table_t tokens = {
 	{Opt_datacow, "datacow"},
 	{Opt_nobarrier, "nobarrier"},
 	{Opt_barrier, "barrier"},
-	{Opt_max_inline, "max_inline=%s"},
+	{Opt_max_inline, "max_inline=%u"},
 	{Opt_alloc_start, "alloc_start=%s"},
 	{Opt_thread_pool, "thread_pool=%d"},
 	{Opt_compress, "compress"},
@@ -407,7 +407,7 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 			unsigned long new_flags)
 {
 	substring_t args[MAX_OPT_ARGS];
-	char *p, *num;
+	char *p;
 	u64 cache_gen;
 	int intarg;
 	int ret = 0;
@@ -604,22 +604,11 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 			}
 			break;
 		case Opt_max_inline:
-			num = match_strdup(&args[0]);
-			if (num) {
-				info->max_inline = memparse(num, NULL);
-				kfree(num);
-
-				if (info->max_inline) {
-					info->max_inline = min_t(u32,
-						info->max_inline,
-						info->sectorsize);
-				}
-				btrfs_info(info, "max_inline at %u",
-					   info->max_inline);
-			} else {
-				ret = -ENOMEM;
+			ret = match_int(&args[0], &intarg);
+			if (ret)
 				goto out;
-			}
+			info->max_inline = min_t(u32, intarg, info->sectorsize);
+			btrfs_info(info, "max_inline at %u", info->max_inline);
 			break;
 		case Opt_alloc_start:
 			btrfs_info(info,
-- 
2.7.0

Re: [PATCH v2 2/2] btrfs: verify max_inline mount parameter

[David Sterba]

On Tue, Feb 13, 2018 at 05:49:50PM +0800, Anand Jain wrote:
> We aren't verifying the parameter passed to the max_inline mount option,
> so we won't report and fail the mount if a junk value is specified for
> example, -o max_inline=abc.
> This patch converts the max_inline option to %d and checks if it's a
> number >= 0.

As the max_inline is a size, the suffixes are allowed here and this is
documented in the btrfs(5) page. I've checked all current options and
max_inline should be the only one where we want the suffixes.

Re: [PATCH v2 1/2] btrfs: declare max_inline as u32

[David Sterba]

On Tue, Feb 13, 2018 at 05:49:49PM +0800, Anand Jain wrote:
> As of now btrfs_fs_info::max_line is u64, which can't be
> larger than btrfs_fs_info::sectorsize which is defined as
> u32, so make btrfs_fs_info::max_line u32,
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH v2 2/2] btrfs: verify max_inline mount parameter

[Anand Jain]

On 02/14/2018 12:28 AM, David Sterba wrote:
> On Tue, Feb 13, 2018 at 05:49:50PM +0800, Anand Jain wrote:
>> We aren't verifying the parameter passed to the max_inline mount option,
>> so we won't report and fail the mount if a junk value is specified for
>> example, -o max_inline=abc.
>> This patch converts the max_inline option to %d and checks if it's a
>> number >= 0.
> 
> As the max_inline is a size, the suffixes are allowed here and this is
> documented in the btrfs(5) page. I've checked all current options and
> max_inline should be the only one where we want the suffixes.

  Oh. I ran out of ideas how to fix this.
  One idea is ... step1: memparse() 4K (for example) so we would get
  4096, step2: convert obtained 4096 back to 4K and step3: do string cmp
  of step1 and step2. This way we eliminate other junk chars passed.
  But looks like there isn't any tool to do the step2.

Thanks, Anand

> --
> 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
> 

Re: [PATCH v2 2/2] btrfs: verify max_inline mount parameter

[David Sterba]

On Thu, Feb 15, 2018 at 01:20:55AM +0800, Anand Jain wrote:
> 
> 
> On 02/14/2018 12:28 AM, David Sterba wrote:
> > On Tue, Feb 13, 2018 at 05:49:50PM +0800, Anand Jain wrote:
> >> We aren't verifying the parameter passed to the max_inline mount option,
> >> so we won't report and fail the mount if a junk value is specified for
> >> example, -o max_inline=abc.
> >> This patch converts the max_inline option to %d and checks if it's a
> >> number >= 0.
> > 
> > As the max_inline is a size, the suffixes are allowed here and this is
> > documented in the btrfs(5) page. I've checked all current options and
> > max_inline should be the only one where we want the suffixes.
> 
>   Oh. I ran out of ideas how to fix this.
>   One idea is ... step1: memparse() 4K (for example) so we would get
>   4096, step2: convert obtained 4096 back to 4K and step3: do string cmp
>   of step1 and step2. This way we eliminate other junk chars passed.
>   But looks like there isn't any tool to do the step2.

Memparse has 2nd parameter that will point after the parsed string, so
we can check if it's empty. This is already done in the resize string
parsing	(in btrfs_ioctl_resize), but missing from other memparse uses.

[PATCH v3] btrfs: verify max_inline mount parameter

[Anand Jain]

We aren't verifying the parameter passed to the max_inline mount option.
So we won't fail the mount if a junk value is specified, for example,
-o max_inline=abc. This patch checks if input is valid.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2->v3: Handle parameter with unit, such as 4K. Use memparse() 2nd arg.
v1->v2: use match_int ret value if error
        use %u instead of %d for parser

 fs/btrfs/super.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 77e0537e1db5..76b58da8d56d 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -605,7 +605,14 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 		case Opt_max_inline:
 			num = match_strdup(&args[0]);
 			if (num) {
-				info->max_inline = memparse(num, NULL);
+				char *retptr;
+
+				info->max_inline = memparse(num, &retptr);
+				if (*retptr != '\0') {
+					ret = -EINVAL;
+					kfree(num);
+					goto out;
+				}
 				kfree(num);
 
 				if (info->max_inline) {
-- 
2.15.0

Re: [PATCH v3] btrfs: verify max_inline mount parameter

[David Sterba]

On Mon, Feb 26, 2018 at 10:47:04AM +0800, Anand Jain wrote:
> We aren't verifying the parameter passed to the max_inline mount option.
> So we won't fail the mount if a junk value is specified, for example,
> -o max_inline=abc. This patch checks if input is valid.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2->v3: Handle parameter with unit, such as 4K. Use memparse() 2nd arg.
> v1->v2: use match_int ret value if error
>         use %u instead of %d for parser
> 
>  fs/btrfs/super.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 77e0537e1db5..76b58da8d56d 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -605,7 +605,14 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
>  		case Opt_max_inline:
>  			num = match_strdup(&args[0]);
>  			if (num) {
> -				info->max_inline = memparse(num, NULL);
> +				char *retptr;
> +
> +				info->max_inline = memparse(num, &retptr);

I missed it in the patch that changed max_inline to u32, memparse
returns unsigned long long, so this is not entrely correct and requires
a temporary variable.

We should also report if the user-specified value is larger than
BTRFS_MAX_METADATA_BLOCKSIZE .

This is not a trivial fix the existing patches so I'll remove "btrfs:
declare max_inline as u32".

To sum it up:

1. add check and return EINVAL with a message if max_inline is larger
   than the metadata block size
2. switch max_inline to u32 and add a temporary value to read from
   memparse
3. add change from this patch that catches the junk

> +				if (*retptr != '\0') {
> +					ret = -EINVAL;
> +					kfree(num);

The kfree can be moved before the check, we don't need 'num'.

> +					goto out;
> +				}
>  				kfree(num);
>  
>  				if (info->max_inline) {
> -- 
> 2.15.0
> 
> --
> 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

Re: [PATCH v3] btrfs: verify max_inline mount parameter

[Anand Jain]

On 02/27/2018 11:45 PM, David Sterba wrote:
> On Mon, Feb 26, 2018 at 10:47:04AM +0800, Anand Jain wrote:
>> We aren't verifying the parameter passed to the max_inline mount option.
>> So we won't fail the mount if a junk value is specified, for example,
>> -o max_inline=abc. This patch checks if input is valid.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v2->v3: Handle parameter with unit, such as 4K. Use memparse() 2nd arg.
>> v1->v2: use match_int ret value if error
>>          use %u instead of %d for parser
>>
>>   fs/btrfs/super.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>> index 77e0537e1db5..76b58da8d56d 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -605,7 +605,14 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
>>   		case Opt_max_inline:
>>   			num = match_strdup(&args[0]);
>>   			if (num) {
>> -				info->max_inline = memparse(num, NULL);
>> +				char *retptr;
>> +
>> +				info->max_inline = memparse(num, &retptr);
> 
> I missed it in the patch that changed max_inline to u32, memparse
> returns unsigned long long, so this is not entrely correct and requires
> a temporary variable.
> 
> We should also report if the user-specified value is larger than
> BTRFS_MAX_METADATA_BLOCKSIZE .

(Got diverted into something else. Sorry for the delay.)

Currently -o max_line can be only upto sectorsize.

We have MAX_INLINE_EXTENT_BUFFER_SIZE which is 64K and is equal to 
BTRFS_MAX_METADATA_BLOCKSIZE (also 64K)

I didn't get the point that max_inline is limited by sector size in the 
current design. Any idea?

Thanks, Anand

-------
#define BTRFS_MAX_METADATA_BLOCKSIZE 65536
#define INLINE_EXTENT_BUFFER_PAGES 16
#define MAX_INLINE_EXTENT_BUFFER_SIZE (INLINE_EXTENT_BUFFER_PAGES * 
PAGE_SIZE)
---------
static struct extent_buffer *
__alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
                       unsigned long len)
{
::
    /*
    * Sanity checks, currently the maximum is 64k covered by 16x 4k pages
    */
         BUILD_BUG_ON(BTRFS_MAX_METADATA_BLOCKSIZE
                 > MAX_INLINE_EXTENT_BUFFER_SIZE);
         BUG_ON(len > MAX_INLINE_EXTENT_BUFFER_SIZE);
-----
int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
                         unsigned long new_flags)
{
::
                                 if (info->max_inline) {
                                         info->max_inline = min_t(u64,
                                                 info->max_inline,
                                                 info->sectorsize);
                                 }
-----



> This is not a trivial fix the existing patches so I'll remove "btrfs:
> declare max_inline as u32".
> 
> To sum it up:
> 
> 1. add check and return EINVAL with a message if max_inline is larger
>     than the metadata block size
> 2. switch max_inline to u32 and add a temporary value to read from
>     memparse
> 3. add change from this patch that catches the junk
> 
>> +				if (*retptr != '\0') {
>> +					ret = -EINVAL;
>> +					kfree(num);
> 
> The kfree can be moved before the check, we don't need 'num'.
> 
>> +					goto out;
>> +				}
>>   				kfree(num);
>>   
>>   				if (info->max_inline) {
>> -- 
>> 2.15.0
>>
>> --
>> 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
> 

Re: [PATCH v3] btrfs: verify max_inline mount parameter

[David Sterba]

On Thu, Mar 01, 2018 at 07:51:23PM +0800, Anand Jain wrote:
> >> @@ -605,7 +605,14 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
> >>   		case Opt_max_inline:
> >>   			num = match_strdup(&args[0]);
> >>   			if (num) {
> >> -				info->max_inline = memparse(num, NULL);
> >> +				char *retptr;
> >> +
> >> +				info->max_inline = memparse(num, &retptr);
> > 
> > I missed it in the patch that changed max_inline to u32, memparse
> > returns unsigned long long, so this is not entrely correct and requires
> > a temporary variable.
> > 
> > We should also report if the user-specified value is larger than
> > BTRFS_MAX_METADATA_BLOCKSIZE .
> 
> (Got diverted into something else. Sorry for the delay.)
> 
> Currently -o max_line can be only upto sectorsize.
> 
> We have MAX_INLINE_EXTENT_BUFFER_SIZE which is 64K and is equal to 
> BTRFS_MAX_METADATA_BLOCKSIZE (also 64K)

BTRFS_MAX_METADATA_BLOCKSIZE is the limit of nodesize,
MAX_INLINE_EXTENT_BUFFER_SIZE exists only for sanity checking.


> I didn't get the point that max_inline is limited by sector size in the 
> current design. Any idea?

The sectorsize is now mandatory to be equal to the page size, so the
inline file size is rather limited by that.