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

["Zach O'Keefe" <zokeefe@google.com>]

On Thu, Dec 22, 2022 at 5:25 PM Hugh Dickins <hughd@google.com> wrote:
>
> 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.

Thanks Hugh,

Andrew -- I should have clarified this question in the description --
apologies there. As Hugh mentions, I don't believe there is a kernel
stability concern here as we always (re)validate the VMA / region
accordingly. Also as Hugh mentions, the user-visible effects are: we
try to collapse more memory than requested by the user, and/or failing
an operation that should have otherwise succeeded. An example is
trying to collapse a 4MiB file contained within a 12MiB VMA.

> > 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?
> >
> > > 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. Yes, I'm planning to backport these both to 6.1 stable
after they were deemed acceptable.

> Thanks,
> Hugh