Re: [PATCH] mm/MADV_COLLAPSE: don't expand collapse when vm_end is past requested end

[Hugh Dickins <hughd@google.com>]

On Thu, 22 Dec 2022, Andrew Morton wrote:
> On Thu, 22 Dec 2022 16:39:53 -0800 "Zach O'Keefe" <zokeefe@google.com> wrote:
> 
> > MADV_COLLAPSE acts on one hugepage-aligned/sized region at a time, until
> > it has collapsed all eligible memory contained within the bounds
> > supplied by the user.
> > 
> > At the top of each hugepage iteration we (re)lock mmap_lock and
> > (re)validate the VMA for eligibility and update variables that might
> > have changed while mmap_lock was dropped.  One thing that might occur,
> > is that the VMA could be resized, and as such, we refetch vma->vm_end
> > to make sure we don't collapse past the end of the VMA.
> > 
> > However, it's possible that during this refetch that we expand the
> > region acted on by MADV_COLLAPSE if vma->vm_end is greater than the end
> > of the user-supplied range.
> > 
> > Don't expand the acted-on region when refetching vma->vm_end.
> 
> What are the user-visible effects of this?

Not any kernel crash, I think; but in my case (I was trying to check
something else about MADV_COLLAPSE, and so was first verifying that
it worked in the simple case) I kept getting EINVAL back from it,
even when I'd fixed all my own userspace mistakes.

It turned out to be that my mmap was bigger than the file itself, and
I was only trying to collapse the file length; but because of the
mis-adjustment to vm_end, it ran off the end of file and got into
EINVAL territory (in a different context, would be EFAULT or SIGBUS).

So in my case, unexpected failure.  But I guess another case would be
too much success: I suppose that if you try to collapse the first 2M
of a 2T file, the mis-adjustment would cause it to spend a very long
time doing much more work than you asked for.

> 
> > Fixes: 4d24de9425f7 ("mm: MADV_COLLAPSE: refetch vm_end after reacquiring mmap_lock")
> 
> Should we backport "mm/shmem: restore SHMEM_HUGE_DENY precedence over
> MADV_COLLAPSE" and/or this patch into 6.1.x?  

Yes, please do Cc stable for them both in 6.1.x: I only just now realized
the nasty "too much success" possibility, which does seem well worth stable;
and I'd particularly like the precedence of SHMEM_HUGE_DENY asserted in
6.1.x, because doing it later it would become a UAPI change - I'm sorry
I didn't catch it sooner, Zach did ask me to check but I was head down
on other things.

Thanks,
Hugh