[PATCH v2 0/5] mseal cleanups, fixup MAP_PRIVATE file-backed case

[PATCH v2 0/5] mseal cleanups, fixup MAP_PRIVATE file-backed case

[Lorenzo Stoakes]

Perform a number of cleanups to the mseal logic. Firstly, VM_SEALED is
treated differently from every other VMA flag, it really doesn't make sense
to do this, so we start by making this consistent with everything else.

Next we place the madvise logic where it belongs - in mm/madvise.c. It
really makes no sense to abstract this elsewhere. In doing so, we go to
great lengths to explain very clearly the previously very confusing logic
as to what sealed mappings are impacted here.

In doing so, we fix an existing logical oversight - previously we permitted
an madvise() discard operation for a sealed, read-only MAP_PRIVATE
file-backed mapping.

However this is incorrect. To see why consider:

1. A MAP_PRIVATE R/W file-backed mapping is established.
2. The mapping is written to, which backs it with anonymous memory.
3. The mapping is mprotect()'d read-only.
4. The mapping is mseal()'d.

At this point you have data that, once sealed, a user cannot alter, but a
discard operation can unrecoverably remove. This contradicts the semantics
of mseal(), so should not be permitted.

We then abstract out and explain the 'are there are any gaps in this range
in the mm?' check being performed as a prerequisite to mseal being
performed.

Finally, we simplify the actual mseal logic which is really quite
straightforward.


v2:
* Propagated tags, thanks everyone!
* Updated can_madvise_modify() to a more logical order re: the checks
  performed, as per David.
* Replaced vma_is_anonymous() check (which was, in the original code, a
  vma->vm_file or vma->vm_ops check) with a vma->vm_flags & VM_SHARED
  check - to explicitly check for shared mappings vs private to preclude
  MAP_PRIVATE-mapping file-baked mappings, as per David.
* Made range_contains_unmapped() static and placed in mm/mseal.c to avoid
  encouraging any other internal users towards this rather silly pattern,
  as per Pedro and Liam.

v1:
https://lore.kernel.org/all/cover.1752497324.git.lorenzo.stoakes@oracle.com/

Lorenzo Stoakes (5):
  mm/mseal: always define VM_SEALED
  mm/mseal: update madvise() logic
  mm/mseal: small cleanups
  mm/mseal: Simplify and rename VMA gap check
  mm/mseal: rework mseal apply logic

 include/linux/mm.h                      |   6 +-
 mm/madvise.c                            |  63 ++++++++-
 mm/mseal.c                              | 169 ++++++------------------
 mm/vma.h                                |  23 +---
 tools/testing/selftests/mm/mseal_test.c |   3 +-
 tools/testing/vma/vma_internal.h        |   6 +-
 6 files changed, 110 insertions(+), 160 deletions(-)

--
2.50.1

[PATCH v2 1/5] mm/mseal: always define VM_SEALED

[Lorenzo Stoakes]

There is no reason to treat VM_SEALED in a special way, in each other case
in which a VMA flag is unavailable due to configuration, we simply assign
that flag to VM_NONE, so make VM_SEALED consistent with all other VMA flags
in this respect.

Additionally, use the next available bit for VM_SEALED, 42, rather than
arbitrarily putting it at 63 and update the declaration to match all other
VMA flags.

No functional change intended.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>
Reviewed-by: Pedro Falcato <pfalcato@suse.de>
Acked-by: David Hildenbrand <david@redhat.com>
---
 include/linux/mm.h               | 6 ++++--
 tools/testing/vma/vma_internal.h | 6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 2e5459d43267..aba67c3df42b 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -414,8 +414,10 @@ extern unsigned int kobjsize(const void *objp);
 #endif

 #ifdef CONFIG_64BIT
-/* VM is sealed, in vm_flags */
-#define VM_SEALED	_BITUL(63)
+#define VM_SEALED_BIT	42
+#define VM_SEALED	BIT(VM_SEALED_BIT)
+#else
+#define VM_SEALED	VM_NONE
 #endif

 /* Bits set in the VMA until the stack is in its final location */
diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
index 991022e9e0d3..0fe52fd6782b 100644
--- a/tools/testing/vma/vma_internal.h
+++ b/tools/testing/vma/vma_internal.h
@@ -108,8 +108,10 @@ extern unsigned long dac_mmap_min_addr;
 #define CAP_IPC_LOCK         14

 #ifdef CONFIG_64BIT
-/* VM is sealed, in vm_flags */
-#define VM_SEALED	_BITUL(63)
+#define VM_SEALED_BIT	42
+#define VM_SEALED	BIT(VM_SEALED_BIT)
+#else
+#define VM_SEALED	VM_NONE
 #endif

 #define FIRST_USER_ADDRESS	0UL
--
2.50.1

[PATCH v2 2/5] mm/mseal: update madvise() logic

[Lorenzo Stoakes]

The madvise() logic is inexplicably performed in mm/mseal.c - this ought to
be located in mm/madvise.c.

Additionally can_modify_vma_madv() is inconsistently named and, in
combination with is_ro_anon(), is very confusing logic.

Put a static function in mm/madvise.c instead - can_madvise_modify() - that
spells out exactly what's happening. Also explicitly check for an anon VMA.

Also add commentary to explain what's going on.

Essentially - we disallow discarding of data in mseal()'d mappings in
instances where the user couldn't otherwise write to that data.

Shared mappings are always backed, so no discard will actually truly
discard the data. Read-only anonymous and MAP_PRIVATE file-backed mappings
are the ones we are interested in.

We make a change to the logic here to correct a mistake - we must disallow
discard of read-only MAP_PRIVATE file-backed mappings, which previously we
were not.

The justification for this change is to account for the case where:

1. A MAP_PRIVATE R/W file-backed mapping is established.
2. The mapping is written to, which backs it with anonymous memory.
3. The mapping is mprotect()'d read-only.
4. The mapping is mseal()'d.

If we were to now allow discard of this data, it would mean mseal() would
not prevent the unrecoverable discarding of data and it was thus violate
the semantics of sealed VMAs.

Finally, update the mseal tests, which were asserting previously that a
read-only MAP_PRIVATE file-backed mapping could be discarded.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>
Reviewed-by: Pedro Falcato <pfalcato@suse.de>
---
 mm/madvise.c                            | 63 ++++++++++++++++++++++++-
 mm/mseal.c                              | 49 -------------------
 mm/vma.h                                |  7 ---
 tools/testing/selftests/mm/mseal_test.c |  3 +-
 4 files changed, 63 insertions(+), 59 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index 9de9b7c797c6..89205693b67a 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -19,6 +19,7 @@
 #include <linux/sched.h>
 #include <linux/sched/mm.h>
 #include <linux/mm_inline.h>
+#include <linux/mmu_context.h>
 #include <linux/string.h>
 #include <linux/uio.h>
 #include <linux/ksm.h>
@@ -1256,6 +1257,66 @@ static long madvise_guard_remove(struct madvise_behavior *madv_behavior)
 			       &guard_remove_walk_ops, NULL);
 }
 
+#ifdef CONFIG_64BIT
+/* Does the madvise operation result in discarding of mapped data? */
+static bool is_discard(int behavior)
+{
+	switch (behavior) {
+	case MADV_FREE:
+	case MADV_DONTNEED:
+	case MADV_DONTNEED_LOCKED:
+	case MADV_REMOVE:
+	case MADV_DONTFORK:
+	case MADV_WIPEONFORK:
+	case MADV_GUARD_INSTALL:
+		return true;
+	}
+
+	return false;
+}
+
+/*
+ * We are restricted from madvise()'ing mseal()'d VMAs only in very particular
+ * circumstances - discarding of data from read-only anonymous SEALED mappings.
+ *
+ * This is because users cannot trivally discard data from these VMAs, and may
+ * only do so via an appropriate madvise() call.
+ */
+static bool can_madvise_modify(struct madvise_behavior *madv_behavior)
+{
+	struct vm_area_struct *vma = madv_behavior->vma;
+
+	/* If the VMA isn't sealed we're good. */
+	if (can_modify_vma(vma))
+		return true;
+
+	/* For a sealed VMA, we only care about discard operations. */
+	if (!is_discard(madv_behavior->behavior))
+		return true;
+
+	/*
+	 * But shared mappings are fine, as dirty pages will be written to a
+	 * backing store regardless of discard.
+	 */
+	if (vma->vm_flags & VM_SHARED)
+		return true;
+
+	/* If the user could write to the mapping anyway, then this is fine. */
+	if ((vma->vm_flags & VM_WRITE) &&
+	    arch_vma_access_permitted(vma, /* write= */ true,
+			/* execute= */ false, /* foreign= */ false))
+		return true;
+
+	/* Otherwise, we are not permitted to perform this operation. */
+	return false;
+}
+#else
+static bool can_madvise_modify(struct madvise_behavior *madv_behavior)
+{
+	return true;
+}
+#endif
+
 /*
  * Apply an madvise behavior to a region of a vma.  madvise_update_vma
  * will handle splitting a vm area into separate areas, each area with its own
@@ -1269,7 +1330,7 @@ static int madvise_vma_behavior(struct madvise_behavior *madv_behavior)
 	struct madvise_behavior_range *range = &madv_behavior->range;
 	int error;
 
-	if (unlikely(!can_modify_vma_madv(madv_behavior->vma, behavior)))
+	if (unlikely(!can_madvise_modify(madv_behavior)))
 		return -EPERM;
 
 	switch (behavior) {
diff --git a/mm/mseal.c b/mm/mseal.c
index c27197ac04e8..1308e88ab184 100644
--- a/mm/mseal.c
+++ b/mm/mseal.c
@@ -11,7 +11,6 @@
 #include <linux/mman.h>
 #include <linux/mm.h>
 #include <linux/mm_inline.h>
-#include <linux/mmu_context.h>
 #include <linux/syscalls.h>
 #include <linux/sched.h>
 #include "internal.h"
@@ -21,54 +20,6 @@ static inline void set_vma_sealed(struct vm_area_struct *vma)
 	vm_flags_set(vma, VM_SEALED);
 }
 
-static bool is_madv_discard(int behavior)
-{
-	switch (behavior) {
-	case MADV_FREE:
-	case MADV_DONTNEED:
-	case MADV_DONTNEED_LOCKED:
-	case MADV_REMOVE:
-	case MADV_DONTFORK:
-	case MADV_WIPEONFORK:
-	case MADV_GUARD_INSTALL:
-		return true;
-	}
-
-	return false;
-}
-
-static bool is_ro_anon(struct vm_area_struct *vma)
-{
-	/* check anonymous mapping. */
-	if (vma->vm_file || vma->vm_flags & VM_SHARED)
-		return false;
-
-	/*
-	 * check for non-writable:
-	 * PROT=RO or PKRU is not writeable.
-	 */
-	if (!(vma->vm_flags & VM_WRITE) ||
-		!arch_vma_access_permitted(vma, true, false, false))
-		return true;
-
-	return false;
-}
-
-/*
- * Check if a vma is allowed to be modified by madvise.
- */
-bool can_modify_vma_madv(struct vm_area_struct *vma, int behavior)
-{
-	if (!is_madv_discard(behavior))
-		return true;
-
-	if (unlikely(!can_modify_vma(vma) && is_ro_anon(vma)))
-		return false;
-
-	/* Allow by default. */
-	return true;
-}
-
 static int mseal_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
 		struct vm_area_struct **prev, unsigned long start,
 		unsigned long end, vm_flags_t newflags)
diff --git a/mm/vma.h b/mm/vma.h
index cf6e3a6371b6..6515045ba342 100644
--- a/mm/vma.h
+++ b/mm/vma.h
@@ -578,8 +578,6 @@ static inline bool can_modify_vma(struct vm_area_struct *vma)
 	return true;
 }
 
-bool can_modify_vma_madv(struct vm_area_struct *vma, int behavior);
-
 #else
 
 static inline bool can_modify_vma(struct vm_area_struct *vma)
@@ -587,11 +585,6 @@ static inline bool can_modify_vma(struct vm_area_struct *vma)
 	return true;
 }
 
-static inline bool can_modify_vma_madv(struct vm_area_struct *vma, int behavior)
-{
-	return true;
-}
-
 #endif
 
 #if defined(CONFIG_STACK_GROWSUP)
diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
index 005f29c86484..34c042da4de2 100644
--- a/tools/testing/selftests/mm/mseal_test.c
+++ b/tools/testing/selftests/mm/mseal_test.c
@@ -1712,7 +1712,6 @@ static void test_seal_discard_ro_anon_on_filebacked(bool seal)
 	unsigned long size = 4 * page_size;
 	int ret;
 	int fd;
-	unsigned long mapflags = MAP_PRIVATE;
 
 	fd = memfd_create("test", 0);
 	FAIL_TEST_IF_FALSE(fd > 0);
@@ -1720,7 +1719,7 @@ static void test_seal_discard_ro_anon_on_filebacked(bool seal)
 	ret = fallocate(fd, 0, 0, size);
 	FAIL_TEST_IF_FALSE(!ret);
 
-	ptr = mmap(NULL, size, PROT_READ, mapflags, fd, 0);
+	ptr = mmap(NULL, size, PROT_READ, MAP_SHARED, fd, 0);
 	FAIL_TEST_IF_FALSE(ptr != MAP_FAILED);
 
 	if (seal) {
-- 
2.50.1

[PATCH v2 3/5] mm/mseal: small cleanups

[Lorenzo Stoakes]

Drop the wholly unnecessary set_vma_sealed() and vma_is_sealed() helpers
which are used only once, and place VMA_ITERATOR() declarations in the
correct place.

No functional change intended.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>
Reviewed-by: Pedro Falcato <pfalcato@suse.de>
Acked-by: David Hildenbrand <david@redhat.com>
---
 mm/mseal.c |  9 +--------
 mm/vma.h   | 16 ++--------------
 2 files changed, 3 insertions(+), 22 deletions(-)

diff --git a/mm/mseal.c b/mm/mseal.c
index 1308e88ab184..adbcc65e9660 100644
--- a/mm/mseal.c
+++ b/mm/mseal.c
@@ -15,11 +15,6 @@
 #include <linux/sched.h>
 #include "internal.h"

-static inline void set_vma_sealed(struct vm_area_struct *vma)
-{
-	vm_flags_set(vma, VM_SEALED);
-}
-
 static int mseal_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
 		struct vm_area_struct **prev, unsigned long start,
 		unsigned long end, vm_flags_t newflags)
@@ -36,7 +31,7 @@ static int mseal_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
 		goto out;
 	}

-	set_vma_sealed(vma);
+	vm_flags_set(vma, VM_SEALED);
 out:
 	*prev = vma;
 	return ret;
@@ -53,7 +48,6 @@ static int check_mm_seal(unsigned long start, unsigned long end)
 {
 	struct vm_area_struct *vma;
 	unsigned long nstart = start;
-
 	VMA_ITERATOR(vmi, current->mm, start);

 	/* going through each vma to check. */
@@ -78,7 +72,6 @@ static int apply_mm_seal(unsigned long start, unsigned long end)
 {
 	unsigned long nstart;
 	struct vm_area_struct *vma, *prev;
-
 	VMA_ITERATOR(vmi, current->mm, start);

 	vma = vma_iter_load(&vmi);
diff --git a/mm/vma.h b/mm/vma.h
index 6515045ba342..d17f560cf53d 100644
--- a/mm/vma.h
+++ b/mm/vma.h
@@ -560,31 +560,19 @@ struct vm_area_struct *vma_iter_next_rewind(struct vma_iterator *vmi,
 }

 #ifdef CONFIG_64BIT
-
-static inline bool vma_is_sealed(struct vm_area_struct *vma)
-{
-	return (vma->vm_flags & VM_SEALED);
-}
-
-/*
- * check if a vma is sealed for modification.
- * return true, if modification is allowed.
- */
+/* Check if a vma is sealed for modification. */
 static inline bool can_modify_vma(struct vm_area_struct *vma)
 {
-	if (unlikely(vma_is_sealed(vma)))
+	if (unlikely(vma->vm_flags & VM_SEALED))
 		return false;

 	return true;
 }
-
 #else
-
 static inline bool can_modify_vma(struct vm_area_struct *vma)
 {
 	return true;
 }
-
 #endif

 #if defined(CONFIG_STACK_GROWSUP)
--
2.50.1

[PATCH v2 4/5] mm/mseal: Simplify and rename VMA gap check

[Lorenzo Stoakes]

The check_mm_seal() function is doing something general - checking whether
a range contains only VMAs (or rather that it does NOT contain any unmapped
regions).

So rename this function to range_contains_unmapped().

Additionally simplify the logic, we are simply checking whether the last
vma->vm_end has either a VMA starting after it or ends before the end
parameter.

This check is rather dubious, so it is sensible to keep it local to
mm/mseal.c as at a later stage it may be removed, and we don't want any
other mm code to perform such a check.

No functional change intended.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>
Acked-by: David Hildenbrand <david@redhat.com>
---
 mm/mseal.c | 56 ++++++++++++++++++++----------------------------------
 1 file changed, 21 insertions(+), 35 deletions(-)

diff --git a/mm/mseal.c b/mm/mseal.c
index adbcc65e9660..794d1043a706 100644
--- a/mm/mseal.c
+++ b/mm/mseal.c
@@ -37,34 +37,6 @@ static int mseal_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
 	return ret;
 }

-/*
- * Check for do_mseal:
- * 1> start is part of a valid vma.
- * 2> end is part of a valid vma.
- * 3> No gap (unallocated address) between start and end.
- * 4> map is sealable.
- */
-static int check_mm_seal(unsigned long start, unsigned long end)
-{
-	struct vm_area_struct *vma;
-	unsigned long nstart = start;
-	VMA_ITERATOR(vmi, current->mm, start);
-
-	/* going through each vma to check. */
-	for_each_vma_range(vmi, vma, end) {
-		if (vma->vm_start > nstart)
-			/* unallocated memory found. */
-			return -ENOMEM;
-
-		if (vma->vm_end >= end)
-			return 0;
-
-		nstart = vma->vm_end;
-	}
-
-	return -ENOMEM;
-}
-
 /*
  * Apply sealing.
  */
@@ -102,6 +74,24 @@ static int apply_mm_seal(unsigned long start, unsigned long end)
 	return 0;
 }

+/* Does the [start, end) range contain any unmapped memory? */
+static bool range_contains_unmapped(struct mm_struct *mm,
+		unsigned long start, unsigned long end)
+{
+	struct vm_area_struct *vma;
+	unsigned long prev_end = start;
+	VMA_ITERATOR(vmi, current->mm, start);
+
+	for_each_vma_range(vmi, vma, end) {
+		if (vma->vm_start > prev_end)
+			return true;
+
+		prev_end = vma->vm_end;
+	}
+
+	return prev_end < end;
+}
+
 /*
  * mseal(2) seals the VM's meta data from
  * selected syscalls.
@@ -184,14 +174,10 @@ int do_mseal(unsigned long start, size_t len_in, unsigned long flags)
 	if (mmap_write_lock_killable(mm))
 		return -EINTR;

-	/*
-	 * First pass, this helps to avoid
-	 * partial sealing in case of error in input address range,
-	 * e.g. ENOMEM error.
-	 */
-	ret = check_mm_seal(start, end);
-	if (ret)
+	if (range_contains_unmapped(mm, start, end)) {
+		ret = -ENOMEM;
 		goto out;
+	}

 	/*
 	 * Second pass, this should success, unless there are errors
--
2.50.1

[PATCH v2 5/5] mm/mseal: rework mseal apply logic

[Lorenzo Stoakes]

The logic can be simplified - firstly by renaming the inconsistently named
apply_mm_seal() to mseal_apply().

We then wrap mseal_fixup() into the main loop as the logic is simple enough
to not require it, equally it isn't a hugely pleasant pattern in mprotect()
etc. so it's not something we want to perpetuate.

We remove some redundant comments, and then avoid the entirely unnecessary
and slightly bizarre invocation of vma_iter_end() on each loop - really
what we want, given we have asserted there are no gaps in the range - is to
handle start, end being offset into a VMAs. This is easily handled with
MIN()/MAX().

There's no need to have an 'out' label block since on vma_modify_flags()
error we abort anyway.

And by refactoring like this we avoid the rather horrid 'pass pointer to
prev around' pattern used in mprotect() et al.

No functional change intended.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Reviewed-by: Pedro Falcato <pfalcato@suse.de>
---
 mm/mseal.c | 69 +++++++++++++++++-------------------------------------
 1 file changed, 22 insertions(+), 47 deletions(-)

diff --git a/mm/mseal.c b/mm/mseal.c
index 794d1043a706..e0fe37623632 100644
--- a/mm/mseal.c
+++ b/mm/mseal.c
@@ -15,60 +15,35 @@
 #include <linux/sched.h>
 #include "internal.h"

-static int mseal_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
-		struct vm_area_struct **prev, unsigned long start,
-		unsigned long end, vm_flags_t newflags)
-{
-	int ret = 0;
-	vm_flags_t oldflags = vma->vm_flags;
-
-	if (newflags == oldflags)
-		goto out;
-
-	vma = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
-	if (IS_ERR(vma)) {
-		ret = PTR_ERR(vma);
-		goto out;
-	}
-
-	vm_flags_set(vma, VM_SEALED);
-out:
-	*prev = vma;
-	return ret;
-}
-
-/*
- * Apply sealing.
- */
-static int apply_mm_seal(unsigned long start, unsigned long end)
+static int mseal_apply(struct mm_struct *mm,
+		unsigned long start, unsigned long end)
 {
-	unsigned long nstart;
 	struct vm_area_struct *vma, *prev;
-	VMA_ITERATOR(vmi, current->mm, start);
+	VMA_ITERATOR(vmi, mm, start);

+	/* We know there are no gaps so this will be non-NULL. */
 	vma = vma_iter_load(&vmi);
-	/*
-	 * Note: check_mm_seal should already checked ENOMEM case.
-	 * so vma should not be null, same for the other ENOMEM cases.
-	 */
 	prev = vma_prev(&vmi);
 	if (start > vma->vm_start)
 		prev = vma;

-	nstart = start;
 	for_each_vma_range(vmi, vma, end) {
-		int error;
-		unsigned long tmp;
-		vm_flags_t newflags;
-
-		newflags = vma->vm_flags | VM_SEALED;
-		tmp = vma->vm_end;
-		if (tmp > end)
-			tmp = end;
-		error = mseal_fixup(&vmi, vma, &prev, nstart, tmp, newflags);
-		if (error)
-			return error;
-		nstart = vma_iter_end(&vmi);
+		unsigned long curr_start, curr_end;
+
+		if (vma->vm_flags & VM_SEALED) {
+			prev = vma;
+			continue;
+		}
+		curr_start = MAX(start, vma->vm_start);
+		curr_end = MIN(vma->vm_end, end);
+
+		vma = vma_modify_flags(&vmi, prev, vma, curr_start, curr_end,
+				vma->vm_flags | VM_SEALED);
+		if (IS_ERR(vma))
+			return PTR_ERR(vma);
+		vm_flags_set(vma, VM_SEALED);
+
+		prev = vma;
 	}

 	return 0;
@@ -185,10 +160,10 @@ int do_mseal(unsigned long start, size_t len_in, unsigned long flags)
 	 * reaching the max supported VMAs, however, those cases shall
 	 * be rare.
 	 */
-	ret = apply_mm_seal(start, end);
+	ret = mseal_apply(mm, start, end);

 out:
-	mmap_write_unlock(current->mm);
+	mmap_write_unlock(mm);
 	return ret;
 }

--
2.50.1

Re: [PATCH v2 5/5] mm/mseal: rework mseal apply logic

[Liam R. Howlett]

* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250715 09:37]:
> The logic can be simplified - firstly by renaming the inconsistently named
> apply_mm_seal() to mseal_apply().
> 
> We then wrap mseal_fixup() into the main loop as the logic is simple enough
> to not require it, equally it isn't a hugely pleasant pattern in mprotect()
> etc. so it's not something we want to perpetuate.
> 
> We remove some redundant comments, and then avoid the entirely unnecessary
> and slightly bizarre invocation of vma_iter_end() on each loop - really
> what we want, given we have asserted there are no gaps in the range - is to
> handle start, end being offset into a VMAs. This is easily handled with
> MIN()/MAX().

The vma_iter_end() was to detect a merge with the next vma and use the
new end, since the code you are replacing didn't update the vma pointer.

> 
> There's no need to have an 'out' label block since on vma_modify_flags()
> error we abort anyway.

Ah, in the mseal_fixup()/mseal_apply().  The out label still exists (and
appears in the below patch) in do_mseal().


> 
> And by refactoring like this we avoid the rather horrid 'pass pointer to
> prev around' pattern used in mprotect() et al.
> 
> No functional change intended.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Reviewed-by: Pedro Falcato <pfalcato@suse.de>
> ---
>  mm/mseal.c | 69 +++++++++++++++++-------------------------------------
>  1 file changed, 22 insertions(+), 47 deletions(-)
> 
> diff --git a/mm/mseal.c b/mm/mseal.c
> index 794d1043a706..e0fe37623632 100644
> --- a/mm/mseal.c
> +++ b/mm/mseal.c
> @@ -15,60 +15,35 @@
>  #include <linux/sched.h>
>  #include "internal.h"
> 
> -static int mseal_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
> -		struct vm_area_struct **prev, unsigned long start,
> -		unsigned long end, vm_flags_t newflags)
> -{
> -	int ret = 0;
> -	vm_flags_t oldflags = vma->vm_flags;
> -
> -	if (newflags == oldflags)
> -		goto out;
> -
> -	vma = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
> -	if (IS_ERR(vma)) {
> -		ret = PTR_ERR(vma);
> -		goto out;
> -	}
> -
> -	vm_flags_set(vma, VM_SEALED);
> -out:
> -	*prev = vma;
> -	return ret;
> -}
> -
> -/*
> - * Apply sealing.
> - */
> -static int apply_mm_seal(unsigned long start, unsigned long end)
> +static int mseal_apply(struct mm_struct *mm,
> +		unsigned long start, unsigned long end)
>  {
> -	unsigned long nstart;
>  	struct vm_area_struct *vma, *prev;
> -	VMA_ITERATOR(vmi, current->mm, start);
> +	VMA_ITERATOR(vmi, mm, start);
> 
> +	/* We know there are no gaps so this will be non-NULL. */
>  	vma = vma_iter_load(&vmi);
> -	/*
> -	 * Note: check_mm_seal should already checked ENOMEM case.
> -	 * so vma should not be null, same for the other ENOMEM cases.
> -	 */
>  	prev = vma_prev(&vmi);
>  	if (start > vma->vm_start)
>  		prev = vma;
> 
> -	nstart = start;
>  	for_each_vma_range(vmi, vma, end) {
> -		int error;
> -		unsigned long tmp;
> -		vm_flags_t newflags;
> -
> -		newflags = vma->vm_flags | VM_SEALED;
> -		tmp = vma->vm_end;
> -		if (tmp > end)
> -			tmp = end;
> -		error = mseal_fixup(&vmi, vma, &prev, nstart, tmp, newflags);
> -		if (error)
> -			return error;
> -		nstart = vma_iter_end(&vmi);
> +		unsigned long curr_start, curr_end;
> +
> +		if (vma->vm_flags & VM_SEALED) {
> +			prev = vma;
> +			continue;
> +		}
> +		curr_start = MAX(start, vma->vm_start);
> +		curr_end = MIN(vma->vm_end, end);

If you assigned curr_start outside the loop, you could just set it to
vma->vm_end after you change the flags, this would reduce instructions
in the loop.  But this is hardly a performance critical path.

And we probably hardly ever execute this loop more than once.

> +
> +		vma = vma_modify_flags(&vmi, prev, vma, curr_start, curr_end,
> +				vma->vm_flags | VM_SEALED);
> +		if (IS_ERR(vma))
> +			return PTR_ERR(vma);
> +		vm_flags_set(vma, VM_SEALED);
> +
> +		prev = vma;
>  	}
> 
>  	return 0;
> @@ -185,10 +160,10 @@ int do_mseal(unsigned long start, size_t len_in, unsigned long flags)
>  	 * reaching the max supported VMAs, however, those cases shall
>  	 * be rare.
>  	 */
> -	ret = apply_mm_seal(start, end);
> +	ret = mseal_apply(mm, start, end);
> 
>  out:
> -	mmap_write_unlock(current->mm);
> +	mmap_write_unlock(mm);

It is nice to see the local variable being used.


Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

>  	return ret;
>  }
> 
> --
> 2.50.1

Re: [PATCH v2 2/5] mm/mseal: update madvise() logic

[David Hildenbrand]

On 15.07.25 15:37, Lorenzo Stoakes wrote:
> The madvise() logic is inexplicably performed in mm/mseal.c - this ought to
> be located in mm/madvise.c.
> 
> Additionally can_modify_vma_madv() is inconsistently named and, in
> combination with is_ro_anon(), is very confusing logic.
> 
> Put a static function in mm/madvise.c instead - can_madvise_modify() - that
> spells out exactly what's happening. Also explicitly check for an anon VMA.
> 
> Also add commentary to explain what's going on.
> 
> Essentially - we disallow discarding of data in mseal()'d mappings in
> instances where the user couldn't otherwise write to that data.
> 
> Shared mappings are always backed, so no discard will actually truly
> discard the data. Read-only anonymous and MAP_PRIVATE file-backed mappings
> are the ones we are interested in.
> 
> We make a change to the logic here to correct a mistake - we must disallow
> discard of read-only MAP_PRIVATE file-backed mappings, which previously we
> were not.
> 
> The justification for this change is to account for the case where:
> 
> 1. A MAP_PRIVATE R/W file-backed mapping is established.
> 2. The mapping is written to, which backs it with anonymous memory.
> 3. The mapping is mprotect()'d read-only.
> 4. The mapping is mseal()'d.
> 
> If we were to now allow discard of this data, it would mean mseal() would
> not prevent the unrecoverable discarding of data and it was thus violate
> the semantics of sealed VMAs.
> 
> Finally, update the mseal tests, which were asserting previously that a
> read-only MAP_PRIVATE file-backed mapping could be discarded.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> Reviewed-by: Pedro Falcato <pfalcato@suse.de>
> ---

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 4/5] mm/mseal: Simplify and rename VMA gap check

[David Hildenbrand]

On 15.07.25 15:37, Lorenzo Stoakes wrote:
> The check_mm_seal() function is doing something general - checking whether
> a range contains only VMAs (or rather that it does NOT contain any unmapped
> regions).
> 
> So rename this function to range_contains_unmapped().
> 
> Additionally simplify the logic, we are simply checking whether the last
> vma->vm_end has either a VMA starting after it or ends before the end
> parameter.
> 
> This check is rather dubious, so it is sensible to keep it local to
> mm/mseal.c as at a later stage it may be removed, and we don't want any
> other mm code to perform such a check.
> 
> No functional change intended.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> Acked-by: David Hildenbrand <david@redhat.com>
> ---
>   mm/mseal.c | 56 ++++++++++++++++++++----------------------------------
>   1 file changed, 21 insertions(+), 35 deletions(-)
> 
> diff --git a/mm/mseal.c b/mm/mseal.c
> index adbcc65e9660..794d1043a706 100644
> --- a/mm/mseal.c
> +++ b/mm/mseal.c
> @@ -37,34 +37,6 @@ static int mseal_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
>   	return ret;
>   }
> 
> -/*
> - * Check for do_mseal:
> - * 1> start is part of a valid vma.
> - * 2> end is part of a valid vma.
> - * 3> No gap (unallocated address) between start and end.
> - * 4> map is sealable.
> - */
> -static int check_mm_seal(unsigned long start, unsigned long end)
> -{
> -	struct vm_area_struct *vma;
> -	unsigned long nstart = start;
> -	VMA_ITERATOR(vmi, current->mm, start);
> -
> -	/* going through each vma to check. */
> -	for_each_vma_range(vmi, vma, end) {
> -		if (vma->vm_start > nstart)
> -			/* unallocated memory found. */
> -			return -ENOMEM;
> -
> -		if (vma->vm_end >= end)
> -			return 0;
> -
> -		nstart = vma->vm_end;
> -	}
> -
> -	return -ENOMEM;
> -}
> -
>   /*
>    * Apply sealing.
>    */
> @@ -102,6 +74,24 @@ static int apply_mm_seal(unsigned long start, unsigned long end)
>   	return 0;
>   }
> 
> +/* Does the [start, end) range contain any unmapped memory? */
> +static bool range_contains_unmapped(struct mm_struct *mm,
> +		unsigned long start, unsigned long end)
> +{
> +	struct vm_area_struct *vma;
> +	unsigned long prev_end = start;
> +	VMA_ITERATOR(vmi, current->mm, start);
> +
> +	for_each_vma_range(vmi, vma, end) {
> +		if (vma->vm_start > prev_end)
> +			return true;
> +
> +		prev_end = vma->vm_end;
> +	}
> +
> +	return prev_end < end;
> +}
> +

Probably better to not ... move the function in the same file? Then, we 
can se the actual diff of changes easily.

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 5/5] mm/mseal: rework mseal apply logic

[David Hildenbrand]

On 15.07.25 15:37, Lorenzo Stoakes wrote:
> The logic can be simplified - firstly by renaming the inconsistently named
> apply_mm_seal() to mseal_apply().
> 
> We then wrap mseal_fixup() into the main loop as the logic is simple enough
> to not require it, equally it isn't a hugely pleasant pattern in mprotect()
> etc. so it's not something we want to perpetuate.
> 
> We remove some redundant comments, and then avoid the entirely unnecessary
> and slightly bizarre invocation of vma_iter_end() on each loop - really
> what we want, given we have asserted there are no gaps in the range - is to
> handle start, end being offset into a VMAs. This is easily handled with
> MIN()/MAX().
> 
> There's no need to have an 'out' label block since on vma_modify_flags()
> error we abort anyway.
> 
> And by refactoring like this we avoid the rather horrid 'pass pointer to
> prev around' pattern used in mprotect() et al.
> 
> No functional change intended.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Reviewed-by: Pedro Falcato <pfalcato@suse.de>
> ---

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 5/5] mm/mseal: rework mseal apply logic

[Lorenzo Stoakes]

Sorry I actually replied to this yestreday, but then closed down emacs and
turned my PC off before I had sent it mistakenly thinking I had :P doh!

On Tue, Jul 15, 2025 at 12:09:10PM -0400, Liam R. Howlett wrote:
> * Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250715 09:37]:
> > The logic can be simplified - firstly by renaming the inconsistently named
> > apply_mm_seal() to mseal_apply().
> >
> > We then wrap mseal_fixup() into the main loop as the logic is simple enough
> > to not require it, equally it isn't a hugely pleasant pattern in mprotect()
> > etc. so it's not something we want to perpetuate.
> >
> > We remove some redundant comments, and then avoid the entirely unnecessary
> > and slightly bizarre invocation of vma_iter_end() on each loop - really
> > what we want, given we have asserted there are no gaps in the range - is to
> > handle start, end being offset into a VMAs. This is easily handled with
> > MIN()/MAX().
>
> The vma_iter_end() was to detect a merge with the next vma and use the
> new end, since the code you are replacing didn't update the vma pointer.

Ah interesting, good that we now no longer need to do this!

>
> >
> > There's no need to have an 'out' label block since on vma_modify_flags()
> > error we abort anyway.
>
> Ah, in the mseal_fixup()/mseal_apply().  The out label still exists (and
> appears in the below patch) in do_mseal().

Yeah indeed this is what I was referring to :)

>
>
> >
> > And by refactoring like this we avoid the rather horrid 'pass pointer to
> > prev around' pattern used in mprotect() et al.
> >
> > No functional change intended.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > Reviewed-by: Pedro Falcato <pfalcato@suse.de>
> > ---
> >  mm/mseal.c | 69 +++++++++++++++++-------------------------------------
> >  1 file changed, 22 insertions(+), 47 deletions(-)
> >
> > diff --git a/mm/mseal.c b/mm/mseal.c
> > index 794d1043a706..e0fe37623632 100644
> > --- a/mm/mseal.c
> > +++ b/mm/mseal.c
> > @@ -15,60 +15,35 @@
> >  #include <linux/sched.h>
> >  #include "internal.h"
> >
> > -static int mseal_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
> > -		struct vm_area_struct **prev, unsigned long start,
> > -		unsigned long end, vm_flags_t newflags)
> > -{
> > -	int ret = 0;
> > -	vm_flags_t oldflags = vma->vm_flags;
> > -
> > -	if (newflags == oldflags)
> > -		goto out;
> > -
> > -	vma = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
> > -	if (IS_ERR(vma)) {
> > -		ret = PTR_ERR(vma);
> > -		goto out;
> > -	}
> > -
> > -	vm_flags_set(vma, VM_SEALED);
> > -out:
> > -	*prev = vma;
> > -	return ret;
> > -}
> > -
> > -/*
> > - * Apply sealing.
> > - */
> > -static int apply_mm_seal(unsigned long start, unsigned long end)
> > +static int mseal_apply(struct mm_struct *mm,
> > +		unsigned long start, unsigned long end)
> >  {
> > -	unsigned long nstart;
> >  	struct vm_area_struct *vma, *prev;
> > -	VMA_ITERATOR(vmi, current->mm, start);
> > +	VMA_ITERATOR(vmi, mm, start);
> >
> > +	/* We know there are no gaps so this will be non-NULL. */
> >  	vma = vma_iter_load(&vmi);
> > -	/*
> > -	 * Note: check_mm_seal should already checked ENOMEM case.
> > -	 * so vma should not be null, same for the other ENOMEM cases.
> > -	 */
> >  	prev = vma_prev(&vmi);
> >  	if (start > vma->vm_start)
> >  		prev = vma;
> >
> > -	nstart = start;
> >  	for_each_vma_range(vmi, vma, end) {
> > -		int error;
> > -		unsigned long tmp;
> > -		vm_flags_t newflags;
> > -
> > -		newflags = vma->vm_flags | VM_SEALED;
> > -		tmp = vma->vm_end;
> > -		if (tmp > end)
> > -			tmp = end;
> > -		error = mseal_fixup(&vmi, vma, &prev, nstart, tmp, newflags);
> > -		if (error)
> > -			return error;
> > -		nstart = vma_iter_end(&vmi);
> > +		unsigned long curr_start, curr_end;
> > +
> > +		if (vma->vm_flags & VM_SEALED) {
> > +			prev = vma;
> > +			continue;
> > +		}
> > +		curr_start = MAX(start, vma->vm_start);
> > +		curr_end = MIN(vma->vm_end, end);
>
> If you assigned curr_start outside the loop, you could just set it to
> vma->vm_end after you change the flags, this would reduce instructions
> in the loop.  But this is hardly a performance critical path.
>
> And we probably hardly ever execute this loop more than once.

True, could do a curr_start = curr_end too which is sort of nicely
self-documenting as well.

I need to respin anyway based on other feedback so will do this!

>
> > +
> > +		vma = vma_modify_flags(&vmi, prev, vma, curr_start, curr_end,
> > +				vma->vm_flags | VM_SEALED);
> > +		if (IS_ERR(vma))
> > +			return PTR_ERR(vma);
> > +		vm_flags_set(vma, VM_SEALED);
> > +
> > +		prev = vma;
> >  	}
> >
> >  	return 0;
> > @@ -185,10 +160,10 @@ int do_mseal(unsigned long start, size_t len_in, unsigned long flags)
> >  	 * reaching the max supported VMAs, however, those cases shall
> >  	 * be rare.
> >  	 */
> > -	ret = apply_mm_seal(start, end);
> > +	ret = mseal_apply(mm, start, end);
> >
> >  out:
> > -	mmap_write_unlock(current->mm);
> > +	mmap_write_unlock(mm);
>
> It is nice to see the local variable being used.

:) yeah good to refactor that.

>
>
> Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

Thanks!

>
> >  	return ret;
> >  }
> >
> > --
> > 2.50.1

Re: [PATCH v2 4/5] mm/mseal: Simplify and rename VMA gap check

[Lorenzo Stoakes]

On Wed, Jul 16, 2025 at 03:38:51PM +0200, David Hildenbrand wrote:
> On 15.07.25 15:37, Lorenzo Stoakes wrote:
> > The check_mm_seal() function is doing something general - checking whether
> > a range contains only VMAs (or rather that it does NOT contain any unmapped
> > regions).
> >
> > So rename this function to range_contains_unmapped().
> >
> > Additionally simplify the logic, we are simply checking whether the last
> > vma->vm_end has either a VMA starting after it or ends before the end
> > parameter.
> >
> > This check is rather dubious, so it is sensible to keep it local to
> > mm/mseal.c as at a later stage it may be removed, and we don't want any
> > other mm code to perform such a check.
> >
> > No functional change intended.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> > Acked-by: David Hildenbrand <david@redhat.com>
> > ---
> >   mm/mseal.c | 56 ++++++++++++++++++++----------------------------------
> >   1 file changed, 21 insertions(+), 35 deletions(-)
> >
> > diff --git a/mm/mseal.c b/mm/mseal.c
> > index adbcc65e9660..794d1043a706 100644
> > --- a/mm/mseal.c
> > +++ b/mm/mseal.c
> > @@ -37,34 +37,6 @@ static int mseal_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
> >   	return ret;
> >   }
> >
> > -/*
> > - * Check for do_mseal:
> > - * 1> start is part of a valid vma.
> > - * 2> end is part of a valid vma.
> > - * 3> No gap (unallocated address) between start and end.
> > - * 4> map is sealable.
> > - */
> > -static int check_mm_seal(unsigned long start, unsigned long end)
> > -{
> > -	struct vm_area_struct *vma;
> > -	unsigned long nstart = start;
> > -	VMA_ITERATOR(vmi, current->mm, start);
> > -
> > -	/* going through each vma to check. */
> > -	for_each_vma_range(vmi, vma, end) {
> > -		if (vma->vm_start > nstart)
> > -			/* unallocated memory found. */
> > -			return -ENOMEM;
> > -
> > -		if (vma->vm_end >= end)
> > -			return 0;
> > -
> > -		nstart = vma->vm_end;
> > -	}
> > -
> > -	return -ENOMEM;
> > -}
> > -
> >   /*
> >    * Apply sealing.
> >    */
> > @@ -102,6 +74,24 @@ static int apply_mm_seal(unsigned long start, unsigned long end)
> >   	return 0;
> >   }
> >
> > +/* Does the [start, end) range contain any unmapped memory? */
> > +static bool range_contains_unmapped(struct mm_struct *mm,
> > +		unsigned long start, unsigned long end)
> > +{
> > +	struct vm_area_struct *vma;
> > +	unsigned long prev_end = start;
> > +	VMA_ITERATOR(vmi, current->mm, start);
> > +
> > +	for_each_vma_range(vmi, vma, end) {
> > +		if (vma->vm_start > prev_end)
> > +			return true;
> > +
> > +		prev_end = vma->vm_end;
> > +	}
> > +
> > +	return prev_end < end;
> > +}
> > +
>
> Probably better to not ... move the function in the same file? Then, we can
> se the actual diff of changes easily.

Sure, will respin with that.

>
> --
> Cheers,
>
> David / dhildenb
>