[4/5] btrfs-progs: resize: more sensible error messages for bad sizes
diff mbox series

Message ID 20190814010402.22546-4-jeffm@suse.com
State New
Headers show
Series
  • [1/5] btrfs-progs: mkfs: treat btrfs_add_to_fsid as fatal error
Related show

Commit Message

Jeff Mahoney Aug. 14, 2019, 1:04 a.m. UTC
If a user attempts to resize a file system to a size under 256MiB,
it will be rejected with EINVAL and get then unhelpful error message
"ERROR: unable to resize '/path': Invalid argument."

This commit performs that check before issuing the ioctl to report
a more sensible error message.   We also do overflow/underflow
checking when -/+ size is used and report those errors as well.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 cmds/filesystem.c | 41 +++++++++++++++++++++++++++++++++++++++++
 common/utils.c    |  2 +-
 common/utils.h    |  2 +-
 3 files changed, 43 insertions(+), 2 deletions(-)

Comments

Qu Wenruo Aug. 14, 2019, 1:53 a.m. UTC | #1
On 2019/8/14 上午9:04, Jeff Mahoney wrote:
> If a user attempts to resize a file system to a size under 256MiB,
> it will be rejected with EINVAL and get then unhelpful error message
> "ERROR: unable to resize '/path': Invalid argument."
> 
> This commit performs that check before issuing the ioctl to report
> a more sensible error message.   We also do overflow/underflow
> checking when -/+ size is used and report those errors as well.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  cmds/filesystem.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  common/utils.c    |  2 +-
>  common/utils.h    |  2 +-
>  3 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/cmds/filesystem.c b/cmds/filesystem.c
> index 4f22089a..e3415126 100644
> --- a/cmds/filesystem.c
> +++ b/cmds/filesystem.c
> @@ -34,10 +34,12 @@
>  #include "kerncompat.h"
>  #include "ctree.h"
>  #include "common/utils.h"
> +#include "common/device-utils.h"
>  #include "volumes.h"
>  #include "cmds/commands.h"
>  #include "cmds/filesystem-usage.h"
>  #include "kernel-lib/list_sort.h"
> +#include "kernel-lib/overflow.h"
>  #include "disk-io.h"
>  #include "common/help.h"
>  #include "common/fsfeatures.h"
> @@ -1062,6 +1064,41 @@ next:
>  }
>  static DEFINE_SIMPLE_COMMAND(filesystem_defrag, "defragment");
>  
> +static int check_resize_size(const char *path, const char *amount)
> +{
> +	int mod = 0;
> +	u64 oldsize = 0, size, newsize;
> +
> +	if (*amount == '-')
> +		mod = -1;
> +	else if (*amount == '+')
> +		mod = 1;
> +
> +	if (mod) {
> +		amount++;
> +		oldsize = disk_size(path);
> +		if (oldsize == 0)
> +			return -1;
> +	}
> +
> +	size = parse_size(amount);
> +
> +	if (mod == -1 && check_sub_overflow(oldsize, size, &newsize)) {
> +		error("can't resize to negative size");
> +		return -1;
> +	} else if (mod == 1 && check_add_overflow(oldsize, size, &newsize)) {
> +		error("can't resize to larger than 16EiB");
> +		return -1;
> +	} else
> +		newsize = size;
> +
> +	if (newsize < SZ_256M) {
> +		error("can't resize to size smaller than 256MiB");
> +		return -1;
> +	}
> +	return 0;
> +}
> +
>  static const char * const cmd_filesystem_resize_usage[] = {
>  	"btrfs filesystem resize [devid:][+/-]<newsize>[kKmMgGtTpPeE]|[devid:]max <path>",
>  	"Resize a filesystem",
> @@ -1110,6 +1147,10 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd,
>  	if (fd < 0)
>  		return 1;
>  
> +	res = check_resize_size(path, amount);
> +	if (res < 0)
> +		return 1;
> +
>  	printf("Resize '%s' of '%s'\n", path, amount);
>  	memset(&args, 0, sizeof(args));
>  	strncpy_null(args.name, amount);
> diff --git a/common/utils.c b/common/utils.c
> index ad938409..f2a10ccc 100644
> --- a/common/utils.c
> +++ b/common/utils.c
> @@ -638,7 +638,7 @@ static int fls64(u64 x)
>  	return 64 - i;
>  }
>  
> -u64 parse_size(char *s)
> +u64 parse_size(const char *s)

Although a good change, not sure if David will ask for an explict patch
for that.

Despite that, looks good.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

>  {
>  	char c;
>  	char *endptr;
> diff --git a/common/utils.h b/common/utils.h
> index 7867fb0a..0ef1d6e8 100644
> --- a/common/utils.h
> +++ b/common/utils.h
> @@ -65,7 +65,7 @@ int pretty_size_snprintf(u64 size, char *str, size_t str_bytes, unsigned unit_mo
>  #define pretty_size(size) 	pretty_size_mode(size, UNITS_DEFAULT)
>  const char *pretty_size_mode(u64 size, unsigned mode);
>  
> -u64 parse_size(char *s);
> +u64 parse_size(const char *s);
>  u64 parse_qgroupid(const char *p);
>  u64 arg_strtou64(const char *str);
>  int open_file_or_dir(const char *fname, DIR **dirstream);
>
David Sterba Aug. 27, 2019, 3:22 p.m. UTC | #2
On Wed, Aug 14, 2019 at 09:53:33AM +0800, Qu Wenruo wrote:
> 
> 
> On 2019/8/14 上午9:04, Jeff Mahoney wrote:
> > If a user attempts to resize a file system to a size under 256MiB,
> > it will be rejected with EINVAL and get then unhelpful error message
> > "ERROR: unable to resize '/path': Invalid argument."
> > 
> > This commit performs that check before issuing the ioctl to report
> > a more sensible error message.   We also do overflow/underflow
> > checking when -/+ size is used and report those errors as well.
> > 
> > Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> > ---
> >  cmds/filesystem.c | 41 +++++++++++++++++++++++++++++++++++++++++
> >  common/utils.c    |  2 +-
> >  common/utils.h    |  2 +-
> >  3 files changed, 43 insertions(+), 2 deletions(-)
> > 
> > diff --git a/cmds/filesystem.c b/cmds/filesystem.c
> > index 4f22089a..e3415126 100644
> > --- a/cmds/filesystem.c
> > +++ b/cmds/filesystem.c
> > @@ -34,10 +34,12 @@
> >  #include "kerncompat.h"
> >  #include "ctree.h"
> >  #include "common/utils.h"
> > +#include "common/device-utils.h"
> >  #include "volumes.h"
> >  #include "cmds/commands.h"
> >  #include "cmds/filesystem-usage.h"
> >  #include "kernel-lib/list_sort.h"
> > +#include "kernel-lib/overflow.h"
> >  #include "disk-io.h"
> >  #include "common/help.h"
> >  #include "common/fsfeatures.h"
> > @@ -1062,6 +1064,41 @@ next:
> >  }
> >  static DEFINE_SIMPLE_COMMAND(filesystem_defrag, "defragment");
> >  
> > +static int check_resize_size(const char *path, const char *amount)
> > +{
> > +	int mod = 0;
> > +	u64 oldsize = 0, size, newsize;
> > +
> > +	if (*amount == '-')
> > +		mod = -1;
> > +	else if (*amount == '+')
> > +		mod = 1;
> > +
> > +	if (mod) {
> > +		amount++;
> > +		oldsize = disk_size(path);
> > +		if (oldsize == 0)
> > +			return -1;
> > +	}
> > +
> > +	size = parse_size(amount);
> > +
> > +	if (mod == -1 && check_sub_overflow(oldsize, size, &newsize)) {
> > +		error("can't resize to negative size");
> > +		return -1;
> > +	} else if (mod == 1 && check_add_overflow(oldsize, size, &newsize)) {
> > +		error("can't resize to larger than 16EiB");
> > +		return -1;
> > +	} else
> > +		newsize = size;
> > +
> > +	if (newsize < SZ_256M) {
> > +		error("can't resize to size smaller than 256MiB");
> > +		return -1;
> > +	}
> > +	return 0;
> > +}
> > +
> >  static const char * const cmd_filesystem_resize_usage[] = {
> >  	"btrfs filesystem resize [devid:][+/-]<newsize>[kKmMgGtTpPeE]|[devid:]max <path>",
> >  	"Resize a filesystem",
> > @@ -1110,6 +1147,10 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd,
> >  	if (fd < 0)
> >  		return 1;
> >  
> > +	res = check_resize_size(path, amount);
> > +	if (res < 0)
> > +		return 1;
> > +
> >  	printf("Resize '%s' of '%s'\n", path, amount);
> >  	memset(&args, 0, sizeof(args));
> >  	strncpy_null(args.name, amount);
> > diff --git a/common/utils.c b/common/utils.c
> > index ad938409..f2a10ccc 100644
> > --- a/common/utils.c
> > +++ b/common/utils.c
> > @@ -638,7 +638,7 @@ static int fls64(u64 x)
> >  	return 64 - i;
> >  }
> >  
> > -u64 parse_size(char *s)
> > +u64 parse_size(const char *s)
> 
> Although a good change, not sure if David will ask for an explict patch
> for that.

I've split that from the patch.

> Despite that, looks good.

Well, no. The resize specifier is more complex and has eg. formats like
1:+1G, max, 2:-2G, 3:max. Moreover the relative change must not be
compared to the 256M limit, 'resize -128M' fails while it should not for
a filesystem that's eg. 2G.

Patch
diff mbox series

diff --git a/cmds/filesystem.c b/cmds/filesystem.c
index 4f22089a..e3415126 100644
--- a/cmds/filesystem.c
+++ b/cmds/filesystem.c
@@ -34,10 +34,12 @@ 
 #include "kerncompat.h"
 #include "ctree.h"
 #include "common/utils.h"
+#include "common/device-utils.h"
 #include "volumes.h"
 #include "cmds/commands.h"
 #include "cmds/filesystem-usage.h"
 #include "kernel-lib/list_sort.h"
+#include "kernel-lib/overflow.h"
 #include "disk-io.h"
 #include "common/help.h"
 #include "common/fsfeatures.h"
@@ -1062,6 +1064,41 @@  next:
 }
 static DEFINE_SIMPLE_COMMAND(filesystem_defrag, "defragment");
 
+static int check_resize_size(const char *path, const char *amount)
+{
+	int mod = 0;
+	u64 oldsize = 0, size, newsize;
+
+	if (*amount == '-')
+		mod = -1;
+	else if (*amount == '+')
+		mod = 1;
+
+	if (mod) {
+		amount++;
+		oldsize = disk_size(path);
+		if (oldsize == 0)
+			return -1;
+	}
+
+	size = parse_size(amount);
+
+	if (mod == -1 && check_sub_overflow(oldsize, size, &newsize)) {
+		error("can't resize to negative size");
+		return -1;
+	} else if (mod == 1 && check_add_overflow(oldsize, size, &newsize)) {
+		error("can't resize to larger than 16EiB");
+		return -1;
+	} else
+		newsize = size;
+
+	if (newsize < SZ_256M) {
+		error("can't resize to size smaller than 256MiB");
+		return -1;
+	}
+	return 0;
+}
+
 static const char * const cmd_filesystem_resize_usage[] = {
 	"btrfs filesystem resize [devid:][+/-]<newsize>[kKmMgGtTpPeE]|[devid:]max <path>",
 	"Resize a filesystem",
@@ -1110,6 +1147,10 @@  static int cmd_filesystem_resize(const struct cmd_struct *cmd,
 	if (fd < 0)
 		return 1;
 
+	res = check_resize_size(path, amount);
+	if (res < 0)
+		return 1;
+
 	printf("Resize '%s' of '%s'\n", path, amount);
 	memset(&args, 0, sizeof(args));
 	strncpy_null(args.name, amount);
diff --git a/common/utils.c b/common/utils.c
index ad938409..f2a10ccc 100644
--- a/common/utils.c
+++ b/common/utils.c
@@ -638,7 +638,7 @@  static int fls64(u64 x)
 	return 64 - i;
 }
 
-u64 parse_size(char *s)
+u64 parse_size(const char *s)
 {
 	char c;
 	char *endptr;
diff --git a/common/utils.h b/common/utils.h
index 7867fb0a..0ef1d6e8 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -65,7 +65,7 @@  int pretty_size_snprintf(u64 size, char *str, size_t str_bytes, unsigned unit_mo
 #define pretty_size(size) 	pretty_size_mode(size, UNITS_DEFAULT)
 const char *pretty_size_mode(u64 size, unsigned mode);
 
-u64 parse_size(char *s);
+u64 parse_size(const char *s);
 u64 parse_qgroupid(const char *p);
 u64 arg_strtou64(const char *str);
 int open_file_or_dir(const char *fname, DIR **dirstream);