The logic can be simplified - firstly by renaming the inconsistently named
apply_mm_seal() to mseal_apply().
We then wrap mseal_fixup() into the main loop as the logic is simple enough
to not require it, equally it isn't a hugely pleasant pattern in mprotect()
etc. so it's not something we want to perpetuate.
We remove some redundant comments, and then avoid the entirely unnecessary
and slightly bizarre invocation of vma_iter_end() on each loop - really
what we want, given we have asserted there are no gaps in the range - is to
handle start, end being offset into a VMAs. This is easily handled with
MIN()/MAX().
There's no need to have an 'out' label block since on vma_modify_flags()
error we abort anyway.
And by refactoring like this we avoid the rather horrid 'pass pointer to
prev around' pattern used in mprotect() et al.
No functional change intended.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Reviewed-by: Pedro Falcato <pfalcato@suse.de>
---
mm/mseal.c | 69 +++++++++++++++++-------------------------------------
1 file changed, 22 insertions(+), 47 deletions(-)
diff --git a/mm/mseal.c b/mm/mseal.c
index 794d1043a706..e0fe37623632 100644
--- a/mm/mseal.c
+++ b/mm/mseal.c
@@ -15,60 +15,35 @@
#include <linux/sched.h>
#include "internal.h"
-static int mseal_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
- struct vm_area_struct **prev, unsigned long start,
- unsigned long end, vm_flags_t newflags)
-{
- int ret = 0;
- vm_flags_t oldflags = vma->vm_flags;
-
- if (newflags == oldflags)
- goto out;
-
- vma = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
- if (IS_ERR(vma)) {
- ret = PTR_ERR(vma);
- goto out;
- }
-
- vm_flags_set(vma, VM_SEALED);
-out:
- *prev = vma;
- return ret;
-}
-
-/*
- * Apply sealing.
- */
-static int apply_mm_seal(unsigned long start, unsigned long end)
+static int mseal_apply(struct mm_struct *mm,
+ unsigned long start, unsigned long end)
{
- unsigned long nstart;
struct vm_area_struct *vma, *prev;
- VMA_ITERATOR(vmi, current->mm, start);
+ VMA_ITERATOR(vmi, mm, start);
+ /* We know there are no gaps so this will be non-NULL. */
vma = vma_iter_load(&vmi);
- /*
- * Note: check_mm_seal should already checked ENOMEM case.
- * so vma should not be null, same for the other ENOMEM cases.
- */
prev = vma_prev(&vmi);
if (start > vma->vm_start)
prev = vma;
- nstart = start;
for_each_vma_range(vmi, vma, end) {
- int error;
- unsigned long tmp;
- vm_flags_t newflags;
-
- newflags = vma->vm_flags | VM_SEALED;
- tmp = vma->vm_end;
- if (tmp > end)
- tmp = end;
- error = mseal_fixup(&vmi, vma, &prev, nstart, tmp, newflags);
- if (error)
- return error;
- nstart = vma_iter_end(&vmi);
+ unsigned long curr_start, curr_end;
+
+ if (vma->vm_flags & VM_SEALED) {
+ prev = vma;
+ continue;
+ }
+ curr_start = MAX(start, vma->vm_start);
+ curr_end = MIN(vma->vm_end, end);
+
+ vma = vma_modify_flags(&vmi, prev, vma, curr_start, curr_end,
+ vma->vm_flags | VM_SEALED);
+ if (IS_ERR(vma))
+ return PTR_ERR(vma);
+ vm_flags_set(vma, VM_SEALED);
+
+ prev = vma;
}
return 0;
@@ -185,10 +160,10 @@ int do_mseal(unsigned long start, size_t len_in, unsigned long flags)
* reaching the max supported VMAs, however, those cases shall
* be rare.
*/
- ret = apply_mm_seal(start, end);
+ ret = mseal_apply(mm, start, end);
out:
- mmap_write_unlock(current->mm);
+ mmap_write_unlock(mm);
return ret;
}
--
2.50.1On 15.07.25 15:37, Lorenzo Stoakes wrote: > The logic can be simplified - firstly by renaming the inconsistently named > apply_mm_seal() to mseal_apply(). > > We then wrap mseal_fixup() into the main loop as the logic is simple enough > to not require it, equally it isn't a hugely pleasant pattern in mprotect() > etc. so it's not something we want to perpetuate. > > We remove some redundant comments, and then avoid the entirely unnecessary > and slightly bizarre invocation of vma_iter_end() on each loop - really > what we want, given we have asserted there are no gaps in the range - is to > handle start, end being offset into a VMAs. This is easily handled with > MIN()/MAX(). > > There's no need to have an 'out' label block since on vma_modify_flags() > error we abort anyway. > > And by refactoring like this we avoid the rather horrid 'pass pointer to > prev around' pattern used in mprotect() et al. > > No functional change intended. > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > Reviewed-by: Pedro Falcato <pfalcato@suse.de> > --- Acked-by: David Hildenbrand <david@redhat.com> -- Cheers, David / dhildenb
* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250715 09:37]:
> The logic can be simplified - firstly by renaming the inconsistently named
> apply_mm_seal() to mseal_apply().
>
> We then wrap mseal_fixup() into the main loop as the logic is simple enough
> to not require it, equally it isn't a hugely pleasant pattern in mprotect()
> etc. so it's not something we want to perpetuate.
>
> We remove some redundant comments, and then avoid the entirely unnecessary
> and slightly bizarre invocation of vma_iter_end() on each loop - really
> what we want, given we have asserted there are no gaps in the range - is to
> handle start, end being offset into a VMAs. This is easily handled with
> MIN()/MAX().
The vma_iter_end() was to detect a merge with the next vma and use the
new end, since the code you are replacing didn't update the vma pointer.
>
> There's no need to have an 'out' label block since on vma_modify_flags()
> error we abort anyway.
Ah, in the mseal_fixup()/mseal_apply(). The out label still exists (and
appears in the below patch) in do_mseal().
>
> And by refactoring like this we avoid the rather horrid 'pass pointer to
> prev around' pattern used in mprotect() et al.
>
> No functional change intended.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Reviewed-by: Pedro Falcato <pfalcato@suse.de>
> ---
> mm/mseal.c | 69 +++++++++++++++++-------------------------------------
> 1 file changed, 22 insertions(+), 47 deletions(-)
>
> diff --git a/mm/mseal.c b/mm/mseal.c
> index 794d1043a706..e0fe37623632 100644
> --- a/mm/mseal.c
> +++ b/mm/mseal.c
> @@ -15,60 +15,35 @@
> #include <linux/sched.h>
> #include "internal.h"
>
> -static int mseal_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
> - struct vm_area_struct **prev, unsigned long start,
> - unsigned long end, vm_flags_t newflags)
> -{
> - int ret = 0;
> - vm_flags_t oldflags = vma->vm_flags;
> -
> - if (newflags == oldflags)
> - goto out;
> -
> - vma = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
> - if (IS_ERR(vma)) {
> - ret = PTR_ERR(vma);
> - goto out;
> - }
> -
> - vm_flags_set(vma, VM_SEALED);
> -out:
> - *prev = vma;
> - return ret;
> -}
> -
> -/*
> - * Apply sealing.
> - */
> -static int apply_mm_seal(unsigned long start, unsigned long end)
> +static int mseal_apply(struct mm_struct *mm,
> + unsigned long start, unsigned long end)
> {
> - unsigned long nstart;
> struct vm_area_struct *vma, *prev;
> - VMA_ITERATOR(vmi, current->mm, start);
> + VMA_ITERATOR(vmi, mm, start);
>
> + /* We know there are no gaps so this will be non-NULL. */
> vma = vma_iter_load(&vmi);
> - /*
> - * Note: check_mm_seal should already checked ENOMEM case.
> - * so vma should not be null, same for the other ENOMEM cases.
> - */
> prev = vma_prev(&vmi);
> if (start > vma->vm_start)
> prev = vma;
>
> - nstart = start;
> for_each_vma_range(vmi, vma, end) {
> - int error;
> - unsigned long tmp;
> - vm_flags_t newflags;
> -
> - newflags = vma->vm_flags | VM_SEALED;
> - tmp = vma->vm_end;
> - if (tmp > end)
> - tmp = end;
> - error = mseal_fixup(&vmi, vma, &prev, nstart, tmp, newflags);
> - if (error)
> - return error;
> - nstart = vma_iter_end(&vmi);
> + unsigned long curr_start, curr_end;
> +
> + if (vma->vm_flags & VM_SEALED) {
> + prev = vma;
> + continue;
> + }
> + curr_start = MAX(start, vma->vm_start);
> + curr_end = MIN(vma->vm_end, end);
If you assigned curr_start outside the loop, you could just set it to
vma->vm_end after you change the flags, this would reduce instructions
in the loop. But this is hardly a performance critical path.
And we probably hardly ever execute this loop more than once.
> +
> + vma = vma_modify_flags(&vmi, prev, vma, curr_start, curr_end,
> + vma->vm_flags | VM_SEALED);
> + if (IS_ERR(vma))
> + return PTR_ERR(vma);
> + vm_flags_set(vma, VM_SEALED);
> +
> + prev = vma;
> }
>
> return 0;
> @@ -185,10 +160,10 @@ int do_mseal(unsigned long start, size_t len_in, unsigned long flags)
> * reaching the max supported VMAs, however, those cases shall
> * be rare.
> */
> - ret = apply_mm_seal(start, end);
> + ret = mseal_apply(mm, start, end);
>
> out:
> - mmap_write_unlock(current->mm);
> + mmap_write_unlock(mm);
It is nice to see the local variable being used.
Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> return ret;
> }
>
> --
> 2.50.1
Sorry I actually replied to this yestreday, but then closed down emacs and
turned my PC off before I had sent it mistakenly thinking I had :P doh!
On Tue, Jul 15, 2025 at 12:09:10PM -0400, Liam R. Howlett wrote:
> * Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250715 09:37]:
> > The logic can be simplified - firstly by renaming the inconsistently named
> > apply_mm_seal() to mseal_apply().
> >
> > We then wrap mseal_fixup() into the main loop as the logic is simple enough
> > to not require it, equally it isn't a hugely pleasant pattern in mprotect()
> > etc. so it's not something we want to perpetuate.
> >
> > We remove some redundant comments, and then avoid the entirely unnecessary
> > and slightly bizarre invocation of vma_iter_end() on each loop - really
> > what we want, given we have asserted there are no gaps in the range - is to
> > handle start, end being offset into a VMAs. This is easily handled with
> > MIN()/MAX().
>
> The vma_iter_end() was to detect a merge with the next vma and use the
> new end, since the code you are replacing didn't update the vma pointer.
Ah interesting, good that we now no longer need to do this!
>
> >
> > There's no need to have an 'out' label block since on vma_modify_flags()
> > error we abort anyway.
>
> Ah, in the mseal_fixup()/mseal_apply(). The out label still exists (and
> appears in the below patch) in do_mseal().
Yeah indeed this is what I was referring to :)
>
>
> >
> > And by refactoring like this we avoid the rather horrid 'pass pointer to
> > prev around' pattern used in mprotect() et al.
> >
> > No functional change intended.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > Reviewed-by: Pedro Falcato <pfalcato@suse.de>
> > ---
> > mm/mseal.c | 69 +++++++++++++++++-------------------------------------
> > 1 file changed, 22 insertions(+), 47 deletions(-)
> >
> > diff --git a/mm/mseal.c b/mm/mseal.c
> > index 794d1043a706..e0fe37623632 100644
> > --- a/mm/mseal.c
> > +++ b/mm/mseal.c
> > @@ -15,60 +15,35 @@
> > #include <linux/sched.h>
> > #include "internal.h"
> >
> > -static int mseal_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
> > - struct vm_area_struct **prev, unsigned long start,
> > - unsigned long end, vm_flags_t newflags)
> > -{
> > - int ret = 0;
> > - vm_flags_t oldflags = vma->vm_flags;
> > -
> > - if (newflags == oldflags)
> > - goto out;
> > -
> > - vma = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
> > - if (IS_ERR(vma)) {
> > - ret = PTR_ERR(vma);
> > - goto out;
> > - }
> > -
> > - vm_flags_set(vma, VM_SEALED);
> > -out:
> > - *prev = vma;
> > - return ret;
> > -}
> > -
> > -/*
> > - * Apply sealing.
> > - */
> > -static int apply_mm_seal(unsigned long start, unsigned long end)
> > +static int mseal_apply(struct mm_struct *mm,
> > + unsigned long start, unsigned long end)
> > {
> > - unsigned long nstart;
> > struct vm_area_struct *vma, *prev;
> > - VMA_ITERATOR(vmi, current->mm, start);
> > + VMA_ITERATOR(vmi, mm, start);
> >
> > + /* We know there are no gaps so this will be non-NULL. */
> > vma = vma_iter_load(&vmi);
> > - /*
> > - * Note: check_mm_seal should already checked ENOMEM case.
> > - * so vma should not be null, same for the other ENOMEM cases.
> > - */
> > prev = vma_prev(&vmi);
> > if (start > vma->vm_start)
> > prev = vma;
> >
> > - nstart = start;
> > for_each_vma_range(vmi, vma, end) {
> > - int error;
> > - unsigned long tmp;
> > - vm_flags_t newflags;
> > -
> > - newflags = vma->vm_flags | VM_SEALED;
> > - tmp = vma->vm_end;
> > - if (tmp > end)
> > - tmp = end;
> > - error = mseal_fixup(&vmi, vma, &prev, nstart, tmp, newflags);
> > - if (error)
> > - return error;
> > - nstart = vma_iter_end(&vmi);
> > + unsigned long curr_start, curr_end;
> > +
> > + if (vma->vm_flags & VM_SEALED) {
> > + prev = vma;
> > + continue;
> > + }
> > + curr_start = MAX(start, vma->vm_start);
> > + curr_end = MIN(vma->vm_end, end);
>
> If you assigned curr_start outside the loop, you could just set it to
> vma->vm_end after you change the flags, this would reduce instructions
> in the loop. But this is hardly a performance critical path.
>
> And we probably hardly ever execute this loop more than once.
True, could do a curr_start = curr_end too which is sort of nicely
self-documenting as well.
I need to respin anyway based on other feedback so will do this!
>
> > +
> > + vma = vma_modify_flags(&vmi, prev, vma, curr_start, curr_end,
> > + vma->vm_flags | VM_SEALED);
> > + if (IS_ERR(vma))
> > + return PTR_ERR(vma);
> > + vm_flags_set(vma, VM_SEALED);
> > +
> > + prev = vma;
> > }
> >
> > return 0;
> > @@ -185,10 +160,10 @@ int do_mseal(unsigned long start, size_t len_in, unsigned long flags)
> > * reaching the max supported VMAs, however, those cases shall
> > * be rare.
> > */
> > - ret = apply_mm_seal(start, end);
> > + ret = mseal_apply(mm, start, end);
> >
> > out:
> > - mmap_write_unlock(current->mm);
> > + mmap_write_unlock(mm);
>
> It is nice to see the local variable being used.
:) yeah good to refactor that.
>
>
> Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>
Thanks!
>
> > return ret;
> > }
> >
> > --
> > 2.50.1
© 2016 - 2025 Red Hat, Inc.