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