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

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

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

But the v2 seems fine,
Liam