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

[Wei Yang <richard.weiyang@gmail.com>]

On Sun, Oct 27, 2024 at 11:41:13AM +0000, Lorenzo Stoakes wrote:
>+ 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!
>

Sure, will add them in later change.

>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.
>

Yes.

>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.
>

Will follow this.

>>
>> 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.
>

I have to say this one look better.

Thanks

>
>>
>>  	/*
>>  	 * 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!

-- 
Wei Yang
Help you, Help me