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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

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