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

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

On Mon, Oct 28, 2024 at 11:00:43AM -0400, Liam R. Howlett wrote:
>* 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>
>

Thanks

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

-- 
Wei Yang
Help you, Help me