[PATCH 0/7] mm: Optimize mseal checks

[PATCH 0/7] mm: Optimize mseal checks

[Pedro Falcato]

Optimize mseal checks by removing the separate can_modify_mm() step, and
just doing checks on the individual vmas, when various operations are
themselves iterating through the tree. This provides a nice speedup.

While I was at it, I found that is_madv_discard() was completely bogus.

Note that my series ignores arch_unmap(), which seems to generally be what we're trending towards[2]. It should
be applied on top of any powerpc vdso ->close patch to avoid regressions on the PPC architecture. No other
architecture seems to use arch_unmap.

Note2: This series does not pass all mseal_tests on my end (test_seal_mremap_move_dontunmap_anyaddr fails twice). But the
top of Linus's tree does not pass these for me either (neither does my Arch Linux 6.10.2 kernel),
for some reason (mremap regression?).

will-it-scale mmap1_process[1] -t 1 results:

commit 3450fe2b574b4345e4296ccae395149e1a357fee:

min:277605 max:277605 total:277605
min:281784 max:281784 total:281784
min:277238 max:277238 total:277238
min:281761 max:281761 total:281761
min:274279 max:274279 total:274279
min:254854 max:254854 total:254854
measurement
min:269143 max:269143 total:269143
min:270454 max:270454 total:270454
min:243523 max:243523 total:243523
min:251148 max:251148 total:251148
min:209669 max:209669 total:209669
min:190426 max:190426 total:190426
min:231219 max:231219 total:231219
min:275364 max:275364 total:275364
min:266540 max:266540 total:266540
min:242572 max:242572 total:242572
min:284469 max:284469 total:284469
min:278882 max:278882 total:278882
min:283269 max:283269 total:283269
min:281204 max:281204 total:281204

After this patch set:

min:280580 max:280580 total:280580
min:290514 max:290514 total:290514
min:291006 max:291006 total:291006
min:290352 max:290352 total:290352
min:294582 max:294582 total:294582
min:293075 max:293075 total:293075
measurement
min:295613 max:295613 total:295613
min:294070 max:294070 total:294070
min:293193 max:293193 total:293193
min:291631 max:291631 total:291631
min:295278 max:295278 total:295278
min:293782 max:293782 total:293782
min:290361 max:290361 total:290361
min:294517 max:294517 total:294517
min:293750 max:293750 total:293750
min:293572 max:293572 total:293572
min:295239 max:295239 total:295239
min:292932 max:292932 total:292932
min:293319 max:293319 total:293319
min:294954 max:294954 total:294954

This was a Completely Unscientific test but seems to show there were around 5-10% gains on ops per second.

[1]: mmap1_process does mmap and munmap in a loop. I didn't bother testing multithreading cases.
[2]: https://lore.kernel.org/all/87o766iehy.fsf@mail.lhotse/
Link: https://lore.kernel.org/all/202408041602.caa0372-oliver.sang@intel.com/

Pedro Falcato (7):
  mm: Move can_modify_vma to mm/internal.h
  mm/munmap: Replace can_modify_mm with can_modify_vma
  mm/mprotect: Replace can_modify_mm with can_modify_vma
  mm/mremap: Replace can_modify_mm with can_modify_vma
  mseal: Fix is_madv_discard()
  mseal: Replace can_modify_mm_madv with a vma variant
  mm: Remove can_modify_mm()

 mm/internal.h | 30 ++++++++++++++++------
 mm/madvise.c  | 13 +++-------
 mm/mmap.c     | 36 ++++++++++-----------------
 mm/mprotect.c | 12 +++------
 mm/mremap.c   | 33 ++++++------------------
 mm/mseal.c    | 69 +++++++++++----------------------------------------
 6 files changed, 63 insertions(+), 130 deletions(-)

-- 
2.46.0

[PATCH 1/7] mm: Move can_modify_vma to mm/internal.h

[Pedro Falcato]

Move can_modify_vma to internal.h so it can be inlined properly (with
the intent to remove can_modify_mm callsites).

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 mm/internal.h | 24 ++++++++++++++++++++++++
 mm/mseal.c    | 17 -----------------
 2 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index b4d86436565..09ea930540f 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -1497,6 +1497,24 @@ static inline int can_do_mseal(unsigned long flags)
 	return 0;
 }
 
+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;
+}
+
+
 bool can_modify_mm(struct mm_struct *mm, unsigned long start,
 		unsigned long end);
 bool can_modify_mm_madv(struct mm_struct *mm, unsigned long start,
@@ -1518,6 +1536,12 @@ static inline bool can_modify_mm_madv(struct mm_struct *mm, unsigned long start,
 {
 	return true;
 }
+
+static inline bool can_modify_vma(struct vm_area_struct *vma)
+{
+	return true;
+}
+
 #endif
 
 #ifdef CONFIG_SHRINKER_DEBUG
diff --git a/mm/mseal.c b/mm/mseal.c
index bf783bba8ed..4591ae8d29c 100644
--- a/mm/mseal.c
+++ b/mm/mseal.c
@@ -16,28 +16,11 @@
 #include <linux/sched.h>
 #include "internal.h"
 
-static inline bool vma_is_sealed(struct vm_area_struct *vma)
-{
-	return (vma->vm_flags & VM_SEALED);
-}
-
 static inline void set_vma_sealed(struct vm_area_struct *vma)
 {
 	vm_flags_set(vma, VM_SEALED);
 }
 
-/*
- * check if a vma is sealed for modification.
- * return true, if modification is allowed.
- */
-static bool can_modify_vma(struct vm_area_struct *vma)
-{
-	if (unlikely(vma_is_sealed(vma)))
-		return false;
-
-	return true;
-}
-
 static bool is_madv_discard(int behavior)
 {
 	return	behavior &
-- 
2.46.0

[PATCH 2/7] mm/munmap: Replace can_modify_mm with can_modify_vma

[Pedro Falcato]

We were doing an extra mmap tree traversal just to check if the entire
range is modifiable. This can be done when we iterate through the VMAs
instead.

Note that this removes the arch_unmap() callsites and therefore isn't
quite ready for Proper(tm) upstreaming.

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 mm/mmap.c | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index d0dfc85b209..b88666f618b 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2660,6 +2660,12 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
 		if (end < vma->vm_end && mm->map_count >= sysctl_max_map_count)
 			goto map_count_exceeded;
 
+		/* Don't bother splitting the VMA if we can't unmap it anyway */
+		if (!can_modify_vma(vma)) {
+			error = -EPERM;
+			goto start_split_failed;
+		}
+
 		error = __split_vma(vmi, vma, start, 1);
 		if (error)
 			goto start_split_failed;
@@ -2671,6 +2677,11 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
 	 */
 	next = vma;
 	do {
+		if (!can_modify_vma(vma)) {
+			error = -EPERM;
+			goto modify_vma_failed;
+		}
+
 		/* Does it split the end? */
 		if (next->vm_end > end) {
 			error = __split_vma(vmi, next, end, 0);
@@ -2763,6 +2774,7 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
 	__mt_destroy(&mt_detach);
 	return 0;
 
+modify_vma_failed:
 clear_tree_failed:
 userfaultfd_error:
 munmap_gather_failed:
@@ -2808,17 +2820,6 @@ int do_vmi_munmap(struct vma_iterator *vmi, struct mm_struct *mm,
 	if (end == start)
 		return -EINVAL;
 
-	/*
-	 * Check if memory is sealed before arch_unmap.
-	 * Prevent unmapping a sealed VMA.
-	 * can_modify_mm assumes we have acquired the lock on MM.
-	 */
-	if (unlikely(!can_modify_mm(mm, start, end)))
-		return -EPERM;
-
-	 /* arch_unmap() might do unmaps itself.  */
-	arch_unmap(mm, start, end);
-
 	/* Find the first overlapping VMA */
 	vma = vma_find(vmi, end);
 	if (!vma) {
@@ -3229,18 +3230,7 @@ int do_vma_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
 		unsigned long start, unsigned long end, struct list_head *uf,
 		bool unlock)
 {
-	struct mm_struct *mm = vma->vm_mm;
-
-	/*
-	 * Check if memory is sealed before arch_unmap.
-	 * Prevent unmapping a sealed VMA.
-	 * can_modify_mm assumes we have acquired the lock on MM.
-	 */
-	if (unlikely(!can_modify_mm(mm, start, end)))
-		return -EPERM;
-
-	arch_unmap(mm, start, end);
-	return do_vmi_align_munmap(vmi, vma, mm, start, end, uf, unlock);
+	return do_vmi_align_munmap(vmi, vma, vma->vm_mm, start, end, uf, unlock);
 }
 
 /*
-- 
2.46.0

[PATCH 3/7] mm/mprotect: Replace can_modify_mm with can_modify_vma

[Pedro Falcato]

Avoid taking an extra trip down the mmap tree by checking the vmas
directly. mprotect (per POSIX) tolerates partial failure.

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 mm/mprotect.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/mm/mprotect.c b/mm/mprotect.c
index 222ab434da5..b1980ea1cc3 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -589,6 +589,9 @@ mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb,
 	unsigned long charged = 0;
 	int error;
 
+	if (!can_modify_vma(vma))
+		return -EPERM;
+
 	if (newflags == oldflags) {
 		*pprev = vma;
 		return 0;
@@ -747,15 +750,6 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
 		}
 	}
 
-	/*
-	 * checking if memory is sealed.
-	 * can_modify_mm assumes we have acquired the lock on MM.
-	 */
-	if (unlikely(!can_modify_mm(current->mm, start, end))) {
-		error = -EPERM;
-		goto out;
-	}
-
 	prev = vma_prev(&vmi);
 	if (start > vma->vm_start)
 		prev = vma;
-- 
2.46.0

[PATCH 4/7] mm/mremap: Replace can_modify_mm with can_modify_vma

[Pedro Falcato]

Delegate all can_modify checks to the proper places. Unmap checks are
done in do_unmap (et al).

This patch allows for mremap partial failure in certain cases (for
instance, when destination VMAs aren't sealed, but the source VMA is).
It shouldn't be too troublesome, as you'd need to go out of your way to
do illegal operations on a VMA.

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 mm/mremap.c | 33 +++++++--------------------------
 1 file changed, 7 insertions(+), 26 deletions(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index e7ae140fc64..8af877d7bb0 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -676,6 +676,9 @@ static unsigned long move_vma(struct vm_area_struct *vma,
 	if (unlikely(flags & MREMAP_DONTUNMAP))
 		to_account = new_len;
 
+	if (!can_modify_vma(vma))
+		return -EPERM;
+
 	if (vma->vm_ops && vma->vm_ops->may_split) {
 		if (vma->vm_start != old_addr)
 			err = vma->vm_ops->may_split(vma, old_addr);
@@ -821,6 +824,10 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
 	if (!vma)
 		return ERR_PTR(-EFAULT);
 
+	/* Don't allow vma expansion when it has already been sealed */
+	if (!can_modify_vma(vma))
+		return ERR_PTR(-EPERM);
+
 	/*
 	 * !old_len is a special case where an attempt is made to 'duplicate'
 	 * a mapping.  This makes no sense for private mappings as it will
@@ -902,19 +909,6 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
 	if ((mm->map_count + 2) >= sysctl_max_map_count - 3)
 		return -ENOMEM;
 
-	/*
-	 * In mremap_to().
-	 * Move a VMA to another location, check if src addr is sealed.
-	 *
-	 * Place can_modify_mm here because mremap_to()
-	 * does its own checking for address range, and we only
-	 * check the sealing after passing those checks.
-	 *
-	 * can_modify_mm assumes we have acquired the lock on MM.
-	 */
-	if (unlikely(!can_modify_mm(mm, addr, addr + old_len)))
-		return -EPERM;
-
 	if (flags & MREMAP_FIXED) {
 		/*
 		 * In mremap_to().
@@ -1079,19 +1073,6 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
 		goto out;
 	}
 
-	/*
-	 * Below is shrink/expand case (not mremap_to())
-	 * Check if src address is sealed, if so, reject.
-	 * In other words, prevent shrinking or expanding a sealed VMA.
-	 *
-	 * Place can_modify_mm here so we can keep the logic related to
-	 * shrink/expand together.
-	 */
-	if (unlikely(!can_modify_mm(mm, addr, addr + old_len))) {
-		ret = -EPERM;
-		goto out;
-	}
-
 	/*
 	 * Always allow a shrinking remap: that just unmaps
 	 * the unnecessary pages..
-- 
2.46.0

[PATCH 5/7] mseal: Fix is_madv_discard()

[Pedro Falcato]

is_madv_discard did its check wrong. MADV_ flags are not bitwise,
they're normal sequential numbers. So, for instance:
	behavior & (/* ... */ | MADV_REMOVE)

tagged both MADV_REMOVE and MADV_RANDOM (bit 0 set) as
discard operations. This is obviously incorrect, so use
a switch statement instead.

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 mm/mseal.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/mm/mseal.c b/mm/mseal.c
index 4591ae8d29c..2170e2139ca 100644
--- a/mm/mseal.c
+++ b/mm/mseal.c
@@ -23,9 +23,17 @@ static inline void set_vma_sealed(struct vm_area_struct *vma)
 
 static bool is_madv_discard(int behavior)
 {
-	return	behavior &
-		(MADV_FREE | MADV_DONTNEED | MADV_DONTNEED_LOCKED |
-		 MADV_REMOVE | MADV_DONTFORK | MADV_WIPEONFORK);
+	switch (behavior) {
+	case MADV_FREE:
+	case MADV_DONTNEED:
+	case MADV_DONTNEED_LOCKED:
+	case MADV_REMOVE:
+	case MADV_DONTFORK:
+	case MADV_WIPEONFORK:
+		return true;
+	}
+
+	return false;
 }
 
 static bool is_ro_anon(struct vm_area_struct *vma)
-- 
2.46.0

[PATCH 6/7] mseal: Replace can_modify_mm_madv with a vma variant

[Pedro Falcato]

Replace can_modify_mm_madv() with a single vma variant, and associated
checks in madvise.

While we're at it, also invert the order of checks in:
 if (unlikely(is_ro_anon(vma) && !can_modify_vma(vma))

Checking if we can modify the vma itself (through vm_flags) is
certainly cheaper than is_ro_anon() due to arch_vma_access_permitted()
looking at e.g pkeys registers (with extra branches) in some
architectures.

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 mm/internal.h |  6 ++----
 mm/madvise.c  | 13 +++----------
 mm/mseal.c    | 17 ++++-------------
 3 files changed, 9 insertions(+), 27 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index 09ea930540f..4b516618389 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -1517,8 +1517,7 @@ static inline bool can_modify_vma(struct vm_area_struct *vma)
 
 bool can_modify_mm(struct mm_struct *mm, unsigned long start,
 		unsigned long end);
-bool can_modify_mm_madv(struct mm_struct *mm, unsigned long start,
-		unsigned long end, int behavior);
+bool can_modify_vma_madv(struct vm_area_struct *vma, int behavior);
 #else
 static inline int can_do_mseal(unsigned long flags)
 {
@@ -1531,8 +1530,7 @@ static inline bool can_modify_mm(struct mm_struct *mm, unsigned long start,
 	return true;
 }
 
-static inline bool can_modify_mm_madv(struct mm_struct *mm, unsigned long start,
-		unsigned long end, int behavior)
+static inline bool can_modify_vma_madv(struct vm_area_struct *vma, int behavior)
 {
 	return true;
 }
diff --git a/mm/madvise.c b/mm/madvise.c
index 89089d84f8d..4e64770be16 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1031,6 +1031,9 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
 	struct anon_vma_name *anon_name;
 	unsigned long new_flags = vma->vm_flags;
 
+	if (unlikely(!can_modify_vma_madv(vma, behavior)))
+		return -EPERM;
+
 	switch (behavior) {
 	case MADV_REMOVE:
 		return madvise_remove(vma, prev, start, end);
@@ -1448,15 +1451,6 @@ int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int beh
 	start = untagged_addr_remote(mm, start);
 	end = start + len;
 
-	/*
-	 * Check if the address range is sealed for do_madvise().
-	 * can_modify_mm_madv assumes we have acquired the lock on MM.
-	 */
-	if (unlikely(!can_modify_mm_madv(mm, start, end, behavior))) {
-		error = -EPERM;
-		goto out;
-	}
-
 	blk_start_plug(&plug);
 	switch (behavior) {
 	case MADV_POPULATE_READ:
@@ -1470,7 +1464,6 @@ int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int beh
 	}
 	blk_finish_plug(&plug);
 
-out:
 	if (write)
 		mmap_write_unlock(mm);
 	else
diff --git a/mm/mseal.c b/mm/mseal.c
index 2170e2139ca..fdd1666344f 100644
--- a/mm/mseal.c
+++ b/mm/mseal.c
@@ -75,24 +75,15 @@ bool can_modify_mm(struct mm_struct *mm, unsigned long start, unsigned long end)
 }
 
 /*
- * Check if the vmas of a memory range are allowed to be modified by madvise.
- * the memory ranger can have a gap (unallocated memory).
- * return true, if it is allowed.
+ * Check if a vma is allowed to be modified by madvise.
  */
-bool can_modify_mm_madv(struct mm_struct *mm, unsigned long start, unsigned long end,
-		int behavior)
+bool can_modify_vma_madv(struct vm_area_struct *vma, int behavior)
 {
-	struct vm_area_struct *vma;
-
-	VMA_ITERATOR(vmi, mm, start);
-
 	if (!is_madv_discard(behavior))
 		return true;
 
-	/* going through each vma to check. */
-	for_each_vma_range(vmi, vma, end)
-		if (unlikely(is_ro_anon(vma) && !can_modify_vma(vma)))
-			return false;
+	if (unlikely(!can_modify_vma(vma) && is_ro_anon(vma)))
+		return false;
 
 	/* Allow by default. */
 	return true;
-- 
2.46.0

[PATCH 7/7] mm: Remove can_modify_mm()

[Pedro Falcato]

With no more users in the tree, we can finally remove can_modify_mm().

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 mm/internal.h |  8 --------
 mm/mseal.c    | 21 ---------------------
 2 files changed, 29 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index 4b516618389..e4416f5da2f 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -1515,8 +1515,6 @@ static inline bool can_modify_vma(struct vm_area_struct *vma)
 }
 
 
-bool can_modify_mm(struct mm_struct *mm, unsigned long start,
-		unsigned long end);
 bool can_modify_vma_madv(struct vm_area_struct *vma, int behavior);
 #else
 static inline int can_do_mseal(unsigned long flags)
@@ -1524,12 +1522,6 @@ static inline int can_do_mseal(unsigned long flags)
 	return -EPERM;
 }
 
-static inline bool can_modify_mm(struct mm_struct *mm, unsigned long start,
-		unsigned long end)
-{
-	return true;
-}
-
 static inline bool can_modify_vma_madv(struct vm_area_struct *vma, int behavior)
 {
 	return true;
diff --git a/mm/mseal.c b/mm/mseal.c
index fdd1666344f..28cd17d7aaf 100644
--- a/mm/mseal.c
+++ b/mm/mseal.c
@@ -53,27 +53,6 @@ static bool is_ro_anon(struct vm_area_struct *vma)
 	return false;
 }
 
-/*
- * Check if the vmas of a memory range are allowed to be modified.
- * the memory ranger can have a gap (unallocated memory).
- * return true, if it is allowed.
- */
-bool can_modify_mm(struct mm_struct *mm, unsigned long start, unsigned long end)
-{
-	struct vm_area_struct *vma;
-
-	VMA_ITERATOR(vmi, mm, start);
-
-	/* going through each vma to check. */
-	for_each_vma_range(vmi, vma, end) {
-		if (unlikely(!can_modify_vma(vma)))
-			return false;
-	}
-
-	/* Allow by default. */
-	return true;
-}
-
 /*
  * Check if a vma is allowed to be modified by madvise.
  */
-- 
2.46.0

Re: [PATCH 0/7] mm: Optimize mseal checks

[Jeff Xu]

On Tue, Aug 6, 2024 at 2:28 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> Optimize mseal checks by removing the separate can_modify_mm() step, and
> just doing checks on the individual vmas, when various operations are
> themselves iterating through the tree. This provides a nice speedup.
>
> While I was at it, I found that is_madv_discard() was completely bogus.
>
Thanks for catching this!
Is it possible to separate this fix out from this series and send it
separately and merge first ?

> Note that my series ignores arch_unmap(), which seems to generally be what we're trending towards[2]. It should
> be applied on top of any powerpc vdso ->close patch to avoid regressions on the PPC architecture. No other
> architecture seems to use arch_unmap.
>
> Note2: This series does not pass all mseal_tests on my end (test_seal_mremap_move_dontunmap_anyaddr fails twice). But the
> top of Linus's tree does not pass these for me either (neither does my Arch Linux 6.10.2 kernel),
> for some reason (mremap regression?).
>
I just sync to Linus's main and I was able to run the test (except two
pkeys related test are skipped because I m on VM)

> will-it-scale mmap1_process[1] -t 1 results:
>
> commit 3450fe2b574b4345e4296ccae395149e1a357fee:
>
> min:277605 max:277605 total:277605
> min:281784 max:281784 total:281784
> min:277238 max:277238 total:277238
> min:281761 max:281761 total:281761
> min:274279 max:274279 total:274279
> min:254854 max:254854 total:254854
> measurement
> min:269143 max:269143 total:269143
> min:270454 max:270454 total:270454
> min:243523 max:243523 total:243523
> min:251148 max:251148 total:251148
> min:209669 max:209669 total:209669
> min:190426 max:190426 total:190426
> min:231219 max:231219 total:231219
> min:275364 max:275364 total:275364
> min:266540 max:266540 total:266540
> min:242572 max:242572 total:242572
> min:284469 max:284469 total:284469
> min:278882 max:278882 total:278882
> min:283269 max:283269 total:283269
> min:281204 max:281204 total:281204
>
> After this patch set:
>
> min:280580 max:280580 total:280580
> min:290514 max:290514 total:290514
> min:291006 max:291006 total:291006
> min:290352 max:290352 total:290352
> min:294582 max:294582 total:294582
> min:293075 max:293075 total:293075
> measurement
> min:295613 max:295613 total:295613
> min:294070 max:294070 total:294070
> min:293193 max:293193 total:293193
> min:291631 max:291631 total:291631
> min:295278 max:295278 total:295278
> min:293782 max:293782 total:293782
> min:290361 max:290361 total:290361
> min:294517 max:294517 total:294517
> min:293750 max:293750 total:293750
> min:293572 max:293572 total:293572
> min:295239 max:295239 total:295239
> min:292932 max:292932 total:292932
> min:293319 max:293319 total:293319
> min:294954 max:294954 total:294954
>
> This was a Completely Unscientific test but seems to show there were around 5-10% gains on ops per second.
>
> [1]: mmap1_process does mmap and munmap in a loop. I didn't bother testing multithreading cases.
> [2]: https://lore.kernel.org/all/87o766iehy.fsf@mail.lhotse/
> Link: https://lore.kernel.org/all/202408041602.caa0372-oliver.sang@intel.com/
>
> Pedro Falcato (7):
>   mm: Move can_modify_vma to mm/internal.h
>   mm/munmap: Replace can_modify_mm with can_modify_vma
>   mm/mprotect: Replace can_modify_mm with can_modify_vma
>   mm/mremap: Replace can_modify_mm with can_modify_vma
>   mseal: Fix is_madv_discard()
>   mseal: Replace can_modify_mm_madv with a vma variant
>   mm: Remove can_modify_mm()
>
>  mm/internal.h | 30 ++++++++++++++++------
>  mm/madvise.c  | 13 +++-------
>  mm/mmap.c     | 36 ++++++++++-----------------
>  mm/mprotect.c | 12 +++------
>  mm/mremap.c   | 33 ++++++------------------
>  mm/mseal.c    | 69 +++++++++++----------------------------------------
>  6 files changed, 63 insertions(+), 130 deletions(-)
>
> --
> 2.46.0
>

Re: [PATCH 4/7] mm/mremap: Replace can_modify_mm with can_modify_vma

[Jeff Xu]

On Tue, Aug 6, 2024 at 2:28 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> Delegate all can_modify checks to the proper places. Unmap checks are
> done in do_unmap (et al).
>
> This patch allows for mremap partial failure in certain cases (for
> instance, when destination VMAs aren't sealed, but the source VMA is).
> It shouldn't be too troublesome, as you'd need to go out of your way to
> do illegal operations on a VMA.
>
> Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
> ---
>  mm/mremap.c | 33 +++++++--------------------------
>  1 file changed, 7 insertions(+), 26 deletions(-)
>
> diff --git a/mm/mremap.c b/mm/mremap.c
> index e7ae140fc64..8af877d7bb0 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -676,6 +676,9 @@ static unsigned long move_vma(struct vm_area_struct *vma,
>         if (unlikely(flags & MREMAP_DONTUNMAP))
>                 to_account = new_len;
>
> +       if (!can_modify_vma(vma))
> +               return -EPERM;
> +
I m not 100% sure, but I suspect you don't need this check? Is
vma_to_resize already checking the src address ?

PS. Is it possible to consolidate all the related changes (except the
fix for madvise) to a single commit ?
 It would be easier to look for dependency, e.g. the remap depends on munmap().

Also selftest is helpful to prove the correctness of the change. (And
I can also test it)

>         if (vma->vm_ops && vma->vm_ops->may_split) {
>                 if (vma->vm_start != old_addr)
>                         err = vma->vm_ops->may_split(vma, old_addr);
> @@ -821,6 +824,10 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
>         if (!vma)
>                 return ERR_PTR(-EFAULT);
>
> +       /* Don't allow vma expansion when it has already been sealed */
> +       if (!can_modify_vma(vma))
> +               return ERR_PTR(-EPERM);
> +
>         /*
>          * !old_len is a special case where an attempt is made to 'duplicate'
>          * a mapping.  This makes no sense for private mappings as it will
> @@ -902,19 +909,6 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
>         if ((mm->map_count + 2) >= sysctl_max_map_count - 3)
>                 return -ENOMEM;
>
> -       /*
> -        * In mremap_to().
> -        * Move a VMA to another location, check if src addr is sealed.
> -        *
> -        * Place can_modify_mm here because mremap_to()
> -        * does its own checking for address range, and we only
> -        * check the sealing after passing those checks.
> -        *
> -        * can_modify_mm assumes we have acquired the lock on MM.
> -        */
> -       if (unlikely(!can_modify_mm(mm, addr, addr + old_len)))
> -               return -EPERM;
> -
>         if (flags & MREMAP_FIXED) {
>                 /*
>                  * In mremap_to().
> @@ -1079,19 +1073,6 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
>                 goto out;
>         }
>
> -       /*
> -        * Below is shrink/expand case (not mremap_to())
> -        * Check if src address is sealed, if so, reject.
> -        * In other words, prevent shrinking or expanding a sealed VMA.
> -        *
> -        * Place can_modify_mm here so we can keep the logic related to
> -        * shrink/expand together.
> -        */
> -       if (unlikely(!can_modify_mm(mm, addr, addr + old_len))) {
> -               ret = -EPERM;
> -               goto out;
> -       }
> -
>         /*
>          * Always allow a shrinking remap: that just unmaps
>          * the unnecessary pages..
> --
> 2.46.0
>

Re: [PATCH 0/7] mm: Optimize mseal checks

[Pedro Falcato]

On Tue, Aug 6, 2024 at 11:25 PM Jeff Xu <jeffxu@google.com> wrote:
>
> On Tue, Aug 6, 2024 at 2:28 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> >
> > Optimize mseal checks by removing the separate can_modify_mm() step, and
> > just doing checks on the individual vmas, when various operations are
> > themselves iterating through the tree. This provides a nice speedup.
> >
> > While I was at it, I found that is_madv_discard() was completely bogus.
> >
> Thanks for catching this!
> Is it possible to separate this fix out from this series and send it
> separately and merge first ?

Sure. This series is definitely too risky to catch this release, so
sending it out as a fix (tomorrow, it's late here) sounds ok.

>
> > Note that my series ignores arch_unmap(), which seems to generally be what we're trending towards[2]. It should
> > be applied on top of any powerpc vdso ->close patch to avoid regressions on the PPC architecture. No other
> > architecture seems to use arch_unmap.
> >
> > Note2: This series does not pass all mseal_tests on my end (test_seal_mremap_move_dontunmap_anyaddr fails twice). But the
> > top of Linus's tree does not pass these for me either (neither does my Arch Linux 6.10.2 kernel),
> > for some reason (mremap regression?).
> >
> I just sync to Linus's main and I was able to run the test (except two
> pkeys related test are skipped because I m on VM)

Okay. Fun bug.

I was really confused as to why no one could repro this except me :)

It looks like recently[1] glibc started consuming the new_address
variadic argument when MREMAP_DONTUNMAP. As to the why,
MREMAP_DONTUNMAP also seems to take new_address as a hint (this is not
documented in the man page, and strace also doesn't know this).
However, this trips up some checks that were always fine before
(because glibc always passed NULL, and musl still does):

if (offset_in_page(new_addr))
if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len)
if (addr + old_len > new_addr && new_addr + new_len > addr)

^^ These all look at the address without looking at MREMAP_FIXED, and
return -EINVAL if they fail.

So, test_seal_mremap_move_dontunmap_anyaddr passes 0xdeadbeef For Some
Reason (why are you testing mremap in mseal_test.c??), it trips up
offset_in_page(new_addr) in mremap_to, and we crash and burn.

As to why no one else could repro this: I guess you're not running a
glibc new enough ;)

[1] https://sourceware.org/git/?p=glibc.git;a=commit;h=6c40cb0e9f893d49dc7caee580a055de53562206

-- 
Pedro

Re: [PATCH 4/7] mm/mremap: Replace can_modify_mm with can_modify_vma

[Pedro Falcato]

On Wed, Aug 7, 2024 at 12:09 AM Jeff Xu <jeffxu@google.com> wrote:
>
> On Tue, Aug 6, 2024 at 2:28 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> >
> > Delegate all can_modify checks to the proper places. Unmap checks are
> > done in do_unmap (et al).
> >
> > This patch allows for mremap partial failure in certain cases (for
> > instance, when destination VMAs aren't sealed, but the source VMA is).
> > It shouldn't be too troublesome, as you'd need to go out of your way to
> > do illegal operations on a VMA.
> >
> > Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
> > ---
> >  mm/mremap.c | 33 +++++++--------------------------
> >  1 file changed, 7 insertions(+), 26 deletions(-)
> >
> > diff --git a/mm/mremap.c b/mm/mremap.c
> > index e7ae140fc64..8af877d7bb0 100644
> > --- a/mm/mremap.c
> > +++ b/mm/mremap.c
> > @@ -676,6 +676,9 @@ static unsigned long move_vma(struct vm_area_struct *vma,
> >         if (unlikely(flags & MREMAP_DONTUNMAP))
> >                 to_account = new_len;
> >
> > +       if (!can_modify_vma(vma))
> > +               return -EPERM;
> > +
> I m not 100% sure, but I suspect you don't need this check? Is
> vma_to_resize already checking the src address ?

Hmm, yes, good point.

>
> PS. Is it possible to consolidate all the related changes (except the
> fix for madvise) to a single commit ?

The patch set is organized logically, in simple clear steps. As a
maintainer, I would prefer to review something like this vs a big
confusing patch that touches many things at once.
Of course if the maintainers think this is too coarse (it's not
exactly a large patch set, just moves a lot of code back and forth),
I'm happy to merge these into larger chunks.

>  It would be easier to look for dependency, e.g. the remap depends on munmap().

All patches depend on the previous (and a single patch would make it
harder to see these dependencies). The kernel should build and the
selftests should pass for every patch in the set.

>
> Also selftest is helpful to prove the correctness of the change. (And
> I can also test it)

I have run selftests, and it is.

-- 
Pedro

Re: [PATCH 0/7] mm: Optimize mseal checks

[Jeff Xu]

On Tue, Aug 6, 2024 at 5:49 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> On Tue, Aug 6, 2024 at 11:25 PM Jeff Xu <jeffxu@google.com> wrote:
> >
> > On Tue, Aug 6, 2024 at 2:28 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> > >
> > > Optimize mseal checks by removing the separate can_modify_mm() step, and
> > > just doing checks on the individual vmas, when various operations are
> > > themselves iterating through the tree. This provides a nice speedup.
> > >
> > > While I was at it, I found that is_madv_discard() was completely bogus.
> > >
> > Thanks for catching this!
> > Is it possible to separate this fix out from this series and send it
> > separately and merge first ?
>
> Sure. This series is definitely too risky to catch this release, so
> sending it out as a fix (tomorrow, it's late here) sounds ok.
>
Do you mind if I send out a fix ? (I will also include a test case to
cover that )

> >
> > > Note that my series ignores arch_unmap(), which seems to generally be what we're trending towards[2]. It should
> > > be applied on top of any powerpc vdso ->close patch to avoid regressions on the PPC architecture. No other
> > > architecture seems to use arch_unmap.
> > >
> > > Note2: This series does not pass all mseal_tests on my end (test_seal_mremap_move_dontunmap_anyaddr fails twice). But the
> > > top of Linus's tree does not pass these for me either (neither does my Arch Linux 6.10.2 kernel),
> > > for some reason (mremap regression?).
> > >
> > I just sync to Linus's main and I was able to run the test (except two
> > pkeys related test are skipped because I m on VM)
>
> Okay. Fun bug.
>
> I was really confused as to why no one could repro this except me :)
>
> It looks like recently[1] glibc started consuming the new_address
> variadic argument when MREMAP_DONTUNMAP. As to the why,
> MREMAP_DONTUNMAP also seems to take new_address as a hint (this is not
> documented in the man page, and strace also doesn't know this).
> However, this trips up some checks that were always fine before
> (because glibc always passed NULL, and musl still does):
>
> if (offset_in_page(new_addr))
> if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len)
> if (addr + old_len > new_addr && new_addr + new_len > addr)
>
> ^^ These all look at the address without looking at MREMAP_FIXED, and
> return -EINVAL if they fail.
>
> So, test_seal_mremap_move_dontunmap_anyaddr passes 0xdeadbeef For Some
> Reason (why are you testing mremap in mseal_test.c??), it trips up
> offset_in_page(new_addr) in mremap_to, and we crash and burn.
>
> As to why no one else could repro this: I guess you're not running a
> glibc new enough ;)
>
That makes sense, mystery resolved.

I added sys_ functions for mmap/munmap/mprotect, etc, so that the test
does not depend on libc, but I didn't do that for mremap, I think the
fix will be to add sys_mremap as well.


> [1] https://sourceware.org/git/?p=glibc.git;a=commit;h=6c40cb0e9f893d49dc7caee580a055de53562206
>
> --
> Pedro

Re: [PATCH 4/7] mm/mremap: Replace can_modify_mm with can_modify_vma

[Jeff Xu]

On Tue, Aug 6, 2024 at 5:59 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> On Wed, Aug 7, 2024 at 12:09 AM Jeff Xu <jeffxu@google.com> wrote:
> >
> > On Tue, Aug 6, 2024 at 2:28 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> > >
> > > Delegate all can_modify checks to the proper places. Unmap checks are
> > > done in do_unmap (et al).
> > >
> > > This patch allows for mremap partial failure in certain cases (for
> > > instance, when destination VMAs aren't sealed, but the source VMA is).
> > > It shouldn't be too troublesome, as you'd need to go out of your way to
> > > do illegal operations on a VMA.
> > >
> > > Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
> > > ---
> > >  mm/mremap.c | 33 +++++++--------------------------
> > >  1 file changed, 7 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/mm/mremap.c b/mm/mremap.c
> > > index e7ae140fc64..8af877d7bb0 100644
> > > --- a/mm/mremap.c
> > > +++ b/mm/mremap.c
> > > @@ -676,6 +676,9 @@ static unsigned long move_vma(struct vm_area_struct *vma,
> > >         if (unlikely(flags & MREMAP_DONTUNMAP))
> > >                 to_account = new_len;
> > >
> > > +       if (!can_modify_vma(vma))
> > > +               return -EPERM;
> > > +
> > I m not 100% sure, but I suspect you don't need this check? Is
> > vma_to_resize already checking the src address ?
>
> Hmm, yes, good point.
>
> >
> > PS. Is it possible to consolidate all the related changes (except the
> > fix for madvise) to a single commit ?
>
> The patch set is organized logically, in simple clear steps. As a
> maintainer, I would prefer to review something like this vs a big
> confusing patch that touches many things at once.
> Of course if the maintainers think this is too coarse (it's not
> exactly a large patch set, just moves a lot of code back and forth),
> I'm happy to merge these into larger chunks.
>
Yes. The patch is not exactly large. (The original mseal patch  has
one single patch for adding mseal code logic, and one for test).

Also the mseal has been backported to  the 6.6 kernel for ChromeOS,
if you keep the code change in one , it will make future backport
easier for me.

> >  It would be easier to look for dependency, e.g. the remap depends on munmap().
>
> All patches depend on the previous (and a single patch would make it
> harder to see these dependencies). The kernel should build and the
> selftests should pass for every patch in the set.
>
> >
> > Also selftest is helpful to prove the correctness of the change. (And
> > I can also test it)
>
> I have run selftests, and it is.
>
> --
> Pedro

Re: [PATCH 0/7] mm: Optimize mseal checks

[Pedro Falcato]

On Wed, Aug 7, 2024 at 2:40 AM Jeff Xu <jeffxu@google.com> wrote:
>
> On Tue, Aug 6, 2024 at 5:49 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> >
> > On Tue, Aug 6, 2024 at 11:25 PM Jeff Xu <jeffxu@google.com> wrote:
> > >
> > > On Tue, Aug 6, 2024 at 2:28 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> > > >
> > > > Optimize mseal checks by removing the separate can_modify_mm() step, and
> > > > just doing checks on the individual vmas, when various operations are
> > > > themselves iterating through the tree. This provides a nice speedup.
> > > >
> > > > While I was at it, I found that is_madv_discard() was completely bogus.
> > > >
> > > Thanks for catching this!
> > > Is it possible to separate this fix out from this series and send it
> > > separately and merge first ?
> >
> > Sure. This series is definitely too risky to catch this release, so
> > sending it out as a fix (tomorrow, it's late here) sounds ok.
> >
> Do you mind if I send out a fix ? (I will also include a test case to
> cover that )

No need, I'll handle it before the end of the day.

>
> > >
> > > > Note that my series ignores arch_unmap(), which seems to generally be what we're trending towards[2]. It should
> > > > be applied on top of any powerpc vdso ->close patch to avoid regressions on the PPC architecture. No other
> > > > architecture seems to use arch_unmap.
> > > >
> > > > Note2: This series does not pass all mseal_tests on my end (test_seal_mremap_move_dontunmap_anyaddr fails twice). But the
> > > > top of Linus's tree does not pass these for me either (neither does my Arch Linux 6.10.2 kernel),
> > > > for some reason (mremap regression?).
> > > >
> > > I just sync to Linus's main and I was able to run the test (except two
> > > pkeys related test are skipped because I m on VM)
> >
> > Okay. Fun bug.
> >
> > I was really confused as to why no one could repro this except me :)
> >
> > It looks like recently[1] glibc started consuming the new_address
> > variadic argument when MREMAP_DONTUNMAP. As to the why,
> > MREMAP_DONTUNMAP also seems to take new_address as a hint (this is not
> > documented in the man page, and strace also doesn't know this).
> > However, this trips up some checks that were always fine before
> > (because glibc always passed NULL, and musl still does):
> >
> > if (offset_in_page(new_addr))
> > if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len)
> > if (addr + old_len > new_addr && new_addr + new_len > addr)
> >
> > ^^ These all look at the address without looking at MREMAP_FIXED, and
> > return -EINVAL if they fail.
> >
> > So, test_seal_mremap_move_dontunmap_anyaddr passes 0xdeadbeef For Some
> > Reason (why are you testing mremap in mseal_test.c??), it trips up
> > offset_in_page(new_addr) in mremap_to, and we crash and burn.
> >
> > As to why no one else could repro this: I guess you're not running a
> > glibc new enough ;)
> >
> That makes sense, mystery resolved.
>
> I added sys_ functions for mmap/munmap/mprotect, etc, so that the test
> does not depend on libc, but I didn't do that for mremap, I think the
> fix will be to add sys_mremap as well.

I disagree, I don't understand why you're doing this test. And even if
you are rightfully doing the test, the test is wrong (and
mremap_dontunmap.c tests agree, and always pass 0 as new_address).
The manpage needs to be updated to reflect this, and this test either
needs the 0xdeadbeef removed, or the whole thing.

Adding a sys_mremap wrapper is inconsequential here, because you'll
need to decide whether to pick up new_address from the flags argument
and, if you do, it'll fail with the same error, but for everyone.

-- 
Pedro

Re: [PATCH 2/7] mm/munmap: Replace can_modify_mm with can_modify_vma

[Lorenzo Stoakes]

On Tue, Aug 06, 2024 at 10:28:03PM GMT, Pedro Falcato wrote:
> We were doing an extra mmap tree traversal just to check if the entire
> range is modifiable. This can be done when we iterate through the VMAs
> instead.
>
> Note that this removes the arch_unmap() callsites and therefore isn't
> quite ready for Proper(tm) upstreaming.

If this isn't ready for upstreaming which is it being submitted as a patch
series and not an RFC or such?

Liam is likely to do some significant rework of this arch_unmap() stuff
soon, and is certainly significantly reworking the munmap() logic, so to
avoid conflicts it goes doubly that if this isn't meant for upstream then
it should be RFC'd.

>
> Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>

This patch doesn't apply in the mm-unstable tree. If you want your series
to come in through the mm tree you need to rebase on this.

I made a major change to file structure which moves a bunch of mm/mmap.c
stuff to mm/vma.c (similarly moving things around in headers), which is
why.

It also means I can't sensibly review it... :)

> ---
>  mm/mmap.c | 36 +++++++++++++-----------------------
>  1 file changed, 13 insertions(+), 23 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index d0dfc85b209..b88666f618b 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2660,6 +2660,12 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  		if (end < vma->vm_end && mm->map_count >= sysctl_max_map_count)
>  			goto map_count_exceeded;
>
> +		/* Don't bother splitting the VMA if we can't unmap it anyway */
> +		if (!can_modify_vma(vma)) {
> +			error = -EPERM;
> +			goto start_split_failed;
> +		}
> +
>  		error = __split_vma(vmi, vma, start, 1);
>  		if (error)
>  			goto start_split_failed;
> @@ -2671,6 +2677,11 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  	 */
>  	next = vma;
>  	do {
> +		if (!can_modify_vma(vma)) {
> +			error = -EPERM;
> +			goto modify_vma_failed;
> +		}
> +
>  		/* Does it split the end? */
>  		if (next->vm_end > end) {
>  			error = __split_vma(vmi, next, end, 0);
> @@ -2763,6 +2774,7 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  	__mt_destroy(&mt_detach);
>  	return 0;
>
> +modify_vma_failed:
>  clear_tree_failed:
>  userfaultfd_error:
>  munmap_gather_failed:
> @@ -2808,17 +2820,6 @@ int do_vmi_munmap(struct vma_iterator *vmi, struct mm_struct *mm,
>  	if (end == start)
>  		return -EINVAL;
>
> -	/*
> -	 * Check if memory is sealed before arch_unmap.
> -	 * Prevent unmapping a sealed VMA.
> -	 * can_modify_mm assumes we have acquired the lock on MM.
> -	 */
> -	if (unlikely(!can_modify_mm(mm, start, end)))
> -		return -EPERM;
> -
> -	 /* arch_unmap() might do unmaps itself.  */
> -	arch_unmap(mm, start, end);
> -
>  	/* Find the first overlapping VMA */
>  	vma = vma_find(vmi, end);
>  	if (!vma) {
> @@ -3229,18 +3230,7 @@ int do_vma_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  		unsigned long start, unsigned long end, struct list_head *uf,
>  		bool unlock)
>  {
> -	struct mm_struct *mm = vma->vm_mm;
> -
> -	/*
> -	 * Check if memory is sealed before arch_unmap.
> -	 * Prevent unmapping a sealed VMA.
> -	 * can_modify_mm assumes we have acquired the lock on MM.
> -	 */
> -	if (unlikely(!can_modify_mm(mm, start, end)))
> -		return -EPERM;
> -
> -	arch_unmap(mm, start, end);
> -	return do_vmi_align_munmap(vmi, vma, mm, start, end, uf, unlock);
> +	return do_vmi_align_munmap(vmi, vma, vma->vm_mm, start, end, uf, unlock);
>  }
>
>  /*
> --
> 2.46.0
>

Re: [PATCH 5/7] mseal: Fix is_madv_discard()

[Lorenzo Stoakes]

On Tue, Aug 06, 2024 at 10:28:06PM GMT, Pedro Falcato wrote:
> is_madv_discard did its check wrong. MADV_ flags are not bitwise,
> they're normal sequential numbers. So, for instance:
> 	behavior & (/* ... */ | MADV_REMOVE)
>
> tagged both MADV_REMOVE and MADV_RANDOM (bit 0 set) as
> discard operations. This is obviously incorrect, so use
> a switch statement instead.
>
> Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
> ---
>  mm/mseal.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/mm/mseal.c b/mm/mseal.c
> index 4591ae8d29c..2170e2139ca 100644
> --- a/mm/mseal.c
> +++ b/mm/mseal.c
> @@ -23,9 +23,17 @@ static inline void set_vma_sealed(struct vm_area_struct *vma)
>
>  static bool is_madv_discard(int behavior)
>  {
> -	return	behavior &
> -		(MADV_FREE | MADV_DONTNEED | MADV_DONTNEED_LOCKED |
> -		 MADV_REMOVE | MADV_DONTFORK | MADV_WIPEONFORK);
> +	switch (behavior) {
> +	case MADV_FREE:
> +	case MADV_DONTNEED:
> +	case MADV_DONTNEED_LOCKED:
> +	case MADV_REMOVE:
> +	case MADV_DONTFORK:
> +	case MADV_WIPEONFORK:
> +		return true;
> +	}
> +
> +	return false;
>  }
>
>  static bool is_ro_anon(struct vm_area_struct *vma)
> --
> 2.46.0
>

Wow. Great spot, and what an oversight. Agree with Jeff, we really sould
pull this out separately as this is something that urgently needs fixing.

Ideally we'd add a test of some kind, but since it's so obviously wrong I
think it'd be fine without at least as a quick fixup.

This will need to be hotfixed/cc-d to stable too since it's in a released
kernel version.

Re: [PATCH 2/7] mm/munmap: Replace can_modify_mm with can_modify_vma

[Pedro Falcato]

On Wed, Aug 7, 2024 at 2:02 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Tue, Aug 06, 2024 at 10:28:03PM GMT, Pedro Falcato wrote:
> > We were doing an extra mmap tree traversal just to check if the entire
> > range is modifiable. This can be done when we iterate through the VMAs
> > instead.
> >
> > Note that this removes the arch_unmap() callsites and therefore isn't
> > quite ready for Proper(tm) upstreaming.
>
> If this isn't ready for upstreaming which is it being submitted as a patch
> series and not an RFC or such?

Crap... I wasn't sure whether to mark this as RFC or not (I wasn't
sure if this could be applied as a hotfix, yes it's a little risky but
the changes themselves are simple, and fix an active regression).
I'll err on the side of caution next time :)

>
> Liam is likely to do some significant rework of this arch_unmap() stuff
> soon, and is certainly significantly reworking the munmap() logic, so to

FWIW there was a new series posted at
https://lore.kernel.org/linuxppc-dev/20240807124103.85644-1-mpe@ellerman.id.au/T/#m353eb23fc263033c9ca023ead6fa82d1a1ff3263
that do away with arch_unmap altogether (resulting from our exchange
on the regression thread).

> avoid conflicts it goes doubly that if this isn't meant for upstream then
> it should be RFC'd.
>
> >
> > Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
>
> This patch doesn't apply in the mm-unstable tree. If you want your series
> to come in through the mm tree you need to rebase on this.
>
> I made a major change to file structure which moves a bunch of mm/mmap.c
> stuff to mm/vma.c (similarly moving things around in headers), which is
> why.
>
> It also means I can't sensibly review it... :)

ACK. I'll rebase on mm-unstable for v2, sorry for the time lost.

-- 
Pedro

Re: [PATCH 0/7] mm: Optimize mseal checks

[Jeff Xu]

On Wed, Aug 7, 2024 at 5:57 AM Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> On Wed, Aug 7, 2024 at 2:40 AM Jeff Xu <jeffxu@google.com> wrote:
> >
> > On Tue, Aug 6, 2024 at 5:49 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> > >
> > > On Tue, Aug 6, 2024 at 11:25 PM Jeff Xu <jeffxu@google.com> wrote:
> > > >
> > > > On Tue, Aug 6, 2024 at 2:28 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> > > > >
> > > > > Optimize mseal checks by removing the separate can_modify_mm() step, and
> > > > > just doing checks on the individual vmas, when various operations are
> > > > > themselves iterating through the tree. This provides a nice speedup.
> > > > >
> > > > > While I was at it, I found that is_madv_discard() was completely bogus.
> > > > >
> > > > Thanks for catching this!
> > > > Is it possible to separate this fix out from this series and send it
> > > > separately and merge first ?
> > >
> > > Sure. This series is definitely too risky to catch this release, so
> > > sending it out as a fix (tomorrow, it's late here) sounds ok.
> > >
> > Do you mind if I send out a fix ? (I will also include a test case to
> > cover that )
>
> No need, I'll handle it before the end of the day.
>
> >
> > > >
> > > > > Note that my series ignores arch_unmap(), which seems to generally be what we're trending towards[2]. It should
> > > > > be applied on top of any powerpc vdso ->close patch to avoid regressions on the PPC architecture. No other
> > > > > architecture seems to use arch_unmap.
> > > > >
> > > > > Note2: This series does not pass all mseal_tests on my end (test_seal_mremap_move_dontunmap_anyaddr fails twice). But the
> > > > > top of Linus's tree does not pass these for me either (neither does my Arch Linux 6.10.2 kernel),
> > > > > for some reason (mremap regression?).
> > > > >
> > > > I just sync to Linus's main and I was able to run the test (except two
> > > > pkeys related test are skipped because I m on VM)
> > >
> > > Okay. Fun bug.
> > >
> > > I was really confused as to why no one could repro this except me :)
> > >
> > > It looks like recently[1] glibc started consuming the new_address
> > > variadic argument when MREMAP_DONTUNMAP. As to the why,
> > > MREMAP_DONTUNMAP also seems to take new_address as a hint (this is not
> > > documented in the man page, and strace also doesn't know this).
> > > However, this trips up some checks that were always fine before
> > > (because glibc always passed NULL, and musl still does):
> > >
> > > if (offset_in_page(new_addr))
> > > if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len)
> > > if (addr + old_len > new_addr && new_addr + new_len > addr)
> > >
> > > ^^ These all look at the address without looking at MREMAP_FIXED, and
> > > return -EINVAL if they fail.
> > >
> > > So, test_seal_mremap_move_dontunmap_anyaddr passes 0xdeadbeef For Some
> > > Reason (why are you testing mremap in mseal_test.c??), it trips up
> > > offset_in_page(new_addr) in mremap_to, and we crash and burn.
> > >
> > > As to why no one else could repro this: I guess you're not running a
> > > glibc new enough ;)
> > >
> > That makes sense, mystery resolved.
> >
> > I added sys_ functions for mmap/munmap/mprotect, etc, so that the test
> > does not depend on libc, but I didn't do that for mremap, I think the
> > fix will be to add sys_mremap as well.
>
> I disagree, I don't understand why you're doing this test.
The test is for testing the return error code should be EPERM in case
of sealing.

> And even if
> you are rightfully doing the test, the test is wrong (and
> mremap_dontunmap.c tests agree, and always pass 0 as new_address).
the new_address don't need to be 0.
E.g.
mremap(ptr, size, size, MREMAP_MAYMOVE | MREMAP_DONTUNMAP, 0xdead0000);
Will success and relocate memory to 0xdead0000


> The manpage needs to be updated to reflect this, and this test either
> needs the 0xdeadbeef removed, or the whole thing.
>
> Adding a sys_mremap wrapper is inconsequential here, because you'll
> need to decide whether to pick up new_address from the flags argument
> and, if you do, it'll fail with the same error, but for everyone.
>
I will send a patch and you can try on your sys (since I don't have
the new glibc installed)

> --
> Pedro