At 2024-09-09 17:09:43, "Lorenzo Stoakes" <lorenzo.stoakes@oracle.com> wrote:
>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.


Hi Lorenzo,


Thanks for your kind reminder.

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


Agreed. I will resend the v2 patch as you suggested.


Best Regards,
Xiao Yang

>
>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!