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@xxxxxxxxxxx>
> 
> 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@xxxxxxxxxxx>
> 
> ---
> 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