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

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

[Wandun Chen]

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);

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;
+
 	cc = kmalloc_obj(*cc);
 	if (!cc)
 		return -ENOMEM;
@@ -2846,9 +2855,6 @@ int madvise_collapse(struct vm_area_struct *vma, unsigned long start,
 	mmgrab(mm);
 	lru_add_drain_all();
 
-	hstart = ALIGN(start, HPAGE_PMD_SIZE);
-	hend = ALIGN_DOWN(end, HPAGE_PMD_SIZE);
-
 	for (addr = hstart; addr < hend; addr += HPAGE_PMD_SIZE) {
 		enum scan_result result = SCAN_FAIL;
 
-- 
2.43.0

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

[David Hildenbrand (Arm)]

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.

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?

-- 
Cheers,

David

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

[Wandun]

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

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

[David Hildenbrand (Arm)]

On 5/11/26 09:35, Wandun wrote:
> 
> 
> 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)

Let's wait for Lorenzo's confirmation.

I think the important part is that we cannot have a situation where start < end
(given that madvise() consumes a length). Because, there we really should have
returned -EINVAL.

For start <= end, if there is nothing suitable to collapse, I'd say we'd just
consistently return 0.

-- 
Cheers,

David