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

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

On Mon, Oct 28, 2024 at 11:07:16AM -0400, Liam R. Howlett wrote:
>* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [241027 07:43]:
>> + 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;
>
>
>If we move the assignment *prev = vma to the start of this function,
>then we can just get rid of the "out:" label and return on errors.
>

Maybe not, if my understanding is correct.

  vma = vma_modify_flags(, vma, ...)
      vma_modify()
          merged = vma_merge_existing_range()
	  return merged

For example, if we merge left, the returned one would be the prev instead of
vma we passed in. Even we may release the original vma. 

>But the v2 seems fine,
>Liam

-- 
Wei Yang
Help you, Help me