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

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

+ Vlastimil, Liam, Jann as this is VMA-related.

We really need to bring all VMA-ish files under the VMA MAINTAINERS
block... will maybe address that once things around that file... calm down
a bit.

But please cc all of us on anything that even vaguely relates to VMAs,
thanks!

On Sun, Oct 27, 2024 at 02:56:29AM +0000, Wei Yang wrote:
> 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.

This is a great spot, but this commit message is missing critical
details. This is only meaningful for apply_mlockall_flags() which is both
ignoring errors AND assuming mlock_fixup(), even on error, is correctly
updating the prev state. Which is imo wrong.

So I'd _add_ a bit more information here like:

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.

We'll also need to backport this, so a:

Fixes: 94d7d9233951 ("mm: abstract the vma_merge()/split_vma() pattern for mprotect() et al.")
Cc: <stable@vger.kernel.org>

Needs to be added, and make the next revision [PATCH hotfix 6.12 v2] to
make it clear this needs to go to 6.12.

>
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
>  mm/mlock.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/mm/mlock.c b/mm/mlock.c
> index e3e3dc2b2956..8c3f9cf8f960 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -478,11 +478,12 @@ static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  		/* don't set VM_LOCKED or VM_LOCKONFAULT and don't count */
>  		goto out;
>
> -	vma = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
> -	if (IS_ERR(vma)) {
> -		ret = PTR_ERR(vma);
> +	*prev = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
> +	if (IS_ERR(*prev)) {
> +		ret = PTR_ERR(*prev);
>  		goto out;
>  	}
> +	vma = *prev;

Yeah sorry I hate this, it was icky before and it's kinda disgusting to
assign *prev then *prev to vma, then, if success, vma to *prev - yeah
that's super confusing :)

I mean a better alternative if you were to do this approach would be to
have a new vma local but I don't actually think that's the correct
approach.

Really the caller _must_ deal with errors, and not assume any state is
valid after an error occurs.

So I think the fix should be in apply_mlockall_flags() instead like:

	...

	for_each_vma(vmi, vma) {
		...
		int error;

		...

		error = mlock_fixup(&vmi, vma, &prev, vma->vm_start, vma->vm_end,
				    newflags);
		/* Ignore errors, but prev needs fixing up. */
		if (error)
			prev = vma;

		...
	}

This is also a smaller delta for backporting.


>
>  	/*
>  	 * Keep track of amount of locked VM.
> --
> 2.34.1
>
>

I'm happy for you to resubmit like this and take full credit by the way! :)
assuming you agree with this approach.

This is also reminding me that I need to refactor all this crap, the whole
passing prev around and looping like that is horrible. Also the outer loop
should be maintaining prev, not the inner one.

This is going on my TODO list!