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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

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()")
> Signed-off-by: Xiao Yang <ice_yangxiao@163.com>
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202409081536.d283a0fb-oliver.sang@intel.com
> ---
>  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.

Thanks!