[PATCH 1/5] btrfs-progs: mkfs: treat btrfs_add_to_fsid as fatal error

[PATCH 1/5] btrfs-progs: mkfs: treat btrfs_add_to_fsid as fatal error

[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

[PATCH 2/5] btrfs-progs: btrfs_add_to_fsid: check if adding device would overflow

[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

Re: [PATCH 2/5] btrfs-progs: btrfs_add_to_fsid: check if adding device would overflow

[Qu Wenruo]

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

Re: [PATCH 2/5] btrfs-progs: btrfs_add_to_fsid: check if adding device would overflow

[David Sterba]

On Tue, Aug 13, 2019 at 09:03:59PM -0400, 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.
> 
> 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 ++

I've split that to 3 patches as each does something else.

[PATCH 3/5] btrfs-progs: qgroups: use parse_size instead of open coding it

[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

Re: [PATCH 3/5] btrfs-progs: qgroups: use parse_size instead of open coding it

[Qu Wenruo]

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

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

[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

Re: [PATCH 4/5] btrfs-progs: resize: more sensible error messages for bad sizes

[Qu Wenruo]

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

Re: [PATCH 4/5] btrfs-progs: resize: more sensible error messages for bad sizes

[David Sterba]

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 5/5] btrfs-progs: mkfs: print error messages instead of just error number

[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

Re: [PATCH 5/5] btrfs-progs: mkfs: print error messages instead of just error number

[Qu Wenruo]

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

Re: [PATCH 5/5] btrfs-progs: mkfs: print error messages instead of just error number

[Jeff Mahoney]

> 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

Re: [PATCH 1/5] btrfs-progs: mkfs: treat btrfs_add_to_fsid as fatal error

[Qu Wenruo]

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

Re: [PATCH 1/5] btrfs-progs: mkfs: treat btrfs_add_to_fsid as fatal error

[David Sterba]

On Tue, Aug 13, 2019 at 09:03:58PM -0400, 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>

I've merged 1-3, splitting some of the patches. Patch 4 is broken for
some of the resize format and can't be trivially fixed. I'll add a test
so we have better coverage for further development.

Re: [PATCH 1/5] btrfs-progs: mkfs: treat btrfs_add_to_fsid as fatal error

[David Sterba]

On Tue, Aug 27, 2019 at 05:27:36PM +0200, David Sterba wrote:
> On Tue, Aug 13, 2019 at 09:03:58PM -0400, 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>
> 
> I've merged 1-3, splitting some of the patches. Patch 4 is broken for
> some of the resize format and can't be trivially fixed. I'll add a test
> so we have better coverage for further development.

Actually we do have a test for the formats, it's cli/003-fi-resize-args
(though lacks 'max', that I'll add).