[PATCH v4 0/5] mseal cleanups

[PATCH v4 0/5] mseal cleanups

[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 retain existing logic regarding treatment of madvise()
discard operations for a sealed, read-only MAP_PRIVATE file-backed
mapping. This is something we likely need to revisit.

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.

No functional change is intended.


NOTE - this replaces the previous "mseal cleanups, fixup MAP_PRIVATE
file-backed case" series (linked below under v3), I renamed to 'mseal
cleaups' only as now we do not adjust MAP_PRIVATE behaviour.

v4:
* Propagated tags, thanks Jeff!
* Remove the change in semantics for MAP_PRIVATE, file-backed, mseal()'d
  mappings, as it turns out there is more complexity than expected
  here. Let's stick to a straight refactoring for now. Based on discussions
  with Jeff, Kees and David.
* Reinstated comment previously present in check_mm_seal() regarding
  expectations of input range in range_contains_unmapped() where this makes
  sense to locate, removing the redundant 'map is sealable' step, as per
  Jeff.

v3:
* Propagated more tags, thanks everyone!
* Updated 5/5 to assign curr_start in a smarter way as per Liam. Adjust
  code to more sensibly handle already-sealed case at the same time.
* Updated 4/5 to not move range_contains_unmapped() for better diff.
* Renamed can_modify_vma() to vma_is_sealed() and inverted the logic - this
  is far clearer than the nebulous 'can modify VMA'.
https://lore.kernel.org/all/cover.1752687069.git.lorenzo.stoakes@oracle.com/

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.
https://lore.kernel.org/all/cover.1752586090.git.lorenzo.stoakes@oracle.com/

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                     |  71 +++++++++++++-
 mm/mprotect.c                    |   2 +-
 mm/mremap.c                      |   2 +-
 mm/mseal.c                       | 160 +++++++------------------------
 mm/vma.c                         |   4 +-
 mm/vma.h                         |  27 +-----
 tools/testing/vma/vma_internal.h |   6 +-
 8 files changed, 119 insertions(+), 159 deletions(-)

--
2.50.1

[PATCH v4 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 8e3a4c5b78ff..ceaa780a703a 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 v4 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.

We retain the existing behaviour here regarding MAP_PRIVATE mappings of
file-backed mappings, which entails some complexity - while this, strictly
speaking - appears to violate mseal() semantics, it may interact badly with
users which expect to be able to madvise(MADV_DONTNEED) .text mappings for
instance.

We may revisit this at a later date.

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/madvise.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 mm/mseal.c   | 49 ------------------------------------
 mm/vma.h     |  7 ------
 3 files changed, 70 insertions(+), 57 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index bb80fc5ea08f..7f9af2dbd044 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,74 @@ 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;
+
+	/*
+	 * We explicitly permit all file-backed mappings, whether MAP_SHARED or
+	 * MAP_PRIVATE.
+	 *
+	 * The latter causes some complications. Because now, one can mmap()
+	 * read/write a MAP_PRIVATE mapping, write to it, then mprotect()
+	 * read-only, mseal() and a discard will be permitted.
+	 *
+	 * However, in order to avoid issues with potential use of madvise(...,
+	 * MADV_DONTNEED) of mseal()'d .text mappings we, for the time being,
+	 * permit this.
+	 */
+	if (!vma_is_anonymous(vma))
+		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 +1338,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 acdcc515c459..85db5e880fcc 100644
--- a/mm/vma.h
+++ b/mm/vma.h
@@ -577,8 +577,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)
@@ -586,11 +584,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)
--
2.50.1

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

[Lorenzo Stoakes]

Drop the wholly unnecessary set_vma_sealed() helper(), which is used only
once, and place VMA_ITERATOR() declarations in the correct place.

Retain vma_is_sealed(), and use it instead of the confusingly named
can_modify_vma(), so it's abundantly clear what's being tested, rather
then a nebulous sense of 'can the VMA be modified'.

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>
Acked-by: Jeff Xu <jeffxu@chromium.org>
---
 mm/madvise.c  |  2 +-
 mm/mprotect.c |  2 +-
 mm/mremap.c   |  2 +-
 mm/mseal.c    |  9 +--------
 mm/vma.c      |  4 ++--
 mm/vma.h      | 20 ++------------------
 6 files changed, 8 insertions(+), 31 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index 7f9af2dbd044..35ed4ab0d7c5 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1287,7 +1287,7 @@ 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))
+	if (!vma_is_sealed(vma))
 		return true;
 
 	/* For a sealed VMA, we only care about discard operations. */
diff --git a/mm/mprotect.c b/mm/mprotect.c
index 2ddd37b2f462..78bded7acf79 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -766,7 +766,7 @@ mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb,
 	unsigned long charged = 0;
 	int error;
 
-	if (!can_modify_vma(vma))
+	if (vma_is_sealed(vma))
 		return -EPERM;
 
 	if (newflags == oldflags) {
diff --git a/mm/mremap.c b/mm/mremap.c
index e15cf2e444c7..ac39845e9718 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -1651,7 +1651,7 @@ static int check_prep_vma(struct vma_remap_struct *vrm)
 		return -EFAULT;
 
 	/* If mseal()'d, mremap() is prohibited. */
-	if (!can_modify_vma(vma))
+	if (vma_is_sealed(vma))
 		return -EPERM;
 
 	/* Align to hugetlb page size, if required. */
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.c b/mm/vma.c
index fc502b741dcf..75fd2759964b 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -1351,7 +1351,7 @@ static int vms_gather_munmap_vmas(struct vma_munmap_struct *vms,
 		}
 
 		/* Don't bother splitting the VMA if we can't unmap it anyway */
-		if (!can_modify_vma(vms->vma)) {
+		if (vma_is_sealed(vms->vma)) {
 			error = -EPERM;
 			goto start_split_failed;
 		}
@@ -1371,7 +1371,7 @@ static int vms_gather_munmap_vmas(struct vma_munmap_struct *vms,
 	for_each_vma_range(*(vms->vmi), next, vms->end) {
 		long nrpages;
 
-		if (!can_modify_vma(next)) {
+		if (vma_is_sealed(next)) {
 			error = -EPERM;
 			goto modify_vma_failed;
 		}
diff --git a/mm/vma.h b/mm/vma.h
index 85db5e880fcc..b123a9cdedb0 100644
--- a/mm/vma.h
+++ b/mm/vma.h
@@ -559,31 +559,15 @@ 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.
- */
-static inline bool can_modify_vma(struct vm_area_struct *vma)
-{
-	if (unlikely(vma_is_sealed(vma)))
-		return false;
-
-	return true;
-}
-
 #else
-
-static inline bool can_modify_vma(struct vm_area_struct *vma)
+static inline bool vma_is_sealed(struct vm_area_struct *vma)
 {
-	return true;
+	return false;
 }
-
 #endif
 
 #if defined(CONFIG_STACK_GROWSUP)
-- 
2.50.1

[PATCH v4 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 | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/mm/mseal.c b/mm/mseal.c
index adbcc65e9660..1059322add34 100644
--- a/mm/mseal.c
+++ b/mm/mseal.c
@@ -38,31 +38,28 @@ static int mseal_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
 }
 
 /*
- * 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.
+ * Does the [start, end) range contain any unmapped memory?
+ *
+ * We ensure that:
+ * - start is part of a valid VMA.
+ * - end is part of a valid VMA.
+ * - no gap (unallocated memory) exists between start and end.
  */
-static int check_mm_seal(unsigned long start, unsigned long end)
+static bool range_contains_unmapped(struct mm_struct *mm,
+		unsigned long start, unsigned long end)
 {
 	struct vm_area_struct *vma;
-	unsigned long nstart = start;
+	unsigned long prev_end = 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;
+		if (vma->vm_start > prev_end)
+			return true;
 
-		nstart = vma->vm_end;
+		prev_end = vma->vm_end;
 	}
 
-	return -ENOMEM;
+	return prev_end < end;
 }
 
 /*
@@ -184,14 +181,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 v4 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 eliminate the need for invoking vma_iter_end() on each loop by directly
determining if the VMA was merged - the only thing we need concern
ourselves with is whether the start/end of the (gapless) range are offset
into VMAs.

This refactoring also avoids 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>
Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>
Acked-by: David Hildenbrand <david@redhat.com>
Acked-by: Jeff Xu <jeffxu@chromium.org>
---
 mm/mseal.c | 67 ++++++++++++++++--------------------------------------
 1 file changed, 20 insertions(+), 47 deletions(-)

diff --git a/mm/mseal.c b/mm/mseal.c
index 1059322add34..3df9581ec828 100644
--- a/mm/mseal.c
+++ b/mm/mseal.c
@@ -15,28 +15,6 @@
 #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;
-}
-
 /*
  * Does the [start, end) range contain any unmapped memory?
  *
@@ -62,38 +40,33 @@ static bool range_contains_unmapped(struct mm_struct *mm,
 	return prev_end < end;
 }
 
-/*
- * 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);
+	unsigned long curr_start = 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_end = MIN(vma->vm_end, end);
+
+		if (!(vma->vm_flags & VM_SEALED)) {
+			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;
+		curr_start = curr_end;
 	}
 
 	return 0;
@@ -192,10 +165,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 v4 2/5] mm/mseal: update madvise() logic

[Jeff Xu]

Hi Lorenzo

On Fri, Jul 25, 2025 at 1:30 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> 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.
>
> We retain the existing behaviour here regarding MAP_PRIVATE mappings of
> file-backed mappings, which entails some complexity - while this, strictly
> speaking - appears to violate mseal() semantics, it may interact badly with
> users which expect to be able to madvise(MADV_DONTNEED) .text mappings for
> instance.
>
> We may revisit this at a later date.
>
> 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/madvise.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  mm/mseal.c   | 49 ------------------------------------
>  mm/vma.h     |  7 ------
>  3 files changed, 70 insertions(+), 57 deletions(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index bb80fc5ea08f..7f9af2dbd044 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,74 @@ 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;
> +
> +       /*
> +        * We explicitly permit all file-backed mappings, whether MAP_SHARED or
> +        * MAP_PRIVATE.
> +        *
> +        * The latter causes some complications. Because now, one can mmap()
> +        * read/write a MAP_PRIVATE mapping, write to it, then mprotect()
> +        * read-only, mseal() and a discard will be permitted.
> +        *
> +        * However, in order to avoid issues with potential use of madvise(...,
> +        * MADV_DONTNEED) of mseal()'d .text mappings we, for the time being,
> +        * permit this.
> +        */
> +       if (!vma_is_anonymous(vma))
> +               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 +1338,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;

In this patch, the check for anonymous mapping are replaced with:

 if (!vma_is_anonymous(vma))
      return true;

vma_is_anonymous()  is implemented as following:
static inline bool vma_is_anonymous(struct vm_area_struct *vma)
{
   return !vma->vm_ops;
}

I'm curious to know if those two checks have the exact same scope.

The original intention is only file-backed mapping can allow
destructive madvise while sealed. I want to make sure that we don't
accidentally increase the scope.

Thanks and regards,
-Jeff



> -
> -       /*
> -        * 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 acdcc515c459..85db5e880fcc 100644
> --- a/mm/vma.h
> +++ b/mm/vma.h
> @@ -577,8 +577,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)
> @@ -586,11 +584,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)
> --
> 2.50.1

Re: [PATCH v4 4/5] mm/mseal: simplify and rename VMA gap check

[Jeff Xu]

Hi Lorenzo,

On Fri, Jul 25, 2025 at 1:30 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> 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().
>
Thanks for keeping the comments.

In the prior version of this patch, I requested that we keep the
check_mm_seal()  and its comments. And this version keeps the comments
but removes the check_mm_seal() name.

As I said, check_mm_seal() with its comments is a contract for
entry-check for mseal().  My understanding is that you are going to
move range_contains_unmapped() to vma.c. When that happens, mseal()
will lose this entry-check contract.

Contact is a great way to hide implementation details. Could you
please keep check_mm_seal() in mseal.c and create
range_contains_unmapped() in vma.c. Then you can refactor as needed.

Thanks and regards,
-Jeff





> 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 | 39 ++++++++++++++++-----------------------
>  1 file changed, 16 insertions(+), 23 deletions(-)
>
> diff --git a/mm/mseal.c b/mm/mseal.c
> index adbcc65e9660..1059322add34 100644
> --- a/mm/mseal.c
> +++ b/mm/mseal.c
> @@ -38,31 +38,28 @@ static int mseal_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  }
>
>  /*
> - * 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.
> + * Does the [start, end) range contain any unmapped memory?
> + *
> + * We ensure that:
> + * - start is part of a valid VMA.
> + * - end is part of a valid VMA.
> + * - no gap (unallocated memory) exists between start and end.
>   */
> -static int check_mm_seal(unsigned long start, unsigned long end)
> +static bool range_contains_unmapped(struct mm_struct *mm,
> +               unsigned long start, unsigned long end)
>  {
>         struct vm_area_struct *vma;
> -       unsigned long nstart = start;
> +       unsigned long prev_end = 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;
> +               if (vma->vm_start > prev_end)
> +                       return true;
>
> -               nstart = vma->vm_end;
> +               prev_end = vma->vm_end;
>         }
>
> -       return -ENOMEM;
> +       return prev_end < end;
>  }
>
>  /*
> @@ -184,14 +181,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
>

Re: [PATCH v4 4/5] mm/mseal: simplify and rename VMA gap check

[Lorenzo Stoakes]

On Fri, Jul 25, 2025 at 10:30:08AM -0700, Jeff Xu wrote:
> Hi Lorenzo,
>
> On Fri, Jul 25, 2025 at 1:30 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> 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().
> >
> Thanks for keeping the comments.

You're welcome.

>
> In the prior version of this patch, I requested that we keep the
> check_mm_seal()  and its comments. And this version keeps the comments
> but removes the check_mm_seal() name.

I didn't catch that being your request.

>
> As I said, check_mm_seal() with its comments is a contract for
> entry-check for mseal().  My understanding is that you are going to
> move range_contains_unmapped() to vma.c. When that happens, mseal()
> will lose this entry-check contract.

This is just bizarre.

Code doesn't stop working if you put it in another function.

And you're now reviewing me for stuff I haven't done? :P

>
> Contact is a great way to hide implementation details. Could you
> please keep check_mm_seal() in mseal.c and create
> range_contains_unmapped() in vma.c. Then you can refactor as needed.

Wait what?

OK maybe now I see what you mean, you want a function that just wraps
range_contains_unmapped() with a comment explaining the 'contract'.

range_contains_unmapped() enforces your required contract and the comments
make it extremely explicit, so this is not a reasonable request, sorry.

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

[Lorenzo Stoakes]

On Fri, Jul 25, 2025 at 10:28:57AM -0700, Jeff Xu wrote:

> > -static bool is_ro_anon(struct vm_area_struct *vma)
> > -{
> > -       /* check anonymous mapping. */
> > -       if (vma->vm_file || vma->vm_flags & VM_SHARED)
> > -               return false;
>
> In this patch, the check for anonymous mapping are replaced with:
>
>  if (!vma_is_anonymous(vma))
>       return true;
>
> vma_is_anonymous()  is implemented as following:
> static inline bool vma_is_anonymous(struct vm_area_struct *vma)
> {
>    return !vma->vm_ops;
> }
>
> I'm curious to know if those two checks have the exact same scope.
>
> The original intention is only file-backed mapping can allow
> destructive madvise while sealed. I want to make sure that we don't
> accidentally increase the scope.
>
> Thanks and regards,
> -Jeff

Thanks, that's a good question.

So for a function to be mmap()'d and file-backed, vm_ops _must_ be
supplied.

You can see this in the fault-handler:

do_pte_mising()
-> do_fault()
if anon -> fault anon otherwise fault file-backed

So if this were not the case, you'd have file-backed mappings going into
the the anonymous fault handler logic.

This covers off MAP_PRIVATE mappings of file-backed mappings too, as this
is handled in do_fault() by:

	} else if (!(vmf->flags & FAULT_FLAG_WRITE))
		ret = do_read_fault(vmf);
	else if (!(vma->vm_flags & VM_SHARED))
		ret = do_cow_fault(vmf);

That does the CoW fault handling.

So the vma_is_anonymous_check() here should have the same semantics.

Cheers, Lorenzo

Re: [PATCH v4 4/5] mm/mseal: simplify and rename VMA gap check

[Jeff Xu]

Hi Lorenzo

On Fri, Jul 25, 2025 at 10:43 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Fri, Jul 25, 2025 at 10:30:08AM -0700, Jeff Xu wrote:
> > Hi Lorenzo,
> >
> > On Fri, Jul 25, 2025 at 1:30 AM Lorenzo Stoakes
> > <lorenzo.stoakes@oracle.com> 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().
> > >
> > Thanks for keeping the comments.
>
> You're welcome.
>
> >
> > In the prior version of this patch, I requested that we keep the
> > check_mm_seal()  and its comments. And this version keeps the comments
> > but removes the check_mm_seal() name.
>
> I didn't catch that being your request.
>
> >
> > As I said, check_mm_seal() with its comments is a contract for
> > entry-check for mseal().  My understanding is that you are going to
> > move range_contains_unmapped() to vma.c. When that happens, mseal()
> > will lose this entry-check contract.
>
> This is just bizarre.
>
> Code doesn't stop working if you put it in another function.
>
> And you're now reviewing me for stuff I haven't done? :P
>
> >
> > Contact is a great way to hide implementation details. Could you
> > please keep check_mm_seal() in mseal.c and create
> > range_contains_unmapped() in vma.c. Then you can refactor as needed.
>
> Wait what?
>
> OK maybe now I see what you mean, you want a function that just wraps
> range_contains_unmapped() with a comment explaining the 'contract'.
>
Yes. You can view it that way from an implementation point of view.

Contract mainly serves as a way to help design and abstract the code.

> range_contains_unmapped() enforces your required contract and the comments
> make it extremely explicit, so this is not a reasonable request, sorry.

Technically, this contract belongs to mseal, but if you have strong
opinions on this, that's fine, as long as range_contains_unmapped()
doesn't accidentally remove those comments in the future, which I'm
sure you won't.

Acked-by: Jeff Xu <jeffxu@chromium.org>

Thanks and regards,
-Jeff

Re: [PATCH v4 4/5] mm/mseal: simplify and rename VMA gap check

[David Hildenbrand]

On 25.07.25 19:43, Lorenzo Stoakes wrote:
> On Fri, Jul 25, 2025 at 10:30:08AM -0700, Jeff Xu wrote:
>> Hi Lorenzo,
>>
>> On Fri, Jul 25, 2025 at 1:30 AM Lorenzo Stoakes
>> <lorenzo.stoakes@oracle.com> 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().
>>>
>> Thanks for keeping the comments.
> 
> You're welcome.
> 
>>
>> In the prior version of this patch, I requested that we keep the
>> check_mm_seal()  and its comments. And this version keeps the comments
>> but removes the check_mm_seal() name.
> 
> I didn't catch that being your request.
> 
>>
>> As I said, check_mm_seal() with its comments is a contract for
>> entry-check for mseal().  My understanding is that you are going to
>> move range_contains_unmapped() to vma.c. When that happens, mseal()
>> will lose this entry-check contract.
> 
> This is just bizarre.
> 
> Code doesn't stop working if you put it in another function.
> 
> And you're now reviewing me for stuff I haven't done? :P
> 
>>
>> Contact is a great way to hide implementation details. Could you
>> please keep check_mm_seal() in mseal.c and create
>> range_contains_unmapped() in vma.c. Then you can refactor as needed.
> 
> Wait what?

do_mseal() calls range_contains_unmapped(), so I don't see the problem.

We could add a comment above the range_contains_unmapped(), call stating 
*why* we do that, which is much more relevant than some check_XXX function.

/*
  * mseal() is documented to reject ranges that contain unmapped ranges
  * (VMA holes): we can only seal VMAs, so nothing would stop mmap() etc.
  * from succeeding on these unmapped ranged later, and we would not
  * actually be sealing the requested range.
  */

Something like that.

-- 
Cheers,

David / dhildenb

Re: [PATCH v4 4/5] mm/mseal: simplify and rename VMA gap check

[Lorenzo Stoakes]

On Fri, Jul 25, 2025 at 11:09:13AM -0700, Jeff Xu wrote:
> Hi Lorenzo
>
> On Fri, Jul 25, 2025 at 10:43 AM Lorenzo Stoakes
> > OK maybe now I see what you mean, you want a function that just wraps
> > range_contains_unmapped() with a comment explaining the 'contract'.
> >
> Yes. You can view it that way from an implementation point of view.
>
> Contract mainly serves as a way to help design and abstract the code.

Right sure, I sort of good the idea, I just think it's a bit OTT for this
check whose contract is already clearly stated in code.

>
> > range_contains_unmapped() enforces your required contract and the comments
> > make it extremely explicit, so this is not a reasonable request, sorry.
>
> Technically, this contract belongs to mseal, but if you have strong
> opinions on this, that's fine, as long as range_contains_unmapped()
> doesn't accidentally remove those comments in the future, which I'm
> sure you won't.

We won't change the semantics without a specific patch suggesting to do so,
which you and Kees will be cc'd on!

I care very much about making sure we get the mechanics of mseal() right,
so I'm not going to allow such changes unless we sensibly reach agreement
that it's the right way forward (i.e. the same obviously as if we chose to
_change_ a contract formulation using your approach).


>
> Acked-by: Jeff Xu <jeffxu@chromium.org>

Thanks, appreciated!

>
> Thanks and regards,
> -Jeff

Cheers, Lorenzo

Re: [PATCH v4 4/5] mm/mseal: simplify and rename VMA gap check

[Lorenzo Stoakes]

On Fri, Jul 25, 2025 at 08:10:11PM +0200, David Hildenbrand wrote:
> On 25.07.25 19:43, Lorenzo Stoakes wrote:
> > >
> > > Contact is a great way to hide implementation details. Could you
> > > please keep check_mm_seal() in mseal.c and create
> > > range_contains_unmapped() in vma.c. Then you can refactor as needed.
> >
> > Wait what?
>
> do_mseal() calls range_contains_unmapped(), so I don't see the problem.

Thanks.

>
> We could add a comment above the range_contains_unmapped(), call stating
> *why* we do that, which is much more relevant than some check_XXX function.
>
> /*
>  * mseal() is documented to reject ranges that contain unmapped ranges
>  * (VMA holes): we can only seal VMAs, so nothing would stop mmap() etc.
>  * from succeeding on these unmapped ranged later, and we would not
>  * actually be sealing the requested range.
>  */
>
> Something like that.

Actually this is useful, as it explains to me why we disallow gaps (which I
found silly).

Though I'm not sure I still agree with this (under what circumstances
exactly you'd map within an mseal()'d range afterwards and then assume that
mapping is sealed, I don't know).

Anyway that's sort of besides the point.

I'll send a fix-patch to include this.

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

Cheers, Lorenzo

Re: [PATCH v4 4/5] mm/mseal: simplify and rename VMA gap check

[Jeff Xu]

On Fri, Jul 25, 2025 at 11:10 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 25.07.25 19:43, Lorenzo Stoakes wrote:
> > On Fri, Jul 25, 2025 at 10:30:08AM -0700, Jeff Xu wrote:
> >> Hi Lorenzo,
> >>
> >> On Fri, Jul 25, 2025 at 1:30 AM Lorenzo Stoakes
> >> <lorenzo.stoakes@oracle.com> 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().
> >>>
> >> Thanks for keeping the comments.
> >
> > You're welcome.
> >
> >>
> >> In the prior version of this patch, I requested that we keep the
> >> check_mm_seal()  and its comments. And this version keeps the comments
> >> but removes the check_mm_seal() name.
> >
> > I didn't catch that being your request.
> >
> >>
> >> As I said, check_mm_seal() with its comments is a contract for
> >> entry-check for mseal().  My understanding is that you are going to
> >> move range_contains_unmapped() to vma.c. When that happens, mseal()
> >> will lose this entry-check contract.
> >
> > This is just bizarre.
> >
> > Code doesn't stop working if you put it in another function.
> >
> > And you're now reviewing me for stuff I haven't done? :P
> >
> >>
> >> Contact is a great way to hide implementation details. Could you
> >> please keep check_mm_seal() in mseal.c and create
> >> range_contains_unmapped() in vma.c. Then you can refactor as needed.
> >
> > Wait what?
>
> do_mseal() calls range_contains_unmapped(), so I don't see the problem.
>
> We could add a comment above the range_contains_unmapped(), call stating
> *why* we do that, which is much more relevant than some check_XXX function.
>
> /*
>   * mseal() is documented to reject ranges that contain unmapped ranges
>   * (VMA holes): we can only seal VMAs, so nothing would stop mmap() etc.
>   * from succeeding on these unmapped ranged later, and we would not
>   * actually be sealing the requested range.
>   */
>
Adding a reason explaining the reason is way more helpful than just
stating what it's doing. Thanks!

a nit: I would use:

> /*
>   * mseal() is documented to reject ranges that contain unmapped ranges
>   * (VMA holes in the middle or both ends): we can only seal VMAs, so nothing
>   * would stop mmap() etc. from succeeding on these unmapped ranged later, and
>   * we would not actually be sealing the requested range.
>   */

To make it clear to the reader, because VMA holes might lead people to
think they're only in the middle.

Thanks and regards,
-Jeff





> Something like that.
>
> --
> Cheers,
>
> David / dhildenb
>

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

[Jeff Xu]

Hi Lorenzo,


On Fri, Jul 25, 2025 at 10:54 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Fri, Jul 25, 2025 at 10:28:57AM -0700, Jeff Xu wrote:
>
> > > -static bool is_ro_anon(struct vm_area_struct *vma)
> > > -{
> > > -       /* check anonymous mapping. */
> > > -       if (vma->vm_file || vma->vm_flags & VM_SHARED)
> > > -               return false;
> >
> > In this patch, the check for anonymous mapping are replaced with:
> >
> >  if (!vma_is_anonymous(vma))
> >       return true;
> >
> > vma_is_anonymous()  is implemented as following:
> > static inline bool vma_is_anonymous(struct vm_area_struct *vma)
> > {
> >    return !vma->vm_ops;
> > }
> >
> > I'm curious to know if those two checks have the exact same scope.
> >
> > The original intention is only file-backed mapping can allow
> > destructive madvise while sealed. I want to make sure that we don't
> > accidentally increase the scope.
> >
> > Thanks and regards,
> > -Jeff
>
> Thanks, that's a good question.
>
> So for a function to be mmap()'d and file-backed, vm_ops _must_ be
> supplied.
>
This says that all file-backed mappings must have vm_ops set, but what
about the reverse? Are mappings with vm_ops always file-backed?

> You can see this in the fault-handler:
>
> do_pte_mising()
> -> do_fault()
> if anon -> fault anon otherwise fault file-backed
>
> So if this were not the case, you'd have file-backed mappings going into
> the the anonymous fault handler logic.
>
> This covers off MAP_PRIVATE mappings of file-backed mappings too, as this
> is handled in do_fault() by:
>
>         } else if (!(vmf->flags & FAULT_FLAG_WRITE))
>                 ret = do_read_fault(vmf);
>         else if (!(vma->vm_flags & VM_SHARED))
>                 ret = do_cow_fault(vmf);
>
> That does the CoW fault handling.
>
> So the vma_is_anonymous_check() here should have the same semantics.
>
Just to be extra careful, does the reverse hold true as well?

In anycase, if you are confident about this, please do state this
change in the commit description that vma->vm_file and VM_SHARED flag
check is replaced by vma_is_anonymous_check(), which is expected to be
a non-functional change.

Thanks and regards,
-Jeff

> Cheers, Lorenzo

Re: [PATCH v4 4/5] mm/mseal: simplify and rename VMA gap check

[Lorenzo Stoakes]

Hi Andrew,

Can you apply the attached trivial fix-patch which adds a clarifying comment to
mm/mseal.c.

Thanks, Lorenzo


----8<----
From bf8211317183353b3652baac1af1d35555733d2b Mon Sep 17 00:00:00 2001
From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Date: Fri, 25 Jul 2025 19:23:50 +0100
Subject: [PATCH] mm/mseal: add comment explaining why we disallow gaps on
 mseal()

This explains the semantics clearly, the 'why' of the situation.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 mm/mseal.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/mm/mseal.c b/mm/mseal.c
index 1059322add34..d140f569c4c3 100644
--- a/mm/mseal.c
+++ b/mm/mseal.c
@@ -37,6 +37,18 @@ static int mseal_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
 	return ret;
 }

+/*
+ * mseal() disallows an input range which contain unmapped ranges (VMA holes).
+ *
+ * It disallows unmapped regions from start to end whether they exist at the
+ * start, in the middle, or at the end of the range, or any combination thereof.
+ *
+ * This is because after sealng a range, there's nothing to stop memory mapping
+ * of ranges in the remaining gaps later, meaning that the user might then
+ * wrongly consider the entirety of the mseal()'d range to be sealed when it
+ * in fact isn't.
+ */
+
 /*
  * Does the [start, end) range contain any unmapped memory?
  *
--
2.50.1

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

[Lorenzo Stoakes]

On Fri, Jul 25, 2025 at 11:41:15AM -0700, Jeff Xu wrote:
> Hi Lorenzo,
>
>
> On Fri, Jul 25, 2025 at 10:54 AM Lorenzo Stoakes
> > Thanks, that's a good question.
> >
> > So for a function to be mmap()'d and file-backed, vm_ops _must_ be
> > supplied.
> >
> This says that all file-backed mappings must have vm_ops set, but what
> about the reverse? Are mappings with vm_ops always file-backed?

Yes? Otherwise they'd get treated as anonymous?

We call this vma_is_anonymous() for a reason ;)

>
> > You can see this in the fault-handler:
> >
> > do_pte_mising()
> > -> do_fault()
> > if anon -> fault anon otherwise fault file-backed
> >
> > So if this were not the case, you'd have file-backed mappings going into
> > the the anonymous fault handler logic.
> >
> > This covers off MAP_PRIVATE mappings of file-backed mappings too, as this
> > is handled in do_fault() by:
> >
> >         } else if (!(vmf->flags & FAULT_FLAG_WRITE))
> >                 ret = do_read_fault(vmf);
> >         else if (!(vma->vm_flags & VM_SHARED))
> >                 ret = do_cow_fault(vmf);
> >
> > That does the CoW fault handling.
> >
> > So the vma_is_anonymous_check() here should have the same semantics.
> >
> Just to be extra careful, does the reverse hold true as well?
>
> In anycase, if you are confident about this, please do state this
> change in the commit description that vma->vm_file and VM_SHARED flag
> check is replaced by vma_is_anonymous_check(), which is expected to be
> a non-functional change.

It's functionally equivalent and can be seen in the diff so I don't think
this is necessary.

Re: [PATCH v4 4/5] mm/mseal: simplify and rename VMA gap check

[Pedro Falcato]

On Fri, Jul 25, 2025 at 11:09:13AM -0700, Jeff Xu wrote:
> Hi Lorenzo
> 
> On Fri, Jul 25, 2025 at 10:43 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > On Fri, Jul 25, 2025 at 10:30:08AM -0700, Jeff Xu wrote:
> > > Hi Lorenzo,
> > >
> > > On Fri, Jul 25, 2025 at 1:30 AM Lorenzo Stoakes
> > > <lorenzo.stoakes@oracle.com> 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().
> > > >
> > > Thanks for keeping the comments.
> >
> > You're welcome.
> >
> > >
> > > In the prior version of this patch, I requested that we keep the
> > > check_mm_seal()  and its comments. And this version keeps the comments
> > > but removes the check_mm_seal() name.
> >
> > I didn't catch that being your request.
> >
> > >
> > > As I said, check_mm_seal() with its comments is a contract for
> > > entry-check for mseal().  My understanding is that you are going to
> > > move range_contains_unmapped() to vma.c. When that happens, mseal()
> > > will lose this entry-check contract.
> >
> > This is just bizarre.
> >
> > Code doesn't stop working if you put it in another function.
> >
> > And you're now reviewing me for stuff I haven't done? :P
> >
> > >
> > > Contact is a great way to hide implementation details. Could you
> > > please keep check_mm_seal() in mseal.c and create
> > > range_contains_unmapped() in vma.c. Then you can refactor as needed.
> >
> > Wait what?
> >
> > OK maybe now I see what you mean, you want a function that just wraps
> > range_contains_unmapped() with a comment explaining the 'contract'.
> >
> Yes. You can view it that way from an implementation point of view.
> 
> Contract mainly serves as a way to help design and abstract the code.
>

What code? This is an extremely simple file. We don't need deep design
and abstractions here.

> > range_contains_unmapped() enforces your required contract and the comments
> > make it extremely explicit, so this is not a reasonable request, sorry.
> 
> Technically, this contract belongs to mseal, but if you have strong
> opinions on this, that's fine, as long as range_contains_unmapped()
> doesn't accidentally remove those comments in the future, which I'm
> sure you won't.
> 

As far as I'm concerned, mseal() has little to no contract - we still don't have
a solid definition of what mseal() is supposed to do, things are still fluctuating,
and there's no man page (and no one is going to look into random kernel comments
for this).

FTR: I entirely plan on axing this function in the near future (or will try to).
There's no valid reason for this to exist, and it's causing extra burden on the
implementation - besides serving as a poor example for future mmap-ish syscalls.

-- 
Pedro

Re: [PATCH v4 4/5] mm/mseal: simplify and rename VMA gap check

[Pedro Falcato]

On Fri, Jul 25, 2025 at 09:29:44AM +0100, 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>

Reviewed-by: Pedro Falcato <pfalcato@suse.de>

-- 
Pedro