Re: [PATCH] mm/vma: Return the exact errno for __split_vma() and mas_store_gfp()

[Liam R. Howlett]

* Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> [240909 05:09]:
> On Mon, Sep 09, 2024 at 02:02:26PM GMT, Xiao Yang wrote:
> > __split_vma() and mas_store_gfp() returns several types of errno on
> > failure so don't ignore them in vms_gather_munmap_vmas(). For example,
> > __split_vma() returns -EINVAL when an unaligned huge page is unmapped.
> > This issue is reproduced by ltp memfd_create03 test.
> 
> Thanks for this! :)
> 
> Though pedantic note - please ensure to check scripts/get_maintainer.pl and cc-
> the reviewers and maintainer, the maintainer being Andrew and the
> reviewers being me, Liam and Vlastimil.
> 
> The maintainer is especially important as it's Andrew who'll take the patch
> ;)
> 
> I've cc'd them here :)
> 
> >
> > Fixes: 6898c9039bc8 ("mm/vma: extract the gathering of vmas from do_vmi_align_munmap()")

This fixes line will mean nothing in the long run, but Andrew can use it
to identify the target to squash things.

If this patch is merged and not squshed, you will create more work for
stable and get emails asking what commit it fixes.

> > Signed-off-by: Xiao Yang <ice_yangxiao@xxxxxxx>
> > Reported-by: kernel test robot <oliver.sang@xxxxxxxxx>
> > Closes: https://lore.kernel.org/oe-lkp/202409081536.d283a0fb-oliver.sang@xxxxxxxxx
> > ---
> >  mm/vma.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/vma.c b/mm/vma.c
> > index 8d1686fc8d5a..3feeea9a8c3d 100644
> > --- a/mm/vma.c
> > +++ b/mm/vma.c
> > @@ -1200,7 +1200,8 @@ int vms_gather_munmap_vmas(struct vma_munmap_struct *vms,
> >  			goto start_split_failed;
> >  		}
> >
> > -		if (__split_vma(vms->vmi, vms->vma, vms->start, 1))
> > +		error = __split_vma(vms->vmi, vms->vma, vms->start, 1);
> > +		if (error)
> >  			goto start_split_failed;
> 
> We'd probably want to stop assigning error = ENOMEM and just leave it
> uninitialised if we're always going to assign it rather than filter.
> 
> You'd want to make sure that you caught any case that relies on it being
> pre-assigned though.
> 
> >  	}
> >  	vms->prev = vma_prev(vms->vmi);
> > @@ -1220,12 +1221,14 @@ int vms_gather_munmap_vmas(struct vma_munmap_struct *vms,
> >  		}
> >  		/* Does it split the end? */
> >  		if (next->vm_end > vms->end) {
> > -			if (__split_vma(vms->vmi, next, vms->end, 0))
> > +			error = __split_vma(vms->vmi, next, vms->end, 0);
> > +			if (error)
> >  				goto end_split_failed;
> 
> Related to point above, In this and above, you are now resetting error to 0
> should this succeed while some later code might rely on this not being the
> case.
> 
> Basically I'd prefer us, if Liam is cool with it, to just not initialise
> error and assign when an error actually occurs.
> 
> But we filtered for a reason, need to figure out if that is still
> needed...
> m
> >  		}
> >  		vma_start_write(next);
> >  		mas_set(mas_detach, vms->vma_count++);
> > -		if (mas_store_gfp(mas_detach, next, GFP_KERNEL))
> > +		error = mas_store_gfp(mas_detach, next, GFP_KERNEL);
> > +		if (error)
> >  			goto munmap_gather_failed;
> >
> >  		vma_mark_detached(next, true);
> > --
> > 2.46.0
> >
> 
> I'm in general in favour of what this patch does (modulo the points about
> not initialising error and checking that we don't rely on it being
> initialised above), but it very much need's Liam's input.
> 
> If Liam is cool with it, I'll add tags, but let's hold off on this until we
> have confirmation from him.

We should probably drop the assignment all together.

Thanks,
Liam