On Fri, May 08, 2026 at 02:27:37PM +0200, David Hildenbrand (Arm) wrote:
> On 5/7/26 09:05, Chen Wandun wrote:
> > madvise_collapse() computes the THP-aligned window:
> >
> > hstart = (start + ~HPAGE_PMD_MASK) & HPAGE_PMD_MASK /* round up */
> > hend = end & HPAGE_PMD_MASK /* round down */
> >
> > Previously this was done after kmalloc_obj(), so problem arose when
> > the range contained no complete PMD-aligned window (hstart >= hend).
> >
> > When hstart > hend, (hend - hstart) wraps unsigned to a huge value, the
> > final comparison fails and -EINVAL is returned instead of 0. Consider
I think both should return -EINVAL.
> > two single-page calls on a 2 MiB-aligned address:
> >
> > /* hstart == hend == aligned -> 0 == 0 -> returns 0 */
> > madvise(aligned, PAGE_SIZE, MADV_COLLAPSE);
What's aligned? You're putting a random variable name in there? Presumably a
PMD-aligned address?
> >
> > /* hstart = aligned + 2MiB, hend = aligned
> > * (hend - hstart) wraps unsigned -> returns -EINVAL */
> > madvise(aligned + PAGE_SIZE, PAGE_SIZE, MADV_COLLAPSE);
> >
> > Both calls cover less than one THP and collapse nothing; both should
> > return 0.
Disagree.
>
> Okay, so we talk about a "userspace is being stupid" scenario.
Yes!
I feel that -EINVAL is correct for hend > hstart, and I think it might even be a
userland A[BP]I break to change it (maybe somebody, somewhere is being foolish
enough to use this to also validate input ranges).
The weirdness is when hstart == hend being 0 but that's sort of established
behaviour I guess.
>
> >
> > In addition, kmalloc_obj(), mmgrab() and lru_add_drain_all() were all
> > called before discovering there was nothing to do, only for the code
> > to kfree() and return immediately after.
>
> Just a comment as you motivate here why this is suboptimal: we do not care
> about
> a "userspace is being stupid" scenario being fast.
Yes, in general - so what? The user is doing stupid things, so the user wins
stupid prizes?
>
> >
> > Fix both by computing hstart/hend after thp_vma_allowable_order() but
> > before kmalloc_obj(), and returning 0 early when hstart >= hend.
> >
> > Fixes: 7d8faaf15545 ("mm/madvise: introduce MADV_COLLAPSE sync hugepage
> > collapse")
>
> Fixes: is likely ok, but I don't think we want to treat this as a hotfix or CC
> stable.
I'm not sure I want a fixes here, this isn't really fixing anything. This isn't
a bug afaik, it's just us not handling this brilliantly, but (possibly by
mistake) getting the right output.
>
> > Signed-off-by: Chen Wandun <[email protected]>
I put this patch through AI detection and it's telling me there's an 80% chance
this whole thing is LLM-generated, which is making me grumpy.
Can you confirm that this is, in fact, your own work? Plagiarism is not a nice
thing to do, and THP doesn't need more traffic, we're overloaded as it is.
> > ---
> > mm/khugepaged.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index b8452dbdb043..92473d93e837 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -2836,6 +2836,12 @@ 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 = (start + ~HPAGE_PMD_MASK) & HPAGE_PMD_MASK;
> > + hend = end & HPAGE_PMD_MASK;
See below re: conflict.
> > +
> > + if (hstart >= hend)
> > + return 0;
if (hstart > hend)
return -EINVAL;
/* For compatibility, users may rely on this. */
if (hstart == hend)
return 0;
Is probably better.
But I'm not sure what the point is if we're already doing this behaviour?
> > +
> > cc = kmalloc_obj(*cc);
> > if (!cc)
> > return -ENOMEM;
> > @@ -2845,9 +2851,6 @@ int madvise_collapse(struct vm_area_struct *vma,
> > unsigned long start,
> > mmgrab(mm);
> > lru_add_drain_all();
> >
> > - hstart = (start + ~HPAGE_PMD_MASK) & HPAGE_PMD_MASK;
> > - hend = end & HPAGE_PMD_MASK;
> > -
> > for (addr = hstart; addr < hend; addr += HPAGE_PMD_SIZE) {
> > enum scan_result result = SCAN_FAIL;
> >
>
> In general, LGTM, but see for conflict:
> https://lore.kernel.org/all/[email protected]/
Please use mm-unstable as a basis for your mm work Chen, this is something you
need to fix, the patch above has been around for a while and is in
mm-unstable.
You have patches in mm already so you should know better by now.
But I'm really not sure I'm in favour of this anyway. I'll defer to David but
this feels useless to me.
>
>
> --
> Cheers,
>
> David
Thanks, Lorenzo
