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

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

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

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
>

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.

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

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
>

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

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

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

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