Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/5] btrfs-progs: mkfs: treat btrfs_add_to_fsid as fatal error
@ 2019-08-14  1:03 Jeff Mahoney
  2019-08-14  1:03 ` [PATCH 2/5] btrfs-progs: btrfs_add_to_fsid: check if adding device would overflow Jeff Mahoney
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Jeff Mahoney @ 2019-08-14  1:03 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Jeff Mahoney

When btrfs_add_to_fsid fails in mkfs we try to close the ctree.  That
complains that we already have a transaction open.  We should be taking
the error path and exit cleanly without writing.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 mkfs/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mkfs/main.c b/mkfs/main.c
index 971cb395..b752da13 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -1268,7 +1268,7 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
 					sectorsize, sectorsize, sectorsize);
 		if (ret) {
 			error("unable to add %s to filesystem: %d", file, ret);
-			goto out;
+			goto error;
 		}
 		if (verbose >= 2) {
 			struct btrfs_device *device;
-- 
2.16.4


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/5] btrfs-progs: btrfs_add_to_fsid: check if adding device would overflow
  2019-08-14  1:03 [PATCH 1/5] btrfs-progs: mkfs: treat btrfs_add_to_fsid as fatal error Jeff Mahoney
@ 2019-08-14  1:03 ` Jeff Mahoney
  2019-08-14  1:50   ` Qu Wenruo
  2019-08-14  1:04 ` [PATCH 3/5] btrfs-progs: qgroups: use parse_size instead of open coding it Jeff Mahoney
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: Jeff Mahoney @ 2019-08-14  1:03 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Jeff Mahoney

It's theoretically possible to add multiple devices with sizes that add up
to or exceed 16EiB.  A file system will be created successfully but will
have a superblock with incorrect values for total_bytes and other fields.

Kernels up to v5.0 will crash when they encounter this scenario.

We need to check for overflow and reject the device if it would overflow.
I've copied include/linux/overflow.h from the kernel to reuse that code.

Link: https://bugzilla.suse.com/show_bug.cgi?id=1099147
Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 common/device-scan.c                              |  15 +-
 kernel-lib/overflow.h                             | 270 ++++++++++++++++++++++
 tests/mkfs-tests/018-multidevice-overflow/test.sh |  22 ++
 3 files changed, 304 insertions(+), 3 deletions(-)
 create mode 100644 kernel-lib/overflow.h
 create mode 100755 tests/mkfs-tests/018-multidevice-overflow/test.sh

diff --git a/common/device-scan.c b/common/device-scan.c
index 2c5ae225..09d90add 100644
--- a/common/device-scan.c
+++ b/common/device-scan.c
@@ -26,6 +26,7 @@
 #include <linux/limits.h>
 #include <blkid/blkid.h>
 #include <uuid/uuid.h>
+#include "kernel-lib/overflow.h"
 #include "common/path-utils.h"
 #include "common/device-scan.h"
 #include "common/messages.h"
@@ -118,7 +119,8 @@ int btrfs_add_to_fsid(struct btrfs_trans_handle *trans,
 	struct btrfs_device *device;
 	struct btrfs_dev_item *dev_item;
 	char *buf = NULL;
-	u64 fs_total_bytes;
+	u64 old_size = btrfs_super_total_bytes(super);
+	u64 new_size;
 	u64 num_devs;
 	int ret;
 
@@ -156,13 +158,20 @@ int btrfs_add_to_fsid(struct btrfs_trans_handle *trans,
 		goto out;
 	}
 
+	if (check_add_overflow(old_size, device_total_bytes, &new_size)) {
+		error(
+"adding device of %llu bytes would exceed max file system size.",
+		      device->total_bytes);
+		ret = -EOVERFLOW;
+		goto out;
+	}
+
 	INIT_LIST_HEAD(&device->dev_list);
 	ret = btrfs_add_device(trans, fs_info, device);
 	if (ret)
 		goto out;
 
-	fs_total_bytes = btrfs_super_total_bytes(super) + device_total_bytes;
-	btrfs_set_super_total_bytes(super, fs_total_bytes);
+	btrfs_set_super_total_bytes(super, new_size);
 
 	num_devs = btrfs_super_num_devices(super) + 1;
 	btrfs_set_super_num_devices(super, num_devs);
diff --git a/kernel-lib/overflow.h b/kernel-lib/overflow.h
new file mode 100644
index 00000000..ab7f5d0e
--- /dev/null
+++ b/kernel-lib/overflow.h
@@ -0,0 +1,270 @@
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+#ifndef __LINUX_OVERFLOW_H
+#define __LINUX_OVERFLOW_H
+
+/*
+ * It would seem more obvious to do something like
+ *
+ * #define type_min(T) (T)(is_signed_type(T) ? (T)1 << (8*sizeof(T)-1) : 0)
+ * #define type_max(T) (T)(is_signed_type(T) ? ((T)1 << (8*sizeof(T)-1)) - 1 : ~(T)0)
+ *
+ * Unfortunately, the middle expressions, strictly speaking, have
+ * undefined behaviour, and at least some versions of gcc warn about
+ * the type_max expression (but not if -fsanitize=undefined is in
+ * effect; in that case, the warning is deferred to runtime...).
+ *
+ * The slightly excessive casting in type_min is to make sure the
+ * macros also produce sensible values for the exotic type _Bool. [The
+ * overflow checkers only almost work for _Bool, but that's
+ * a-feature-not-a-bug, since people shouldn't be doing arithmetic on
+ * _Bools. Besides, the gcc builtins don't allow _Bool* as third
+ * argument.]
+ *
+ * Idea stolen from
+ * https://mail-index.netbsd.org/tech-misc/2007/02/05/0000.html -
+ * credit to Christian Biere.
+ */
+#define is_signed_type(type)       (((type)(-1)) < (type)1)
+#define __type_half_max(type) ((type)1 << (8*sizeof(type) - 1 - is_signed_type(type)))
+#define type_max(T) ((T)((__type_half_max(T) - 1) + __type_half_max(T)))
+#define type_min(T) ((T)((T)-type_max(T)-(T)1))
+
+/*
+ * Avoids triggering -Wtype-limits compilation warning,
+ * while using unsigned data types to check a < 0.
+ */
+#define is_non_negative(a) ((a) > 0 || (a) == 0)
+#define is_negative(a) (!(is_non_negative(a)))
+
+/* Checking for unsigned overflow is relatively easy without causing UB. */
+#define __unsigned_add_overflow(a, b, d) ({	\
+	typeof(a) __a = (a);			\
+	typeof(b) __b = (b);			\
+	typeof(d) __d = (d);			\
+	(void) (&__a == &__b);			\
+	(void) (&__a == __d);			\
+	*__d = __a + __b;			\
+	*__d < __a;				\
+})
+#define __unsigned_sub_overflow(a, b, d) ({	\
+	typeof(a) __a = (a);			\
+	typeof(b) __b = (b);			\
+	typeof(d) __d = (d);			\
+	(void) (&__a == &__b);			\
+	(void) (&__a == __d);			\
+	*__d = __a - __b;			\
+	__a < __b;				\
+})
+/*
+ * If one of a or b is a compile-time constant, this avoids a division.
+ */
+#define __unsigned_mul_overflow(a, b, d) ({		\
+	typeof(a) __a = (a);				\
+	typeof(b) __b = (b);				\
+	typeof(d) __d = (d);				\
+	(void) (&__a == &__b);				\
+	(void) (&__a == __d);				\
+	*__d = __a * __b;				\
+	__builtin_constant_p(__b) ?			\
+	  __b > 0 && __a > type_max(typeof(__a)) / __b : \
+	  __a > 0 && __b > type_max(typeof(__b)) / __a;	 \
+})
+
+/*
+ * For signed types, detecting overflow is much harder, especially if
+ * we want to avoid UB. But the interface of these macros is such that
+ * we must provide a result in *d, and in fact we must produce the
+ * result promised by gcc's builtins, which is simply the possibly
+ * wrapped-around value. Fortunately, we can just formally do the
+ * operations in the widest relevant unsigned type (u64) and then
+ * truncate the result - gcc is smart enough to generate the same code
+ * with and without the (u64) casts.
+ */
+
+/*
+ * Adding two signed integers can overflow only if they have the same
+ * sign, and overflow has happened iff the result has the opposite
+ * sign.
+ */
+#define __signed_add_overflow(a, b, d) ({	\
+	typeof(a) __a = (a);			\
+	typeof(b) __b = (b);			\
+	typeof(d) __d = (d);			\
+	(void) (&__a == &__b);			\
+	(void) (&__a == __d);			\
+	*__d = (u64)__a + (u64)__b;		\
+	(((~(__a ^ __b)) & (*__d ^ __a))	\
+		& type_min(typeof(__a))) != 0;	\
+})
+
+/*
+ * Subtraction is similar, except that overflow can now happen only
+ * when the signs are opposite. In this case, overflow has happened if
+ * the result has the opposite sign of a.
+ */
+#define __signed_sub_overflow(a, b, d) ({	\
+	typeof(a) __a = (a);			\
+	typeof(b) __b = (b);			\
+	typeof(d) __d = (d);			\
+	(void) (&__a == &__b);			\
+	(void) (&__a == __d);			\
+	*__d = (u64)__a - (u64)__b;		\
+	((((__a ^ __b)) & (*__d ^ __a))		\
+		& type_min(typeof(__a))) != 0;	\
+})
+
+/*
+ * Signed multiplication is rather hard. gcc always follows C99, so
+ * division is truncated towards 0. This means that we can write the
+ * overflow check like this:
+ *
+ * (a > 0 && (b > MAX/a || b < MIN/a)) ||
+ * (a < -1 && (b > MIN/a || b < MAX/a) ||
+ * (a == -1 && b == MIN)
+ *
+ * The redundant casts of -1 are to silence an annoying -Wtype-limits
+ * (included in -Wextra) warning: When the type is u8 or u16, the
+ * __b_c_e in check_mul_overflow obviously selects
+ * __unsigned_mul_overflow, but unfortunately gcc still parses this
+ * code and warns about the limited range of __b.
+ */
+
+#define __signed_mul_overflow(a, b, d) ({				\
+	typeof(a) __a = (a);						\
+	typeof(b) __b = (b);						\
+	typeof(d) __d = (d);						\
+	typeof(a) __tmax = type_max(typeof(a));				\
+	typeof(a) __tmin = type_min(typeof(a));				\
+	(void) (&__a == &__b);						\
+	(void) (&__a == __d);						\
+	*__d = (u64)__a * (u64)__b;					\
+	(__b > 0   && (__a > __tmax/__b || __a < __tmin/__b)) ||	\
+	(__b < (typeof(__b))-1  && (__a > __tmin/__b || __a < __tmax/__b)) || \
+	(__b == (typeof(__b))-1 && __a == __tmin);			\
+})
+
+
+#define check_add_overflow(a, b, d)					\
+	__builtin_choose_expr(is_signed_type(typeof(a)),		\
+			__signed_add_overflow(a, b, d),			\
+			__unsigned_add_overflow(a, b, d))
+
+#define check_sub_overflow(a, b, d)					\
+	__builtin_choose_expr(is_signed_type(typeof(a)),		\
+			__signed_sub_overflow(a, b, d),			\
+			__unsigned_sub_overflow(a, b, d))
+
+#define check_mul_overflow(a, b, d)					\
+	__builtin_choose_expr(is_signed_type(typeof(a)),		\
+			__signed_mul_overflow(a, b, d),			\
+			__unsigned_mul_overflow(a, b, d))
+
+/** check_shl_overflow() - Calculate a left-shifted value and check overflow
+ *
+ * @a: Value to be shifted
+ * @s: How many bits left to shift
+ * @d: Pointer to where to store the result
+ *
+ * Computes *@d = (@a << @s)
+ *
+ * Returns true if '*d' cannot hold the result or when 'a << s' doesn't
+ * make sense. Example conditions:
+ * - 'a << s' causes bits to be lost when stored in *d.
+ * - 's' is garbage (e.g. negative) or so large that the result of
+ *   'a << s' is guaranteed to be 0.
+ * - 'a' is negative.
+ * - 'a << s' sets the sign bit, if any, in '*d'.
+ *
+ * '*d' will hold the results of the attempted shift, but is not
+ * considered "safe for use" if false is returned.
+ */
+#define check_shl_overflow(a, s, d) ({					\
+	typeof(a) _a = a;						\
+	typeof(s) _s = s;						\
+	typeof(d) _d = d;						\
+	u64 _a_full = _a;						\
+	unsigned int _to_shift =					\
+		is_non_negative(_s) && _s < 8 * sizeof(*d) ? _s : 0;	\
+	*_d = (_a_full << _to_shift);					\
+	(_to_shift != _s || is_negative(*_d) || is_negative(_a) ||	\
+	(*_d >> _to_shift) != _a);					\
+})
+
+/**
+ * array_size() - Calculate size of 2-dimensional array.
+ *
+ * @a: dimension one
+ * @b: dimension two
+ *
+ * Calculates size of 2-dimensional array: @a * @b.
+ *
+ * Returns: number of bytes needed to represent the array or SIZE_MAX on
+ * overflow.
+ */
+static inline size_t array_size(size_t a, size_t b)
+{
+	size_t bytes;
+
+	if (check_mul_overflow(a, b, &bytes))
+		return SIZE_MAX;
+
+	return bytes;
+}
+
+/**
+ * array3_size() - Calculate size of 3-dimensional array.
+ *
+ * @a: dimension one
+ * @b: dimension two
+ * @c: dimension three
+ *
+ * Calculates size of 3-dimensional array: @a * @b * @c.
+ *
+ * Returns: number of bytes needed to represent the array or SIZE_MAX on
+ * overflow.
+ */
+static inline size_t array3_size(size_t a, size_t b, size_t c)
+{
+	size_t bytes;
+
+	if (check_mul_overflow(a, b, &bytes))
+		return SIZE_MAX;
+	if (check_mul_overflow(bytes, c, &bytes))
+		return SIZE_MAX;
+
+	return bytes;
+}
+
+/*
+ * Compute a*b+c, returning SIZE_MAX on overflow. Internal helper for
+ * struct_size() below.
+ */
+static inline size_t __ab_c_size(size_t a, size_t b, size_t c)
+{
+	size_t bytes;
+
+	if (check_mul_overflow(a, b, &bytes))
+		return SIZE_MAX;
+	if (check_add_overflow(bytes, c, &bytes))
+		return SIZE_MAX;
+
+	return bytes;
+}
+
+/**
+ * struct_size() - Calculate size of structure with trailing array.
+ * @p: Pointer to the structure.
+ * @member: Name of the array member.
+ * @n: Number of elements in the array.
+ *
+ * Calculates size of memory needed for structure @p followed by an
+ * array of @n @member elements.
+ *
+ * Return: number of bytes needed or SIZE_MAX on overflow.
+ */
+#define struct_size(p, member, n)					\
+	__ab_c_size(n,							\
+		    sizeof(*(p)->member) + __must_be_array((p)->member),\
+		    sizeof(*(p)))
+
+#endif /* __LINUX_OVERFLOW_H */
diff --git a/tests/mkfs-tests/018-multidevice-overflow/test.sh b/tests/mkfs-tests/018-multidevice-overflow/test.sh
new file mode 100755
index 00000000..0b550685
--- /dev/null
+++ b/tests/mkfs-tests/018-multidevice-overflow/test.sh
@@ -0,0 +1,22 @@
+#!/bin/bash
+# test if mkfs.btrfs will create file systems that overflow total_bytes
+
+source "$TEST_TOP/common"
+
+check_prereq mkfs.btrfs
+check_prereq btrfs
+
+mkdev()
+{
+	truncate -s 0 "$1"
+	truncate -s "$2" "$1"
+}
+
+mkdev "$TEST_TOP/test-dev1.img" 6E
+mkdev "$TEST_TOP/test-dev2.img" 6E
+mkdev "$TEST_TOP/test-dev3.img" 6E
+
+run_mustfail "mkfs.btrfs for too-large images" \
+	     "$TOP/mkfs.btrfs" -f "$TEST_TOP"/test-dev[123].img
+
+rm -f "$TEST_TOP"/test-dev[123].img
-- 
2.16.4


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 3/5] btrfs-progs: qgroups: use parse_size instead of open coding it
  2019-08-14  1:03 [PATCH 1/5] btrfs-progs: mkfs: treat btrfs_add_to_fsid as fatal error Jeff Mahoney
  2019-08-14  1:03 ` [PATCH 2/5] btrfs-progs: btrfs_add_to_fsid: check if adding device would overflow Jeff Mahoney
@ 2019-08-14  1:04 ` Jeff Mahoney
  2019-08-14  1:51   ` Qu Wenruo
  2019-08-14  1:04 ` [PATCH 4/5] btrfs-progs: resize: more sensible error messages for bad sizes Jeff Mahoney
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: Jeff Mahoney @ 2019-08-14  1:04 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Jeff Mahoney

The only difference between parse_limit and parse_size is that
parse_limit accepts "none" as a valid input.  That's easy enough
to handle as a special case and lets us drop the duplicate code.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 cmds/qgroup.c | 58 ++++------------------------------------------------------
 1 file changed, 4 insertions(+), 54 deletions(-)

diff --git a/cmds/qgroup.c b/cmds/qgroup.c
index 59b43c98..ba81052a 100644
--- a/cmds/qgroup.c
+++ b/cmds/qgroup.c
@@ -159,56 +159,6 @@ static int _cmd_qgroup_create(int create, int argc, char **argv)
 	return 0;
 }
 
-static int parse_limit(const char *p, unsigned long long *s)
-{
-	char *endptr;
-	unsigned long long size;
-	unsigned long long CLEAR_VALUE = -1;
-
-	if (strcasecmp(p, "none") == 0) {
-		*s = CLEAR_VALUE;
-		return 1;
-	}
-
-	if (p[0] == '-')
-		return 0;
-
-	size = strtoull(p, &endptr, 10);
-	if (p == endptr)
-		return 0;
-
-	switch (*endptr) {
-	case 'T':
-	case 't':
-		size *= 1024;
-		/* fallthrough */
-	case 'G':
-	case 'g':
-		size *= 1024;
-		/* fallthrough */
-	case 'M':
-	case 'm':
-		size *= 1024;
-		/* fallthrough */
-	case 'K':
-	case 'k':
-		size *= 1024;
-		++endptr;
-		break;
-	case 0:
-		break;
-	default:
-		return 0;
-	}
-
-	if (*endptr)
-		return 0;
-
-	*s = size;
-
-	return 1;
-}
-
 static const char * const cmd_qgroup_assign_usage[] = {
 	"btrfs qgroup assign [options] <src> <dst> <path>",
 	"Assign SRC as the child qgroup of DST",
@@ -455,10 +405,10 @@ static int cmd_qgroup_limit(const struct cmd_struct *cmd, int argc, char **argv)
 	if (check_argc_min(argc - optind, 2))
 		return 1;
 
-	if (!parse_limit(argv[optind], &size)) {
-		error("invalid size argument: %s", argv[optind]);
-		return 1;
-	}
+	if (!strcasecmp(argv[optind], "none"))
+		size = -1ULL;
+	else
+		size = parse_size(argv[optind]);
 
 	memset(&args, 0, sizeof(args));
 	if (compressed)
-- 
2.16.4


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 4/5] btrfs-progs: resize: more sensible error messages for bad sizes
  2019-08-14  1:03 [PATCH 1/5] btrfs-progs: mkfs: treat btrfs_add_to_fsid as fatal error Jeff Mahoney
  2019-08-14  1:03 ` [PATCH 2/5] btrfs-progs: btrfs_add_to_fsid: check if adding device would overflow Jeff Mahoney
  2019-08-14  1:04 ` [PATCH 3/5] btrfs-progs: qgroups: use parse_size instead of open coding it Jeff Mahoney
@ 2019-08-14  1:04 ` Jeff Mahoney
  2019-08-14  1:53   ` Qu Wenruo
  2019-08-14  1:04 ` [PATCH 5/5] btrfs-progs: mkfs: print error messages instead of just error number Jeff Mahoney
  2019-08-14  1:48 ` [PATCH 1/5] btrfs-progs: mkfs: treat btrfs_add_to_fsid as fatal error Qu Wenruo
  4 siblings, 1 reply; 11+ messages in thread
From: Jeff Mahoney @ 2019-08-14  1:04 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Jeff Mahoney

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)
 {
 	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);
-- 
2.16.4


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 5/5] btrfs-progs: mkfs: print error messages instead of just error number
  2019-08-14  1:03 [PATCH 1/5] btrfs-progs: mkfs: treat btrfs_add_to_fsid as fatal error Jeff Mahoney
                   ` (2 preceding siblings ...)
  2019-08-14  1:04 ` [PATCH 4/5] btrfs-progs: resize: more sensible error messages for bad sizes Jeff Mahoney
@ 2019-08-14  1:04 ` Jeff Mahoney
  2019-08-14  1:54   ` Qu Wenruo
  2019-08-14  1:48 ` [PATCH 1/5] btrfs-progs: mkfs: treat btrfs_add_to_fsid as fatal error Qu Wenruo
  4 siblings, 1 reply; 11+ messages in thread
From: Jeff Mahoney @ 2019-08-14  1:04 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Jeff Mahoney

Printing the error number means having to go look up what that error
number means.  For a developer, it's easy.  For a user, it's unhelpful.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 mkfs/main.c | 47 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 17 deletions(-)

diff --git a/mkfs/main.c b/mkfs/main.c
index b752da13..7bfeb610 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -1197,37 +1197,43 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
 
 	ret = create_metadata_block_groups(root, mixed, &allocation);
 	if (ret) {
-		error("failed to create default block groups: %d", ret);
+		error("failed to create default block groups: %d/%s",
+		      ret, strerror(-ret));
 		goto error;
 	}
 
 	trans = btrfs_start_transaction(root, 1);
 	if (IS_ERR(trans)) {
-		error("failed to start transaction");
+		error("failed to start transaction: %ld/%s",
+		      PTR_ERR(trans), strerror(-PTR_ERR(trans)));
 		goto error;
 	}
 
 	ret = create_data_block_groups(trans, root, mixed, &allocation);
 	if (ret) {
-		error("failed to create default data block groups: %d", ret);
+		error("failed to create default data block groups: %d/%s",
+		      ret, strerror(-ret));
 		goto error;
 	}
 
 	ret = make_root_dir(trans, root);
 	if (ret) {
-		error("failed to setup the root directory: %d", ret);
+		error("failed to setup the root directory: %d/%s",
+		      ret, strerror(-ret));
 		goto error;
 	}
 
 	ret = btrfs_commit_transaction(trans, root);
 	if (ret) {
-		error("unable to commit transaction: %d", ret);
+		error("unable to commit transaction: %d/%s",
+		      ret, strerror(-ret));
 		goto out;
 	}
 
 	trans = btrfs_start_transaction(root, 1);
 	if (IS_ERR(trans)) {
-		error("failed to start transaction");
+		error("failed to start transaction: %ld/%s",
+		      PTR_ERR(trans), strerror(-PTR_ERR(trans)));
 		goto error;
 	}
 
@@ -1267,7 +1273,7 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
 		ret = btrfs_add_to_fsid(trans, root, fd, file, dev_block_count,
 					sectorsize, sectorsize, sectorsize);
 		if (ret) {
-			error("unable to add %s to filesystem: %d", file, ret);
+			error("unable to add %s to filesystem: %d/%s", file, ret, strerror(-ret));
 			goto error;
 		}
 		if (verbose >= 2) {
@@ -1284,46 +1290,52 @@ raid_groups:
 	ret = create_raid_groups(trans, root, data_profile,
 			 metadata_profile, mixed, &allocation);
 	if (ret) {
-		error("unable to create raid groups: %d", ret);
+		error("unable to create raid groups: %d/%s",
+		      ret, strerror(-ret));
 		goto out;
 	}
 
 	ret = create_data_reloc_tree(trans);
 	if (ret) {
-		error("unable to create data reloc tree: %d", ret);
+		error("unable to create data reloc tree: %d/%s",
+		      ret, strerror(-ret));
 		goto out;
 	}
 
 	ret = create_uuid_tree(trans);
 	if (ret)
 		warning(
-	"unable to create uuid tree, will be created after mount: %d", ret);
+	"unable to create uuid tree, will be created after mount: %d/%s",
+			ret, strerror(-ret));
 
 	ret = btrfs_commit_transaction(trans, root);
 	if (ret) {
-		error("unable to commit transaction: %d", ret);
+		error("unable to commit transaction: %d/%s",
+		      ret, strerror(-ret));
 		goto out;
 	}
 
 	ret = cleanup_temp_chunks(fs_info, &allocation, data_profile,
 				  metadata_profile, metadata_profile);
 	if (ret < 0) {
-		error("failed to cleanup temporary chunks: %d", ret);
+		error("failed to cleanup temporary chunks: %d/%s",
+		      ret, strerror(-ret));
 		goto out;
 	}
 
 	if (source_dir_set) {
 		ret = btrfs_mkfs_fill_dir(source_dir, root, verbose);
 		if (ret) {
-			error("error while filling filesystem: %d", ret);
+			error("error while filling filesystem: %d/%s",
+			      ret, strerror(-ret));
 			goto out;
 		}
 		if (shrink_rootdir) {
 			ret = btrfs_mkfs_shrink_fs(fs_info, &shrink_size,
 						   shrink_rootdir);
 			if (ret < 0) {
-				error("error while shrinking filesystem: %d",
-					ret);
+				error("error while shrinking filesystem: %d/%s",
+					ret, strerror(-ret));
 				goto out;
 			}
 		}
@@ -1383,8 +1395,9 @@ out:
 
 	if (!ret && close_ret) {
 		ret = close_ret;
-		error("failed to close ctree, the filesystem may be inconsistent: %d",
-		      ret);
+		error(
+	"failed to close ctree, the filesystem may be inconsistent: %d/%s",
+		      ret, strerror(-ret));
 	}
 
 	btrfs_close_all_devices();
-- 
2.16.4


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/5] btrfs-progs: mkfs: treat btrfs_add_to_fsid as fatal error
  2019-08-14  1:03 [PATCH 1/5] btrfs-progs: mkfs: treat btrfs_add_to_fsid as fatal error Jeff Mahoney
                   ` (3 preceding siblings ...)
  2019-08-14  1:04 ` [PATCH 5/5] btrfs-progs: mkfs: print error messages instead of just error number Jeff Mahoney
@ 2019-08-14  1:48 ` Qu Wenruo
  4 siblings, 0 replies; 11+ messages in thread
From: Qu Wenruo @ 2019-08-14  1:48 UTC (permalink / raw)
  To: Jeff Mahoney, linux-btrfs

[-- Attachment #1.1: Type: text/plain, Size: 868 bytes --]



On 2019/8/14 上午9:03, Jeff Mahoney wrote:
> When btrfs_add_to_fsid fails in mkfs we try to close the ctree.  That
> complains that we already have a transaction open.  We should be taking
> the error path and exit cleanly without writing.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>

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

Thanks,
Qu

> ---
>  mkfs/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mkfs/main.c b/mkfs/main.c
> index 971cb395..b752da13 100644
> --- a/mkfs/main.c
> +++ b/mkfs/main.c
> @@ -1268,7 +1268,7 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>  					sectorsize, sectorsize, sectorsize);
>  		if (ret) {
>  			error("unable to add %s to filesystem: %d", file, ret);
> -			goto out;
> +			goto error;
>  		}
>  		if (verbose >= 2) {
>  			struct btrfs_device *device;
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/5] btrfs-progs: btrfs_add_to_fsid: check if adding device would overflow
  2019-08-14  1:03 ` [PATCH 2/5] btrfs-progs: btrfs_add_to_fsid: check if adding device would overflow Jeff Mahoney
@ 2019-08-14  1:50   ` Qu Wenruo
  0 siblings, 0 replies; 11+ messages in thread
From: Qu Wenruo @ 2019-08-14  1:50 UTC (permalink / raw)
  To: Jeff Mahoney, linux-btrfs

[-- Attachment #1.1: Type: text/plain, Size: 13054 bytes --]



On 2019/8/14 上午9:03, Jeff Mahoney wrote:
> It's theoretically possible to add multiple devices with sizes that add up
> to or exceed 16EiB.  A file system will be created successfully but will
> have a superblock with incorrect values for total_bytes and other fields.
> 
> Kernels up to v5.0 will crash when they encounter this scenario.
> 
> We need to check for overflow and reject the device if it would overflow.
> I've copied include/linux/overflow.h from the kernel to reuse that code.

That would be a pretty good base for later overflow check.

> 
> Link: https://bugzilla.suse.com/show_bug.cgi?id=1099147
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>

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

Thanks,
Qu

> ---
>  common/device-scan.c                              |  15 +-
>  kernel-lib/overflow.h                             | 270 ++++++++++++++++++++++
>  tests/mkfs-tests/018-multidevice-overflow/test.sh |  22 ++
>  3 files changed, 304 insertions(+), 3 deletions(-)
>  create mode 100644 kernel-lib/overflow.h
>  create mode 100755 tests/mkfs-tests/018-multidevice-overflow/test.sh
> 
> diff --git a/common/device-scan.c b/common/device-scan.c
> index 2c5ae225..09d90add 100644
> --- a/common/device-scan.c
> +++ b/common/device-scan.c
> @@ -26,6 +26,7 @@
>  #include <linux/limits.h>
>  #include <blkid/blkid.h>
>  #include <uuid/uuid.h>
> +#include "kernel-lib/overflow.h"
>  #include "common/path-utils.h"
>  #include "common/device-scan.h"
>  #include "common/messages.h"
> @@ -118,7 +119,8 @@ int btrfs_add_to_fsid(struct btrfs_trans_handle *trans,
>  	struct btrfs_device *device;
>  	struct btrfs_dev_item *dev_item;
>  	char *buf = NULL;
> -	u64 fs_total_bytes;
> +	u64 old_size = btrfs_super_total_bytes(super);
> +	u64 new_size;
>  	u64 num_devs;
>  	int ret;
>  
> @@ -156,13 +158,20 @@ int btrfs_add_to_fsid(struct btrfs_trans_handle *trans,
>  		goto out;
>  	}
>  
> +	if (check_add_overflow(old_size, device_total_bytes, &new_size)) {
> +		error(
> +"adding device of %llu bytes would exceed max file system size.",
> +		      device->total_bytes);
> +		ret = -EOVERFLOW;
> +		goto out;
> +	}
> +
>  	INIT_LIST_HEAD(&device->dev_list);
>  	ret = btrfs_add_device(trans, fs_info, device);
>  	if (ret)
>  		goto out;
>  
> -	fs_total_bytes = btrfs_super_total_bytes(super) + device_total_bytes;
> -	btrfs_set_super_total_bytes(super, fs_total_bytes);
> +	btrfs_set_super_total_bytes(super, new_size);
>  
>  	num_devs = btrfs_super_num_devices(super) + 1;
>  	btrfs_set_super_num_devices(super, num_devs);
> diff --git a/kernel-lib/overflow.h b/kernel-lib/overflow.h
> new file mode 100644
> index 00000000..ab7f5d0e
> --- /dev/null
> +++ b/kernel-lib/overflow.h
> @@ -0,0 +1,270 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +#ifndef __LINUX_OVERFLOW_H
> +#define __LINUX_OVERFLOW_H
> +
> +/*
> + * It would seem more obvious to do something like
> + *
> + * #define type_min(T) (T)(is_signed_type(T) ? (T)1 << (8*sizeof(T)-1) : 0)
> + * #define type_max(T) (T)(is_signed_type(T) ? ((T)1 << (8*sizeof(T)-1)) - 1 : ~(T)0)
> + *
> + * Unfortunately, the middle expressions, strictly speaking, have
> + * undefined behaviour, and at least some versions of gcc warn about
> + * the type_max expression (but not if -fsanitize=undefined is in
> + * effect; in that case, the warning is deferred to runtime...).
> + *
> + * The slightly excessive casting in type_min is to make sure the
> + * macros also produce sensible values for the exotic type _Bool. [The
> + * overflow checkers only almost work for _Bool, but that's
> + * a-feature-not-a-bug, since people shouldn't be doing arithmetic on
> + * _Bools. Besides, the gcc builtins don't allow _Bool* as third
> + * argument.]
> + *
> + * Idea stolen from
> + * https://mail-index.netbsd.org/tech-misc/2007/02/05/0000.html -
> + * credit to Christian Biere.
> + */
> +#define is_signed_type(type)       (((type)(-1)) < (type)1)
> +#define __type_half_max(type) ((type)1 << (8*sizeof(type) - 1 - is_signed_type(type)))
> +#define type_max(T) ((T)((__type_half_max(T) - 1) + __type_half_max(T)))
> +#define type_min(T) ((T)((T)-type_max(T)-(T)1))
> +
> +/*
> + * Avoids triggering -Wtype-limits compilation warning,
> + * while using unsigned data types to check a < 0.
> + */
> +#define is_non_negative(a) ((a) > 0 || (a) == 0)
> +#define is_negative(a) (!(is_non_negative(a)))
> +
> +/* Checking for unsigned overflow is relatively easy without causing UB. */
> +#define __unsigned_add_overflow(a, b, d) ({	\
> +	typeof(a) __a = (a);			\
> +	typeof(b) __b = (b);			\
> +	typeof(d) __d = (d);			\
> +	(void) (&__a == &__b);			\
> +	(void) (&__a == __d);			\
> +	*__d = __a + __b;			\
> +	*__d < __a;				\
> +})
> +#define __unsigned_sub_overflow(a, b, d) ({	\
> +	typeof(a) __a = (a);			\
> +	typeof(b) __b = (b);			\
> +	typeof(d) __d = (d);			\
> +	(void) (&__a == &__b);			\
> +	(void) (&__a == __d);			\
> +	*__d = __a - __b;			\
> +	__a < __b;				\
> +})
> +/*
> + * If one of a or b is a compile-time constant, this avoids a division.
> + */
> +#define __unsigned_mul_overflow(a, b, d) ({		\
> +	typeof(a) __a = (a);				\
> +	typeof(b) __b = (b);				\
> +	typeof(d) __d = (d);				\
> +	(void) (&__a == &__b);				\
> +	(void) (&__a == __d);				\
> +	*__d = __a * __b;				\
> +	__builtin_constant_p(__b) ?			\
> +	  __b > 0 && __a > type_max(typeof(__a)) / __b : \
> +	  __a > 0 && __b > type_max(typeof(__b)) / __a;	 \
> +})
> +
> +/*
> + * For signed types, detecting overflow is much harder, especially if
> + * we want to avoid UB. But the interface of these macros is such that
> + * we must provide a result in *d, and in fact we must produce the
> + * result promised by gcc's builtins, which is simply the possibly
> + * wrapped-around value. Fortunately, we can just formally do the
> + * operations in the widest relevant unsigned type (u64) and then
> + * truncate the result - gcc is smart enough to generate the same code
> + * with and without the (u64) casts.
> + */
> +
> +/*
> + * Adding two signed integers can overflow only if they have the same
> + * sign, and overflow has happened iff the result has the opposite
> + * sign.
> + */
> +#define __signed_add_overflow(a, b, d) ({	\
> +	typeof(a) __a = (a);			\
> +	typeof(b) __b = (b);			\
> +	typeof(d) __d = (d);			\
> +	(void) (&__a == &__b);			\
> +	(void) (&__a == __d);			\
> +	*__d = (u64)__a + (u64)__b;		\
> +	(((~(__a ^ __b)) & (*__d ^ __a))	\
> +		& type_min(typeof(__a))) != 0;	\
> +})
> +
> +/*
> + * Subtraction is similar, except that overflow can now happen only
> + * when the signs are opposite. In this case, overflow has happened if
> + * the result has the opposite sign of a.
> + */
> +#define __signed_sub_overflow(a, b, d) ({	\
> +	typeof(a) __a = (a);			\
> +	typeof(b) __b = (b);			\
> +	typeof(d) __d = (d);			\
> +	(void) (&__a == &__b);			\
> +	(void) (&__a == __d);			\
> +	*__d = (u64)__a - (u64)__b;		\
> +	((((__a ^ __b)) & (*__d ^ __a))		\
> +		& type_min(typeof(__a))) != 0;	\
> +})
> +
> +/*
> + * Signed multiplication is rather hard. gcc always follows C99, so
> + * division is truncated towards 0. This means that we can write the
> + * overflow check like this:
> + *
> + * (a > 0 && (b > MAX/a || b < MIN/a)) ||
> + * (a < -1 && (b > MIN/a || b < MAX/a) ||
> + * (a == -1 && b == MIN)
> + *
> + * The redundant casts of -1 are to silence an annoying -Wtype-limits
> + * (included in -Wextra) warning: When the type is u8 or u16, the
> + * __b_c_e in check_mul_overflow obviously selects
> + * __unsigned_mul_overflow, but unfortunately gcc still parses this
> + * code and warns about the limited range of __b.
> + */
> +
> +#define __signed_mul_overflow(a, b, d) ({				\
> +	typeof(a) __a = (a);						\
> +	typeof(b) __b = (b);						\
> +	typeof(d) __d = (d);						\
> +	typeof(a) __tmax = type_max(typeof(a));				\
> +	typeof(a) __tmin = type_min(typeof(a));				\
> +	(void) (&__a == &__b);						\
> +	(void) (&__a == __d);						\
> +	*__d = (u64)__a * (u64)__b;					\
> +	(__b > 0   && (__a > __tmax/__b || __a < __tmin/__b)) ||	\
> +	(__b < (typeof(__b))-1  && (__a > __tmin/__b || __a < __tmax/__b)) || \
> +	(__b == (typeof(__b))-1 && __a == __tmin);			\
> +})
> +
> +
> +#define check_add_overflow(a, b, d)					\
> +	__builtin_choose_expr(is_signed_type(typeof(a)),		\
> +			__signed_add_overflow(a, b, d),			\
> +			__unsigned_add_overflow(a, b, d))
> +
> +#define check_sub_overflow(a, b, d)					\
> +	__builtin_choose_expr(is_signed_type(typeof(a)),		\
> +			__signed_sub_overflow(a, b, d),			\
> +			__unsigned_sub_overflow(a, b, d))
> +
> +#define check_mul_overflow(a, b, d)					\
> +	__builtin_choose_expr(is_signed_type(typeof(a)),		\
> +			__signed_mul_overflow(a, b, d),			\
> +			__unsigned_mul_overflow(a, b, d))
> +
> +/** check_shl_overflow() - Calculate a left-shifted value and check overflow
> + *
> + * @a: Value to be shifted
> + * @s: How many bits left to shift
> + * @d: Pointer to where to store the result
> + *
> + * Computes *@d = (@a << @s)
> + *
> + * Returns true if '*d' cannot hold the result or when 'a << s' doesn't
> + * make sense. Example conditions:
> + * - 'a << s' causes bits to be lost when stored in *d.
> + * - 's' is garbage (e.g. negative) or so large that the result of
> + *   'a << s' is guaranteed to be 0.
> + * - 'a' is negative.
> + * - 'a << s' sets the sign bit, if any, in '*d'.
> + *
> + * '*d' will hold the results of the attempted shift, but is not
> + * considered "safe for use" if false is returned.
> + */
> +#define check_shl_overflow(a, s, d) ({					\
> +	typeof(a) _a = a;						\
> +	typeof(s) _s = s;						\
> +	typeof(d) _d = d;						\
> +	u64 _a_full = _a;						\
> +	unsigned int _to_shift =					\
> +		is_non_negative(_s) && _s < 8 * sizeof(*d) ? _s : 0;	\
> +	*_d = (_a_full << _to_shift);					\
> +	(_to_shift != _s || is_negative(*_d) || is_negative(_a) ||	\
> +	(*_d >> _to_shift) != _a);					\
> +})
> +
> +/**
> + * array_size() - Calculate size of 2-dimensional array.
> + *
> + * @a: dimension one
> + * @b: dimension two
> + *
> + * Calculates size of 2-dimensional array: @a * @b.
> + *
> + * Returns: number of bytes needed to represent the array or SIZE_MAX on
> + * overflow.
> + */
> +static inline size_t array_size(size_t a, size_t b)
> +{
> +	size_t bytes;
> +
> +	if (check_mul_overflow(a, b, &bytes))
> +		return SIZE_MAX;
> +
> +	return bytes;
> +}
> +
> +/**
> + * array3_size() - Calculate size of 3-dimensional array.
> + *
> + * @a: dimension one
> + * @b: dimension two
> + * @c: dimension three
> + *
> + * Calculates size of 3-dimensional array: @a * @b * @c.
> + *
> + * Returns: number of bytes needed to represent the array or SIZE_MAX on
> + * overflow.
> + */
> +static inline size_t array3_size(size_t a, size_t b, size_t c)
> +{
> +	size_t bytes;
> +
> +	if (check_mul_overflow(a, b, &bytes))
> +		return SIZE_MAX;
> +	if (check_mul_overflow(bytes, c, &bytes))
> +		return SIZE_MAX;
> +
> +	return bytes;
> +}
> +
> +/*
> + * Compute a*b+c, returning SIZE_MAX on overflow. Internal helper for
> + * struct_size() below.
> + */
> +static inline size_t __ab_c_size(size_t a, size_t b, size_t c)
> +{
> +	size_t bytes;
> +
> +	if (check_mul_overflow(a, b, &bytes))
> +		return SIZE_MAX;
> +	if (check_add_overflow(bytes, c, &bytes))
> +		return SIZE_MAX;
> +
> +	return bytes;
> +}
> +
> +/**
> + * struct_size() - Calculate size of structure with trailing array.
> + * @p: Pointer to the structure.
> + * @member: Name of the array member.
> + * @n: Number of elements in the array.
> + *
> + * Calculates size of memory needed for structure @p followed by an
> + * array of @n @member elements.
> + *
> + * Return: number of bytes needed or SIZE_MAX on overflow.
> + */
> +#define struct_size(p, member, n)					\
> +	__ab_c_size(n,							\
> +		    sizeof(*(p)->member) + __must_be_array((p)->member),\
> +		    sizeof(*(p)))
> +
> +#endif /* __LINUX_OVERFLOW_H */
> diff --git a/tests/mkfs-tests/018-multidevice-overflow/test.sh b/tests/mkfs-tests/018-multidevice-overflow/test.sh
> new file mode 100755
> index 00000000..0b550685
> --- /dev/null
> +++ b/tests/mkfs-tests/018-multidevice-overflow/test.sh
> @@ -0,0 +1,22 @@
> +#!/bin/bash
> +# test if mkfs.btrfs will create file systems that overflow total_bytes
> +
> +source "$TEST_TOP/common"
> +
> +check_prereq mkfs.btrfs
> +check_prereq btrfs
> +
> +mkdev()
> +{
> +	truncate -s 0 "$1"
> +	truncate -s "$2" "$1"
> +}
> +
> +mkdev "$TEST_TOP/test-dev1.img" 6E
> +mkdev "$TEST_TOP/test-dev2.img" 6E
> +mkdev "$TEST_TOP/test-dev3.img" 6E
> +
> +run_mustfail "mkfs.btrfs for too-large images" \
> +	     "$TOP/mkfs.btrfs" -f "$TEST_TOP"/test-dev[123].img
> +
> +rm -f "$TEST_TOP"/test-dev[123].img
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/5] btrfs-progs: qgroups: use parse_size instead of open coding it
  2019-08-14  1:04 ` [PATCH 3/5] btrfs-progs: qgroups: use parse_size instead of open coding it Jeff Mahoney
@ 2019-08-14  1:51   ` Qu Wenruo
  0 siblings, 0 replies; 11+ messages in thread
From: Qu Wenruo @ 2019-08-14  1:51 UTC (permalink / raw)
  To: Jeff Mahoney, linux-btrfs

[-- Attachment #1.1: Type: text/plain, Size: 2251 bytes --]



On 2019/8/14 上午9:04, Jeff Mahoney wrote:
> The only difference between parse_limit and parse_size is that
> parse_limit accepts "none" as a valid input.  That's easy enough
> to handle as a special case and lets us drop the duplicate code.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>

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

Thanks,
Qu

> ---
>  cmds/qgroup.c | 58 ++++------------------------------------------------------
>  1 file changed, 4 insertions(+), 54 deletions(-)
> 
> diff --git a/cmds/qgroup.c b/cmds/qgroup.c
> index 59b43c98..ba81052a 100644
> --- a/cmds/qgroup.c
> +++ b/cmds/qgroup.c
> @@ -159,56 +159,6 @@ static int _cmd_qgroup_create(int create, int argc, char **argv)
>  	return 0;
>  }
>  
> -static int parse_limit(const char *p, unsigned long long *s)
> -{
> -	char *endptr;
> -	unsigned long long size;
> -	unsigned long long CLEAR_VALUE = -1;
> -
> -	if (strcasecmp(p, "none") == 0) {
> -		*s = CLEAR_VALUE;
> -		return 1;
> -	}
> -
> -	if (p[0] == '-')
> -		return 0;
> -
> -	size = strtoull(p, &endptr, 10);
> -	if (p == endptr)
> -		return 0;
> -
> -	switch (*endptr) {
> -	case 'T':
> -	case 't':
> -		size *= 1024;
> -		/* fallthrough */
> -	case 'G':
> -	case 'g':
> -		size *= 1024;
> -		/* fallthrough */
> -	case 'M':
> -	case 'm':
> -		size *= 1024;
> -		/* fallthrough */
> -	case 'K':
> -	case 'k':
> -		size *= 1024;
> -		++endptr;
> -		break;
> -	case 0:
> -		break;
> -	default:
> -		return 0;
> -	}
> -
> -	if (*endptr)
> -		return 0;
> -
> -	*s = size;
> -
> -	return 1;
> -}
> -
>  static const char * const cmd_qgroup_assign_usage[] = {
>  	"btrfs qgroup assign [options] <src> <dst> <path>",
>  	"Assign SRC as the child qgroup of DST",
> @@ -455,10 +405,10 @@ static int cmd_qgroup_limit(const struct cmd_struct *cmd, int argc, char **argv)
>  	if (check_argc_min(argc - optind, 2))
>  		return 1;
>  
> -	if (!parse_limit(argv[optind], &size)) {
> -		error("invalid size argument: %s", argv[optind]);
> -		return 1;
> -	}
> +	if (!strcasecmp(argv[optind], "none"))
> +		size = -1ULL;
> +	else
> +		size = parse_size(argv[optind]);
>  
>  	memset(&args, 0, sizeof(args));
>  	if (compressed)
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 4/5] btrfs-progs: resize: more sensible error messages for bad sizes
  2019-08-14  1:04 ` [PATCH 4/5] btrfs-progs: resize: more sensible error messages for bad sizes Jeff Mahoney
@ 2019-08-14  1:53   ` Qu Wenruo
  0 siblings, 0 replies; 11+ messages in thread
From: Qu Wenruo @ 2019-08-14  1:53 UTC (permalink / raw)
  To: Jeff Mahoney, linux-btrfs

[-- Attachment #1.1: Type: text/plain, Size: 3763 bytes --]



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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 5/5] btrfs-progs: mkfs: print error messages instead of just error number
  2019-08-14  1:04 ` [PATCH 5/5] btrfs-progs: mkfs: print error messages instead of just error number Jeff Mahoney
@ 2019-08-14  1:54   ` Qu Wenruo
  2019-08-14  2:30     ` Jeff Mahoney
  0 siblings, 1 reply; 11+ messages in thread
From: Qu Wenruo @ 2019-08-14  1:54 UTC (permalink / raw)
  To: Jeff Mahoney, linux-btrfs

[-- Attachment #1.1: Type: text/plain, Size: 4924 bytes --]



On 2019/8/14 上午9:04, Jeff Mahoney wrote:
> Printing the error number means having to go look up what that error
> number means.  For a developer, it's easy.  For a user, it's unhelpful.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  mkfs/main.c | 47 ++++++++++++++++++++++++++++++-----------------
>  1 file changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/mkfs/main.c b/mkfs/main.c
> index b752da13..7bfeb610 100644
> --- a/mkfs/main.c
> +++ b/mkfs/main.c
> @@ -1197,37 +1197,43 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>  
>  	ret = create_metadata_block_groups(root, mixed, &allocation);
>  	if (ret) {
> -		error("failed to create default block groups: %d", ret);
> +		error("failed to create default block groups: %d/%s",
> +		      ret, strerror(-ret));

The new trend is to use %m.

So we would do something like:
	errno = -ret;
	error("%m");

Thanks,
Qu

>  		goto error;
>  	}
>  
>  	trans = btrfs_start_transaction(root, 1);
>  	if (IS_ERR(trans)) {
> -		error("failed to start transaction");
> +		error("failed to start transaction: %ld/%s",
> +		      PTR_ERR(trans), strerror(-PTR_ERR(trans)));
>  		goto error;
>  	}
>  
>  	ret = create_data_block_groups(trans, root, mixed, &allocation);
>  	if (ret) {
> -		error("failed to create default data block groups: %d", ret);
> +		error("failed to create default data block groups: %d/%s",
> +		      ret, strerror(-ret));
>  		goto error;
>  	}
>  
>  	ret = make_root_dir(trans, root);
>  	if (ret) {
> -		error("failed to setup the root directory: %d", ret);
> +		error("failed to setup the root directory: %d/%s",
> +		      ret, strerror(-ret));
>  		goto error;
>  	}
>  
>  	ret = btrfs_commit_transaction(trans, root);
>  	if (ret) {
> -		error("unable to commit transaction: %d", ret);
> +		error("unable to commit transaction: %d/%s",
> +		      ret, strerror(-ret));
>  		goto out;
>  	}
>  
>  	trans = btrfs_start_transaction(root, 1);
>  	if (IS_ERR(trans)) {
> -		error("failed to start transaction");
> +		error("failed to start transaction: %ld/%s",
> +		      PTR_ERR(trans), strerror(-PTR_ERR(trans)));
>  		goto error;
>  	}
>  
> @@ -1267,7 +1273,7 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>  		ret = btrfs_add_to_fsid(trans, root, fd, file, dev_block_count,
>  					sectorsize, sectorsize, sectorsize);
>  		if (ret) {
> -			error("unable to add %s to filesystem: %d", file, ret);
> +			error("unable to add %s to filesystem: %d/%s", file, ret, strerror(-ret));
>  			goto error;
>  		}
>  		if (verbose >= 2) {
> @@ -1284,46 +1290,52 @@ raid_groups:
>  	ret = create_raid_groups(trans, root, data_profile,
>  			 metadata_profile, mixed, &allocation);
>  	if (ret) {
> -		error("unable to create raid groups: %d", ret);
> +		error("unable to create raid groups: %d/%s",
> +		      ret, strerror(-ret));
>  		goto out;
>  	}
>  
>  	ret = create_data_reloc_tree(trans);
>  	if (ret) {
> -		error("unable to create data reloc tree: %d", ret);
> +		error("unable to create data reloc tree: %d/%s",
> +		      ret, strerror(-ret));
>  		goto out;
>  	}
>  
>  	ret = create_uuid_tree(trans);
>  	if (ret)
>  		warning(
> -	"unable to create uuid tree, will be created after mount: %d", ret);
> +	"unable to create uuid tree, will be created after mount: %d/%s",
> +			ret, strerror(-ret));
>  
>  	ret = btrfs_commit_transaction(trans, root);
>  	if (ret) {
> -		error("unable to commit transaction: %d", ret);
> +		error("unable to commit transaction: %d/%s",
> +		      ret, strerror(-ret));
>  		goto out;
>  	}
>  
>  	ret = cleanup_temp_chunks(fs_info, &allocation, data_profile,
>  				  metadata_profile, metadata_profile);
>  	if (ret < 0) {
> -		error("failed to cleanup temporary chunks: %d", ret);
> +		error("failed to cleanup temporary chunks: %d/%s",
> +		      ret, strerror(-ret));
>  		goto out;
>  	}
>  
>  	if (source_dir_set) {
>  		ret = btrfs_mkfs_fill_dir(source_dir, root, verbose);
>  		if (ret) {
> -			error("error while filling filesystem: %d", ret);
> +			error("error while filling filesystem: %d/%s",
> +			      ret, strerror(-ret));
>  			goto out;
>  		}
>  		if (shrink_rootdir) {
>  			ret = btrfs_mkfs_shrink_fs(fs_info, &shrink_size,
>  						   shrink_rootdir);
>  			if (ret < 0) {
> -				error("error while shrinking filesystem: %d",
> -					ret);
> +				error("error while shrinking filesystem: %d/%s",
> +					ret, strerror(-ret));
>  				goto out;
>  			}
>  		}
> @@ -1383,8 +1395,9 @@ out:
>  
>  	if (!ret && close_ret) {
>  		ret = close_ret;
> -		error("failed to close ctree, the filesystem may be inconsistent: %d",
> -		      ret);
> +		error(
> +	"failed to close ctree, the filesystem may be inconsistent: %d/%s",
> +		      ret, strerror(-ret));
>  	}
>  
>  	btrfs_close_all_devices();
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 5/5] btrfs-progs: mkfs: print error messages instead of just error number
  2019-08-14  1:54   ` Qu Wenruo
@ 2019-08-14  2:30     ` Jeff Mahoney
  0 siblings, 0 replies; 11+ messages in thread
From: Jeff Mahoney @ 2019-08-14  2:30 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs


> On Aug 13, 2019, at 9:54 PM, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
> 
> 
> 
>> On 2019/8/14 上午9:04, Jeff Mahoney wrote:
>> Printing the error number means having to go look up what that error
>> number means.  For a developer, it's easy.  For a user, it's unhelpful.
>> 
>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>> ---
>> mkfs/main.c | 47 ++++++++++++++++++++++++++++++-----------------
>> 1 file changed, 30 insertions(+), 17 deletions(-)
>> 
>> diff --git a/mkfs/main.c b/mkfs/main.c
>> index b752da13..7bfeb610 100644
>> --- a/mkfs/main.c
>> +++ b/mkfs/main.c
>> @@ -1197,37 +1197,43 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>> 
>>    ret = create_metadata_block_groups(root, mixed, &allocation);
>>    if (ret) {
>> -        error("failed to create default block groups: %d", ret);
>> +        error("failed to create default block groups: %d/%s",
>> +              ret, strerror(-ret));
> 
> The new trend is to use %m.
> 
> So we would do something like:
>    errno = -ret;
>    error("%m");
> 

Ok. It seems like that’s a job for a macro to set errno and autoappend the error message. I have a local branch that does that already.

-Jeff


> Thanks,
> Qu
> 
>>        goto error;
>>    }
>> 
>>    trans = btrfs_start_transaction(root, 1);
>>    if (IS_ERR(trans)) {
>> -        error("failed to start transaction");
>> +        error("failed to start transaction: %ld/%s",
>> +              PTR_ERR(trans), strerror(-PTR_ERR(trans)));
>>        goto error;
>>    }
>> 
>>    ret = create_data_block_groups(trans, root, mixed, &allocation);
>>    if (ret) {
>> -        error("failed to create default data block groups: %d", ret);
>> +        error("failed to create default data block groups: %d/%s",
>> +              ret, strerror(-ret));
>>        goto error;
>>    }
>> 
>>    ret = make_root_dir(trans, root);
>>    if (ret) {
>> -        error("failed to setup the root directory: %d", ret);
>> +        error("failed to setup the root directory: %d/%s",
>> +              ret, strerror(-ret));
>>        goto error;
>>    }
>> 
>>    ret = btrfs_commit_transaction(trans, root);
>>    if (ret) {
>> -        error("unable to commit transaction: %d", ret);
>> +        error("unable to commit transaction: %d/%s",
>> +              ret, strerror(-ret));
>>        goto out;
>>    }
>> 
>>    trans = btrfs_start_transaction(root, 1);
>>    if (IS_ERR(trans)) {
>> -        error("failed to start transaction");
>> +        error("failed to start transaction: %ld/%s",
>> +              PTR_ERR(trans), strerror(-PTR_ERR(trans)));
>>        goto error;
>>    }
>> 
>> @@ -1267,7 +1273,7 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>>        ret = btrfs_add_to_fsid(trans, root, fd, file, dev_block_count,
>>                    sectorsize, sectorsize, sectorsize);
>>        if (ret) {
>> -            error("unable to add %s to filesystem: %d", file, ret);
>> +            error("unable to add %s to filesystem: %d/%s", file, ret, strerror(-ret));
>>            goto error;
>>        }
>>        if (verbose >= 2) {
>> @@ -1284,46 +1290,52 @@ raid_groups:
>>    ret = create_raid_groups(trans, root, data_profile,
>>             metadata_profile, mixed, &allocation);
>>    if (ret) {
>> -        error("unable to create raid groups: %d", ret);
>> +        error("unable to create raid groups: %d/%s",
>> +              ret, strerror(-ret));
>>        goto out;
>>    }
>> 
>>    ret = create_data_reloc_tree(trans);
>>    if (ret) {
>> -        error("unable to create data reloc tree: %d", ret);
>> +        error("unable to create data reloc tree: %d/%s",
>> +              ret, strerror(-ret));
>>        goto out;
>>    }
>> 
>>    ret = create_uuid_tree(trans);
>>    if (ret)
>>        warning(
>> -    "unable to create uuid tree, will be created after mount: %d", ret);
>> +    "unable to create uuid tree, will be created after mount: %d/%s",
>> +            ret, strerror(-ret));
>> 
>>    ret = btrfs_commit_transaction(trans, root);
>>    if (ret) {
>> -        error("unable to commit transaction: %d", ret);
>> +        error("unable to commit transaction: %d/%s",
>> +              ret, strerror(-ret));
>>        goto out;
>>    }
>> 
>>    ret = cleanup_temp_chunks(fs_info, &allocation, data_profile,
>>                  metadata_profile, metadata_profile);
>>    if (ret < 0) {
>> -        error("failed to cleanup temporary chunks: %d", ret);
>> +        error("failed to cleanup temporary chunks: %d/%s",
>> +              ret, strerror(-ret));
>>        goto out;
>>    }
>> 
>>    if (source_dir_set) {
>>        ret = btrfs_mkfs_fill_dir(source_dir, root, verbose);
>>        if (ret) {
>> -            error("error while filling filesystem: %d", ret);
>> +            error("error while filling filesystem: %d/%s",
>> +                  ret, strerror(-ret));
>>            goto out;
>>        }
>>        if (shrink_rootdir) {
>>            ret = btrfs_mkfs_shrink_fs(fs_info, &shrink_size,
>>                           shrink_rootdir);
>>            if (ret < 0) {
>> -                error("error while shrinking filesystem: %d",
>> -                    ret);
>> +                error("error while shrinking filesystem: %d/%s",
>> +                    ret, strerror(-ret));
>>                goto out;
>>            }
>>        }
>> @@ -1383,8 +1395,9 @@ out:
>> 
>>    if (!ret && close_ret) {
>>        ret = close_ret;
>> -        error("failed to close ctree, the filesystem may be inconsistent: %d",
>> -              ret);
>> +        error(
>> +    "failed to close ctree, the filesystem may be inconsistent: %d/%s",
>> +              ret, strerror(-ret));
>>    }
>> 
>>    btrfs_close_all_devices();
>> 
> 

--
Jeff Mahoney
SUSE Labs

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-14  1:03 [PATCH 1/5] btrfs-progs: mkfs: treat btrfs_add_to_fsid as fatal error Jeff Mahoney
2019-08-14  1:03 ` [PATCH 2/5] btrfs-progs: btrfs_add_to_fsid: check if adding device would overflow Jeff Mahoney
2019-08-14  1:50   ` Qu Wenruo
2019-08-14  1:04 ` [PATCH 3/5] btrfs-progs: qgroups: use parse_size instead of open coding it Jeff Mahoney
2019-08-14  1:51   ` Qu Wenruo
2019-08-14  1:04 ` [PATCH 4/5] btrfs-progs: resize: more sensible error messages for bad sizes Jeff Mahoney
2019-08-14  1:53   ` Qu Wenruo
2019-08-14  1:04 ` [PATCH 5/5] btrfs-progs: mkfs: print error messages instead of just error number Jeff Mahoney
2019-08-14  1:54   ` Qu Wenruo
2019-08-14  2:30     ` Jeff Mahoney
2019-08-14  1:48 ` [PATCH 1/5] btrfs-progs: mkfs: treat btrfs_add_to_fsid as fatal error Qu Wenruo

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org linux-btrfs@archiver.kernel.org
	public-inbox-index linux-btrfs


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-btrfs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox