Re: [PATCH v2] mm/khugepaged: avoid underflow in madvise_collapse for sub-PMD MADV_COLLAPSE

[Wandun <chenwandun1@gmail.com>]

On 5/11/26 15:17, David Hildenbrand (Arm) wrote:
> On 5/11/26 08:57, Wandun Chen wrote:
>> From: Chen Wandun <chenwandun@lixiang.com>
>>
>> madvise_collapse() computes the THP-aligned window:
>>
>>      hstart = ALIGN(start, HPAGE_PMD_SIZE); 	/* round up */
>>      hend = ALIGN_DOWN(end, HPAGE_PMD_SIZE);	/* round down */
>>
>> The following case will cause hstart > hend, and result in underflow
>> in the return statement, avoid it by returning -EINVAL early when
>> hstart > hend.
>>
>>      madvise(PMD-aligned + PAGE_SIZE, PAGE_SIZE, MADV_COLLAPSE);
> Ok, so providing a PMD-aligned address as start will result in 0 and a
> non-aligned address will result in -EINVAL.
>
> Didn't Lorenzo agree that just returning 0 in both cases would be clearer? But I
> might have misunderstood it.
Lorenzo suggested retuern -EINVAL for both case at the beginning,
Later, Lorenzo add an correction, suggested should return 0 for
compatibilty reasons for hstart == hend case.
(If I haven't missed any information)

Best regards,
Wandun
>
> That would also make the code easier ...
>
>> In addition, kmalloc_obj(), mmgrab() and lru_add_drain_all() are
>> unnecessary when hstart == hend, so skip these operations by
>> returning early too.
>>
>> Signed-off-by: Chen Wandun <chenwandun@lixiang.com>
>>
>> ---
>> v1 --> v2:
>> - Rebase and resolve code conflict.
>> - Return -EINVAL when hstart > hend, suggested by Lorenzo.
>> - Drop Fixes tag, suggested by David and Lorenzo.
>> - Updated commit message to be more explicit, suggested by Lorenzo.
>> ---
>>   mm/khugepaged.c | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index 28a843f30b32..36baab17f098 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -2837,6 +2837,15 @@ int madvise_collapse(struct vm_area_struct *vma, unsigned long start,
>>   	if (!thp_vma_allowable_order(vma, vma->vm_flags, TVA_FORCED_COLLAPSE, PMD_ORDER))
>>   		return -EINVAL;
>>   
>> +	hstart = ALIGN(start, HPAGE_PMD_SIZE);
>> +	hend = ALIGN_DOWN(end, HPAGE_PMD_SIZE);
>> +
>> +	if (hstart > hend)
>> +		return -EINVAL;
>> +
>> +	if (hstart == hend)
>> +		return 0;
>> +
> ... as it would simply be:
>
> /* Nothing to collapse. */
> if (hstart >= hend)
> 	return 0;
> @Lorenzo, or do we want to keep the (questionable : ) ) existing behavior?
>