linux-ext4.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RESEND PATCH v2 0/5] ext4: fsmap: Consolidate fsmap_head checks
@ 2023-05-08 15:42 Tudor Ambarus
  2023-05-08 15:42 ` [RESEND PATCH v2 1/5] ext4: ioctl: Add missing linux/string.h header Tudor Ambarus
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Tudor Ambarus @ 2023-05-08 15:42 UTC (permalink / raw)
  To: tytso; +Cc: adilger.kernel, linux-ext4, linux-kernel, joneslee, Tudor Ambarus

The sanity checks on user provided data were scattered along three
representations of the keys. This is not only difficult to follow but
also inefficient in case one of the checks returns an error because we
waste CPU cycles by copying data and preparing other local structures
that won't be used in case of errors.
Consolidate the logic around fsmap sanity checks.

No functional change in the code. Tested with the ext4 fsmap xfstests
027, 028, 029. All passed, see the summary reports below.

v2:
- ext4: fsmap: Consolidate fsmap_head checks
  - split patch for easier review
  - rewrite commit message
  - new patches {1, 2, 4}/5
v1:
https://lore.kernel.org/linux-ext4/20230222131211.3898066-1-tudor.ambarus@linaro.org/

-------------------- Summary report
KERNEL:    kernel 6.2.0-rc5-xfstests-00005-gf59f84395275 #16 SMP PREEMPT_DYNAMIC Wed Mar 15 11:06:14 UTC 2023 x86_64
CMDLINE:   ext4/027
CPUS:      2
MEM:       1975.31

ext4/4k: 1 tests, 1 seconds
  ext4/027     Pass     1s
ext4/1k: 1 tests, 1 seconds
  ext4/027     Pass     0s
ext4/ext3: 1 tests, 1 seconds
  ext4/027     Pass     0s
ext4/encrypt: 1 tests, 1 seconds
  ext4/027     Pass     1s
ext4/nojournal: 1 tests, 0 seconds
  ext4/027     Pass     0s
ext4/ext3conv: 1 tests, 1 seconds
  ext4/027     Pass     0s
ext4/adv: 1 tests, 1 seconds
  ext4/027     Pass     1s
ext4/dioread_nolock: 1 tests, 1 seconds
  ext4/027     Pass     1s
ext4/data_journal: 1 tests, 0 seconds
  ext4/027     Pass     0s
ext4/bigalloc: 1 tests, 0 seconds
  ext4/027     Pass     0s
ext4/bigalloc_1k: 1 tests, 1 seconds
  ext4/027     Pass     0s
Totals: 11 tests, 0 skipped, 0 failures, 0 errors, 4s

FSTESTVER: blktests 4e07b0c (Fri, 15 Jul 2022 14:40:03 +0900)
FSTESTVER: fio  fio-3.31 (Tue, 9 Aug 2022 14:41:25 -0600)
FSTESTVER: fsverity v1.5 (Sun, 6 Feb 2022 10:59:13 -0800)
FSTESTVER: ima-evm-utils v1.3.2 (Wed, 28 Oct 2020 13:18:08 -0400)
FSTESTVER: nvme-cli v1.16 (Thu, 11 Nov 2021 13:09:06 -0800)
FSTESTVER: quota  v4.05-43-gd2256ac (Fri, 17 Sep 2021 14:04:16 +0200)
FSTESTVER: util-linux v2.38.1 (Thu, 4 Aug 2022 11:06:21 +0200)
FSTESTVER: xfsprogs v5.19.0 (Fri, 12 Aug 2022 13:45:01 -0500)
FSTESTVER: xfstests v2022.08.21-8-g289f50f8 (Sun, 21 Aug 2022 15:21:34 -0400)
FSTESTVER: xfstests-bld bb566bcf (Wed, 24 Aug 2022 23:07:24 -0400)
FSTESTVER: zz_build-distro bullseye
FSTESTCFG: all
FSTESTSET: ext4/027
FSTESTOPT: aex
[   59.850894] ACPI: PM: Preparing to enter system sleep state S5
[   59.855495] reboot: Power down

-------------------- Summary report
KERNEL:    kernel 6.2.0-rc5-xfstests-00005-gf59f84395275 #16 SMP PREEMPT_DYNAMIC Wed Mar 15 11:06:14 UTC 2023 x86_64
CMDLINE:   ext4/028
CPUS:      2
MEM:       1975.31

ext4/4k: 1 tests, 5 seconds
  ext4/028     Pass     5s
ext4/1k: 1 tests, 2 seconds
  ext4/028     Pass     2s
ext4/ext3: 1 tests, 1 skipped, 1 seconds
  ext4/028     Skipped  0s
ext4/encrypt: 0 tests, 0 seconds
ext4/nojournal: 1 tests, 4 seconds
  ext4/028     Pass     4s
ext4/ext3conv: 1 tests, 4 seconds
  ext4/028     Pass     4s
ext4/adv: 1 tests, 4 seconds
  ext4/028     Pass     4s
ext4/dioread_nolock: 1 tests, 1 seconds
  ext4/028     Pass     0s
ext4/data_journal: 1 tests, 1 seconds
  ext4/028     Pass     0s
ext4/bigalloc: 1 tests, 5 seconds
  ext4/028     Pass     5s
ext4/bigalloc_1k: 1 tests, 2 seconds
  ext4/028     Pass     2s
Totals: 10 tests, 1 skipped, 0 failures, 0 errors, 26s

FSTESTVER: blktests 4e07b0c (Fri, 15 Jul 2022 14:40:03 +0900)
FSTESTVER: fio  fio-3.31 (Tue, 9 Aug 2022 14:41:25 -0600)
FSTESTVER: fsverity v1.5 (Sun, 6 Feb 2022 10:59:13 -0800)
FSTESTVER: ima-evm-utils v1.3.2 (Wed, 28 Oct 2020 13:18:08 -0400)
FSTESTVER: nvme-cli v1.16 (Thu, 11 Nov 2021 13:09:06 -0800)
FSTESTVER: quota  v4.05-43-gd2256ac (Fri, 17 Sep 2021 14:04:16 +0200)
FSTESTVER: util-linux v2.38.1 (Thu, 4 Aug 2022 11:06:21 +0200)
FSTESTVER: xfsprogs v5.19.0 (Fri, 12 Aug 2022 13:45:01 -0500)
FSTESTVER: xfstests v2022.08.21-8-g289f50f8 (Sun, 21 Aug 2022 15:21:34 -0400)
FSTESTVER: xfstests-bld bb566bcf (Wed, 24 Aug 2022 23:07:24 -0400)
FSTESTVER: zz_build-distro bullseye
FSTESTCFG: all
FSTESTSET: ext4/028
FSTESTOPT: aex
[   79.583715] ACPI: PM: Preparing to enter system sleep state S5
[   79.588092] reboot: Power down

-------------------- Summary report
KERNEL:    kernel 6.2.0-rc5-xfstests-00005-gf59f84395275 #16 SMP PREEMPT_DYNAMIC Wed Mar 15 11:06:14 UTC 2023 x86_64
CMDLINE:   -c logdev ext4/029
CPUS:      2
MEM:       1975.31

ext4/logdev: 1 tests, 1 seconds
  ext4/029     Pass     1s
Totals: 1 tests, 0 skipped, 0 failures, 0 errors, 1s

FSTESTVER: blktests 4e07b0c (Fri, 15 Jul 2022 14:40:03 +0900)
FSTESTVER: fio  fio-3.31 (Tue, 9 Aug 2022 14:41:25 -0600)
FSTESTVER: fsverity v1.5 (Sun, 6 Feb 2022 10:59:13 -0800)
FSTESTVER: ima-evm-utils v1.3.2 (Wed, 28 Oct 2020 13:18:08 -0400)
FSTESTVER: nvme-cli v1.16 (Thu, 11 Nov 2021 13:09:06 -0800)
FSTESTVER: quota  v4.05-43-gd2256ac (Fri, 17 Sep 2021 14:04:16 +0200)
FSTESTVER: util-linux v2.38.1 (Thu, 4 Aug 2022 11:06:21 +0200)
FSTESTVER: xfsprogs v5.19.0 (Fri, 12 Aug 2022 13:45:01 -0500)
FSTESTVER: xfstests v2022.08.21-8-g289f50f8 (Sun, 21 Aug 2022 15:21:34 -0400)
FSTESTVER: xfstests-bld bb566bcf (Wed, 24 Aug 2022 23:07:24 -0400)
FSTESTVER: zz_build-distro bullseye
FSTESTCFG: logdev
FSTESTSET: ext4/029
FSTESTOPT: aex
[    8.712254] reboot: Power down

Tudor Ambarus (5):
  ext4: ioctl: Add missing linux/string.h header
  ext4: fsmap: Check fmh_iflags value directly on the user copied data
  ext4: fsmap: Consolidate fsmap_head checks
  ext4: fsmap: Do the validation checks on constified fsmap data
  ext4: fsmap: Remove duplicated initialization

 fs/ext4/fsmap.c | 52 ++++++++++++++++++++++++++++++++++---------------
 fs/ext4/fsmap.h |  3 +++
 fs/ext4/ioctl.c | 18 ++++-------------
 3 files changed, 43 insertions(+), 30 deletions(-)

-- 
2.40.1.521.gf1e218fcd8-goog


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

* [RESEND PATCH v2 1/5] ext4: ioctl: Add missing linux/string.h header
  2023-05-08 15:42 [RESEND PATCH v2 0/5] ext4: fsmap: Consolidate fsmap_head checks Tudor Ambarus
@ 2023-05-08 15:42 ` Tudor Ambarus
  2023-05-08 15:42 ` [RESEND PATCH v2 2/5] ext4: fsmap: Check fmh_iflags value directly on the user copied data Tudor Ambarus
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Tudor Ambarus @ 2023-05-08 15:42 UTC (permalink / raw)
  To: tytso; +Cc: adilger.kernel, linux-ext4, linux-kernel, joneslee, Tudor Ambarus

ext4/ioctl.c uses strnlen(), strncpy(), memchr_inv() that are defined
in linux/string.h, but those were being included by sheer luck, indirectly,
via <linux/uuid.h> which includes <linux/string.h>. Add missing header.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 fs/ext4/ioctl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index f9a430152063..6d5210b94ac2 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -20,6 +20,7 @@
 #include <linux/delay.h>
 #include <linux/iversion.h>
 #include <linux/fileattr.h>
+#include <linux/string.h>
 #include <linux/uuid.h>
 #include "ext4_jbd2.h"
 #include "ext4.h"
-- 
2.40.1.521.gf1e218fcd8-goog


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

* [RESEND PATCH v2 2/5] ext4: fsmap: Check fmh_iflags value directly on the user copied data
  2023-05-08 15:42 [RESEND PATCH v2 0/5] ext4: fsmap: Consolidate fsmap_head checks Tudor Ambarus
  2023-05-08 15:42 ` [RESEND PATCH v2 1/5] ext4: ioctl: Add missing linux/string.h header Tudor Ambarus
@ 2023-05-08 15:42 ` Tudor Ambarus
  2023-05-08 15:42 ` [RESEND PATCH v2 3/5] ext4: fsmap: Consolidate fsmap_head checks Tudor Ambarus
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Tudor Ambarus @ 2023-05-08 15:42 UTC (permalink / raw)
  To: tytso; +Cc: adilger.kernel, linux-ext4, linux-kernel, joneslee, Tudor Ambarus

struct ext4_fsmap_head is the ext4 internal fsmap representation of
struct fsmap_head. As the code was, the fmh_iflags validation was done
on the fmh_iflags value of the internal fsmap representation. Since
xhead.fmh_iflags is initialized with head.fmh_iflags and not changed
afterwards, do the validation of fmh_iflags directly on fsmap_head data,
it spares some superfluous initializations in case the user provides a
wrong value for fmh_iflags.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 fs/ext4/fsmap.c | 2 --
 fs/ext4/ioctl.c | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/fsmap.c b/fs/ext4/fsmap.c
index cdf9bfe10137..7765293bfa5d 100644
--- a/fs/ext4/fsmap.c
+++ b/fs/ext4/fsmap.c
@@ -635,8 +635,6 @@ int ext4_getfsmap(struct super_block *sb, struct ext4_fsmap_head *head,
 	int i;
 	int error = 0;
 
-	if (head->fmh_iflags & ~FMH_IF_VALID)
-		return -EINVAL;
 	if (!ext4_getfsmap_is_valid_device(sb, &head->fmh_keys[0]) ||
 	    !ext4_getfsmap_is_valid_device(sb, &head->fmh_keys[1]))
 		return -EINVAL;
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 6d5210b94ac2..a585d96567b5 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -873,6 +873,8 @@ static int ext4_ioc_getfsmap(struct super_block *sb,
 
 	if (copy_from_user(&head, arg, sizeof(struct fsmap_head)))
 		return -EFAULT;
+	if (head.fmh_iflags & ~FMH_IF_VALID)
+		return -EINVAL;
 	if (memchr_inv(head.fmh_reserved, 0, sizeof(head.fmh_reserved)) ||
 	    memchr_inv(head.fmh_keys[0].fmr_reserved, 0,
 		       sizeof(head.fmh_keys[0].fmr_reserved)) ||
-- 
2.40.1.521.gf1e218fcd8-goog


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

* [RESEND PATCH v2 3/5] ext4: fsmap: Consolidate fsmap_head checks
  2023-05-08 15:42 [RESEND PATCH v2 0/5] ext4: fsmap: Consolidate fsmap_head checks Tudor Ambarus
  2023-05-08 15:42 ` [RESEND PATCH v2 1/5] ext4: ioctl: Add missing linux/string.h header Tudor Ambarus
  2023-05-08 15:42 ` [RESEND PATCH v2 2/5] ext4: fsmap: Check fmh_iflags value directly on the user copied data Tudor Ambarus
@ 2023-05-08 15:42 ` Tudor Ambarus
  2023-05-08 15:42 ` [RESEND PATCH v2 4/5] ext4: fsmap: Do the validation checks on constified fsmap data Tudor Ambarus
  2023-05-08 15:42 ` [RESEND PATCH v2 5/5] ext4: fsmap: Remove duplicated initialization Tudor Ambarus
  4 siblings, 0 replies; 6+ messages in thread
From: Tudor Ambarus @ 2023-05-08 15:42 UTC (permalink / raw)
  To: tytso; +Cc: adilger.kernel, linux-ext4, linux-kernel, joneslee, Tudor Ambarus

The sanity checks on the user provided data were scattered in three
representations of the fsmap keys:
1/ the keys from struct fsmap_head in ext4_ioc_getfsmap() -> contain the
   data copied from the user.
2/ the keys from struct ext4_fsmap_head -> contain the ext4 internal
   representation of the keys. These are the same keys as in 1/ but with
   the fmr_physical and fmr_length shifted to right by
   sb->s_blocksize_bits, see ext4_fsmap_to_internal(). The sanity checks
   on these keys were done in ext4_getfsmap(), see where
   ext4_getfsmap_is_valid_device() and ext4_getfsmap_check_keys() are
   called.
3/ dkeys in ext4_getfsmap() -> local keys used to query the device. These
   are 2/ but with the low key bumped by fmr_length. The low key is
   bumped because userspace is allowed to use the last mapping from the
   previous call as the low key to the next. In consequence, the low key
   is incremented to ensure we return the next mapping. The low key
   from dkey was checked together with the high key fron 2/ by calling
   ext4_getfsmap_check_keys().

Having the sanity checks on user provided data scattered along these
three representations of the keys is not only difficult to follow but
also inefficient in case one of the checks returns an error because we
waste CPU cycles by copying data and preparing other local structures
that won't be used in case of errors.

Since 2/ and 3/ are just adapted copies of 1/, do all the checks
directly on 1/. Gather all the checks done on the user data in a single
method and call it immediately after copying the data from user. One may
notice that I introduced a local u64 l_fmr_phys in
ext4_getfsmap_check_keys() where I bumped the low key by fmr_length
in order to preserve the validation check that was done on the low key
from 3/.

With this we should have better clarity about the sanity checks and also
better efficiency in case the user provides bad data. No change in
functionality. Patch tested with the ext4 fsmap xfstests 027, 028, 029.
All passed.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 fs/ext4/fsmap.c | 48 ++++++++++++++++++++++++++++++++++++------------
 fs/ext4/fsmap.h |  2 ++
 fs/ext4/ioctl.c | 19 +++----------------
 3 files changed, 41 insertions(+), 28 deletions(-)

diff --git a/fs/ext4/fsmap.c b/fs/ext4/fsmap.c
index 7765293bfa5d..463e8165b1e9 100644
--- a/fs/ext4/fsmap.c
+++ b/fs/ext4/fsmap.c
@@ -9,6 +9,7 @@
 #include "fsmap.h"
 #include "mballoc.h"
 #include <linux/sort.h>
+#include <linux/string.h>
 #include <linux/list_sort.h>
 #include <trace/events/ext4.h>
 
@@ -571,7 +572,7 @@ static int ext4_getfsmap_datadev(struct super_block *sb,
 
 /* Do we recognize the device? */
 static bool ext4_getfsmap_is_valid_device(struct super_block *sb,
-					  struct ext4_fsmap *fm)
+					  struct fsmap *fm)
 {
 	if (fm->fmr_device == 0 || fm->fmr_device == UINT_MAX ||
 	    fm->fmr_device == new_encode_dev(sb->s_bdev->bd_dev))
@@ -583,17 +584,19 @@ static bool ext4_getfsmap_is_valid_device(struct super_block *sb,
 }
 
 /* Ensure that the low key is less than the high key. */
-static bool ext4_getfsmap_check_keys(struct ext4_fsmap *low_key,
-				     struct ext4_fsmap *high_key)
+static bool ext4_getfsmap_check_keys(struct fsmap *low_key,
+				     struct fsmap *high_key)
 {
+	u64 l_fmr_phys = low_key->fmr_physical + low_key->fmr_length;
+
 	if (low_key->fmr_device > high_key->fmr_device)
 		return false;
 	if (low_key->fmr_device < high_key->fmr_device)
 		return true;
 
-	if (low_key->fmr_physical > high_key->fmr_physical)
+	if (l_fmr_phys > high_key->fmr_physical)
 		return false;
-	if (low_key->fmr_physical < high_key->fmr_physical)
+	if (l_fmr_phys < high_key->fmr_physical)
 		return true;
 
 	if (low_key->fmr_owner > high_key->fmr_owner)
@@ -604,6 +607,34 @@ static bool ext4_getfsmap_check_keys(struct ext4_fsmap *low_key,
 	return false;
 }
 
+int ext4_fsmap_check_head(struct super_block *sb, struct fsmap_head *head)
+{
+	const struct fsmap *l = &head->fmh_keys[0];
+	const struct fsmap *h = &head->fmh_keys[1];
+
+	if (head->fmh_iflags & ~FMH_IF_VALID)
+		return -EINVAL;
+	if (memchr_inv(head->fmh_reserved, 0, sizeof(head->fmh_reserved)) ||
+	    memchr_inv(l->fmr_reserved, 0, sizeof(l->fmr_reserved)) ||
+	    memchr_inv(h->fmr_reserved, 0, sizeof(h->fmr_reserved)))
+		return -EINVAL;
+	/*
+	 * ext4 doesn't report file extents at all, so the only valid
+	 * file offsets are the magic ones (all zeroes or all ones).
+	 */
+	if (l->fmr_offset || (h->fmr_offset != 0 && h->fmr_offset != -1ULL))
+		return -EINVAL;
+
+	if (!ext4_getfsmap_is_valid_device(sb, l) ||
+	    !ext4_getfsmap_is_valid_device(sb, h))
+		return -EINVAL;
+
+	if (!ext4_getfsmap_check_keys(l, h))
+		return -EINVAL;
+
+	return 0;
+}
+
 #define EXT4_GETFSMAP_DEVS	2
 /*
  * Get filesystem's extents as described in head, and format for
@@ -635,10 +666,6 @@ int ext4_getfsmap(struct super_block *sb, struct ext4_fsmap_head *head,
 	int i;
 	int error = 0;
 
-	if (!ext4_getfsmap_is_valid_device(sb, &head->fmh_keys[0]) ||
-	    !ext4_getfsmap_is_valid_device(sb, &head->fmh_keys[1]))
-		return -EINVAL;
-
 	head->fmh_entries = 0;
 
 	/* Set up our device handlers. */
@@ -671,9 +698,6 @@ int ext4_getfsmap(struct super_block *sb, struct ext4_fsmap_head *head,
 	dkeys[0].fmr_length = 0;
 	memset(&dkeys[1], 0xFF, sizeof(struct ext4_fsmap));
 
-	if (!ext4_getfsmap_check_keys(dkeys, &head->fmh_keys[1]))
-		return -EINVAL;
-
 	info.gfi_next_fsblk = head->fmh_keys[0].fmr_physical +
 			  head->fmh_keys[0].fmr_length;
 	info.gfi_formatter = formatter;
diff --git a/fs/ext4/fsmap.h b/fs/ext4/fsmap.h
index ac642be2302e..e7c510afd672 100644
--- a/fs/ext4/fsmap.h
+++ b/fs/ext4/fsmap.h
@@ -8,6 +8,7 @@
 #define	__EXT4_FSMAP_H__
 
 struct fsmap;
+struct fsmap_head;
 
 /* internal fsmap representation */
 struct ext4_fsmap {
@@ -32,6 +33,7 @@ void ext4_fsmap_from_internal(struct super_block *sb, struct fsmap *dest,
 		struct ext4_fsmap *src);
 void ext4_fsmap_to_internal(struct super_block *sb, struct ext4_fsmap *dest,
 		struct fsmap *src);
+int ext4_fsmap_check_head(struct super_block *sb, struct fsmap_head *head);
 
 /* fsmap to userspace formatter - copy to user & advance pointer */
 typedef int (*ext4_fsmap_format_t)(struct ext4_fsmap *, void *);
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index a585d96567b5..11d83ee6ba89 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -873,22 +873,9 @@ static int ext4_ioc_getfsmap(struct super_block *sb,
 
 	if (copy_from_user(&head, arg, sizeof(struct fsmap_head)))
 		return -EFAULT;
-	if (head.fmh_iflags & ~FMH_IF_VALID)
-		return -EINVAL;
-	if (memchr_inv(head.fmh_reserved, 0, sizeof(head.fmh_reserved)) ||
-	    memchr_inv(head.fmh_keys[0].fmr_reserved, 0,
-		       sizeof(head.fmh_keys[0].fmr_reserved)) ||
-	    memchr_inv(head.fmh_keys[1].fmr_reserved, 0,
-		       sizeof(head.fmh_keys[1].fmr_reserved)))
-		return -EINVAL;
-	/*
-	 * ext4 doesn't report file extents at all, so the only valid
-	 * file offsets are the magic ones (all zeroes or all ones).
-	 */
-	if (head.fmh_keys[0].fmr_offset ||
-	    (head.fmh_keys[1].fmr_offset != 0 &&
-	     head.fmh_keys[1].fmr_offset != -1ULL))
-		return -EINVAL;
+	error = ext4_fsmap_check_head(sb, &head);
+	if (error)
+		return error;
 
 	xhead.fmh_iflags = head.fmh_iflags;
 	xhead.fmh_count = head.fmh_count;
-- 
2.40.1.521.gf1e218fcd8-goog


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

* [RESEND PATCH v2 4/5] ext4: fsmap: Do the validation checks on constified fsmap data
  2023-05-08 15:42 [RESEND PATCH v2 0/5] ext4: fsmap: Consolidate fsmap_head checks Tudor Ambarus
                   ` (2 preceding siblings ...)
  2023-05-08 15:42 ` [RESEND PATCH v2 3/5] ext4: fsmap: Consolidate fsmap_head checks Tudor Ambarus
@ 2023-05-08 15:42 ` Tudor Ambarus
  2023-05-08 15:42 ` [RESEND PATCH v2 5/5] ext4: fsmap: Remove duplicated initialization Tudor Ambarus
  4 siblings, 0 replies; 6+ messages in thread
From: Tudor Ambarus @ 2023-05-08 15:42 UTC (permalink / raw)
  To: tytso; +Cc: adilger.kernel, linux-ext4, linux-kernel, joneslee, Tudor Ambarus

Now that we do the sanity checks directly on the data copied from user,
we can also constify the fsmap data while the checks are in progress.
Do the validation checks on constified data, it imposes that the fsmap
data is not updated during validation and assures readers that nothing
strange happens during the validation sequence of calls.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 fs/ext4/fsmap.c | 8 ++++----
 fs/ext4/fsmap.h | 3 ++-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/fsmap.c b/fs/ext4/fsmap.c
index 463e8165b1e9..655379c96fcf 100644
--- a/fs/ext4/fsmap.c
+++ b/fs/ext4/fsmap.c
@@ -572,7 +572,7 @@ static int ext4_getfsmap_datadev(struct super_block *sb,
 
 /* Do we recognize the device? */
 static bool ext4_getfsmap_is_valid_device(struct super_block *sb,
-					  struct fsmap *fm)
+					  const struct fsmap *fm)
 {
 	if (fm->fmr_device == 0 || fm->fmr_device == UINT_MAX ||
 	    fm->fmr_device == new_encode_dev(sb->s_bdev->bd_dev))
@@ -584,8 +584,8 @@ static bool ext4_getfsmap_is_valid_device(struct super_block *sb,
 }
 
 /* Ensure that the low key is less than the high key. */
-static bool ext4_getfsmap_check_keys(struct fsmap *low_key,
-				     struct fsmap *high_key)
+static bool ext4_getfsmap_check_keys(const struct fsmap *low_key,
+				     const struct fsmap *high_key)
 {
 	u64 l_fmr_phys = low_key->fmr_physical + low_key->fmr_length;
 
@@ -607,7 +607,7 @@ static bool ext4_getfsmap_check_keys(struct fsmap *low_key,
 	return false;
 }
 
-int ext4_fsmap_check_head(struct super_block *sb, struct fsmap_head *head)
+int ext4_fsmap_check_head(struct super_block *sb, const struct fsmap_head *head)
 {
 	const struct fsmap *l = &head->fmh_keys[0];
 	const struct fsmap *h = &head->fmh_keys[1];
diff --git a/fs/ext4/fsmap.h b/fs/ext4/fsmap.h
index e7c510afd672..8325258def7b 100644
--- a/fs/ext4/fsmap.h
+++ b/fs/ext4/fsmap.h
@@ -33,7 +33,8 @@ void ext4_fsmap_from_internal(struct super_block *sb, struct fsmap *dest,
 		struct ext4_fsmap *src);
 void ext4_fsmap_to_internal(struct super_block *sb, struct ext4_fsmap *dest,
 		struct fsmap *src);
-int ext4_fsmap_check_head(struct super_block *sb, struct fsmap_head *head);
+int ext4_fsmap_check_head(struct super_block *sb,
+			  const struct fsmap_head *head);
 
 /* fsmap to userspace formatter - copy to user & advance pointer */
 typedef int (*ext4_fsmap_format_t)(struct ext4_fsmap *, void *);
-- 
2.40.1.521.gf1e218fcd8-goog


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

* [RESEND PATCH v2 5/5] ext4: fsmap: Remove duplicated initialization
  2023-05-08 15:42 [RESEND PATCH v2 0/5] ext4: fsmap: Consolidate fsmap_head checks Tudor Ambarus
                   ` (3 preceding siblings ...)
  2023-05-08 15:42 ` [RESEND PATCH v2 4/5] ext4: fsmap: Do the validation checks on constified fsmap data Tudor Ambarus
@ 2023-05-08 15:42 ` Tudor Ambarus
  4 siblings, 0 replies; 6+ messages in thread
From: Tudor Ambarus @ 2023-05-08 15:42 UTC (permalink / raw)
  To: tytso; +Cc: adilger.kernel, linux-ext4, linux-kernel, joneslee, Tudor Ambarus

All members of struct ext4_fsmap_head were already initialized with zero
in the caller, ext4_ioc_getfsmap(), remove duplicated initialization.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 fs/ext4/fsmap.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/ext4/fsmap.c b/fs/ext4/fsmap.c
index 655379c96fcf..d19d85be3404 100644
--- a/fs/ext4/fsmap.c
+++ b/fs/ext4/fsmap.c
@@ -666,8 +666,6 @@ int ext4_getfsmap(struct super_block *sb, struct ext4_fsmap_head *head,
 	int i;
 	int error = 0;
 
-	head->fmh_entries = 0;
-
 	/* Set up our device handlers. */
 	memset(handlers, 0, sizeof(handlers));
 	handlers[0].gfd_dev = new_encode_dev(sb->s_bdev->bd_dev);
-- 
2.40.1.521.gf1e218fcd8-goog


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

end of thread, other threads:[~2023-05-08 15:43 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-08 15:42 [RESEND PATCH v2 0/5] ext4: fsmap: Consolidate fsmap_head checks Tudor Ambarus
2023-05-08 15:42 ` [RESEND PATCH v2 1/5] ext4: ioctl: Add missing linux/string.h header Tudor Ambarus
2023-05-08 15:42 ` [RESEND PATCH v2 2/5] ext4: fsmap: Check fmh_iflags value directly on the user copied data Tudor Ambarus
2023-05-08 15:42 ` [RESEND PATCH v2 3/5] ext4: fsmap: Consolidate fsmap_head checks Tudor Ambarus
2023-05-08 15:42 ` [RESEND PATCH v2 4/5] ext4: fsmap: Do the validation checks on constified fsmap data Tudor Ambarus
2023-05-08 15:42 ` [RESEND PATCH v2 5/5] ext4: fsmap: Remove duplicated initialization Tudor Ambarus

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).