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

[Wandun <chenwandun1@gmail.com>]

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@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.
> :) 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