Re: [PATCH hotfix 6.12 v2] mm/mlock: set the correct prev on failure

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

* Wei Yang <richard.weiyang@gmail.com> [241027 08:34]:
> After commit 94d7d9233951 ("mm: abstract the vma_merge()/split_vma()
> pattern for mprotect() et al."), if vma_modify_flags() return error, the
> vma is set to an error code. This will lead to an invalid prev be
> returned.
> 
> Generally this shouldn't matter as the caller should treat an error as
> indicating state is now invalidated, however unfortunately
> apply_mlockall_flags() does not check for errors and assumes that
> mlock_fixup() correctly maintains prev even if an error were to occur.
> 
> This patch fixes that assumption.
> 
> [lorenzo: provide a better fix and rephrase the log]
> 
> Fixes: 94d7d9233951 ("mm: abstract the vma_merge()/split_vma() pattern for mprotect() et al.")
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> CC: Liam R. Howlett <Liam.Howlett@Oracle.com>
> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> CC: Vlastimil Babka <vbabka@suse.cz>
> CC: Jann Horn <jannh@google.com>
> Cc: <stable@vger.kernel.org>
> 
> ---
> v2: 
>    rearrange the fix and change log per Lorenzo's suggestion
>    add fix tag and cc stable
> 
> ---
>  mm/mlock.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/mlock.c b/mm/mlock.c
> index e3e3dc2b2956..cde076fa7d5e 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -725,14 +725,17 @@ static int apply_mlockall_flags(int flags)
>  	}
>  
>  	for_each_vma(vmi, vma) {
> +		int error;
>  		vm_flags_t newflags;
>  
>  		newflags = vma->vm_flags & ~VM_LOCKED_MASK;
>  		newflags |= to_add;
>  
> -		/* Ignore errors */
> -		mlock_fixup(&vmi, vma, &prev, vma->vm_start, vma->vm_end,
> -			    newflags);
> +		error = mlock_fixup(&vmi, vma, &prev, vma->vm_start, vma->vm_end,
> +				    newflags);
> +		/* Ignore errors, but prev needs fixing up. */
> +		if (error)
> +			prev = vma;

I don't think we need a local variable for the error since it's not used
for anything besides ensuring there was a non-zero return here, but it
probably doesn't make a difference.  I'd have to check the assembly to
be sure.

Either way,

Reviewed-by: Liam R. Howlett <Liam.Howlett@Oracle.com>

>  		cond_resched();
>  	}
>  out:
> -- 
> 2.34.1
>