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

[Wandun]

On 5/12/26 00:21, Lorenzo Stoakes wrote:
> On Mon, May 11, 2026 at 10:01:39AM +0200, David Hildenbrand (Arm) wrote:
> > 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@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)
> > Let's wait for Lorenzo's confirmation.
> :) thanks.
>
> See below but TL;DR I convinced myself that actually, I agree with David...
>
> I hadn't really examined the madvise() <-> madvise_collapse() logic closely
> enough but yeah. Return 0 for both cases.
>
> > 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.
> Right so madvise_vma_behavior() should be called with range->[start, end] tied
> to the VMA (under VMA lock we assert this also).
>
> So what we're really talking about is hstart, hend.
>
> Really we should NEVER have aligned the addresses for the user, that was the
> real mistake here. But that ship has sailed...
>
> Since we do:
>
> hstart = ALIGN(start, HPAGE_PMD_SIZE);
> hend = ALIGN_DOWN(end, HPAGE_PMD_SIZE);
>
> That means e.g.
>
> start = <some PMD aligned addr> + 1
> end = start + <not enough to get it to the next hpage>
>
> Results in hstart > hend, so the user must have given incorrect input.
>
> hstart == hend would be e.g.:
>
> start = <some PMD aligned addr> + 1
> end = start + HPAGE_PMD_SIZE
>
> Which is still an invalid input. I honestly wish we just required that the user
> provided PMD aligned ranges and we didn't align for them, it's stupid that we
> do.
>
> So the real question is, what constitutes an actual invalid input here? We've
> made a mess and now we have to decide how we interpret it... :)
>
> I still feel that hstart > hend is an error. Since we are aligning things the
> stupid way we do, we are treating start, end as _bounds_. So we are saying 'turn
> everything in the bounded range [start, end) into huge pages'.
>
> So we use ALIGN() on start so nothing BEFORE start gets converted. And we use
> ALIGN_DOWN() on end so nothing AFTER end gets converted.
>
> But if you do:
>
> (first case above)
>
> PMD aligned                  PMD aligned
>      |  <----------------->      |
>
> You're kinda doing something stupid obviously, because that range cannot span
> any huge pages, you don't even cross a boundary, and importantly - your range
> _isn't even large enough to include a single page_.
>
> With:
>
> PMD aligned                  PMD aligned
>      |  <------------------------|->
>
> You are crossing a boundary but not enough to get a page. But you might well
> have a large enough range to span a single page...
>
> But OTOH...
>
> PMD aligned                  PMD aligned
>      |                       <---|->
>
> Would get you the same as is equally silly.
>
> Yeah ok this is a long way round of coming to the same conclusion as David,
> godamnit, and I so wanted to disagree here :P
>
> Since both get you nothing, and the input was valid _to madvise()_ let's just
> return 0 for both cases.
Thanks for taking the time to walk through this, Lorenzo and David.

Now we've agreed both cases should consistently return 0, I'll send
a v3 that simply bails out with 0 when hstart >= hend.

Best regards,
Wandun
> > --
> > Cheers,
> >
> > David
> Cheers, Lorenzo