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

["Liam R. Howlett" <Liam.Howlett@oracle.com>]

* 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