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

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

[Zach O'Keefe]

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.

Fixes: 4d24de9425f7 ("mm: MADV_COLLAPSE: refetch vm_end after reacquiring mmap_lock")
Reported-by: Hugh Dickins <hughd@google.com>
Signed-off-by: Zach O'Keefe <zokeefe@google.com>
Cc: Yang Shi <shy828301@gmail.com>
---
 mm/khugepaged.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 5cb401aa2b9d..b4d2ec0a94ed 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -2649,7 +2649,7 @@ int madvise_collapse(struct vm_area_struct *vma, struct vm_area_struct **prev,
 				goto out_nolock;
 			}
 
-			hend = vma->vm_end & HPAGE_PMD_MASK;
+			hend = min(hend, vma->vm_end & HPAGE_PMD_MASK);
 		}
 		mmap_assert_locked(mm);
 		memset(cc->node_load, 0, sizeof(cc->node_load));
-- 
2.39.0.314.g84b9a713c41-goog

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

[Andrew Morton]

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?

> 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?  

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

[Hugh Dickins]

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

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

[Zach O'Keefe]

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

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

[Andrew Morton]

On Thu, 22 Dec 2022 17:33:00 -0800 "Zach O'Keefe" <zokeefe@google.com> wrote:

> Andrew -- I should have clarified this question in the description --
> apologies there.

OK.  Could you please send along revised changelogs which make
clearer our reasons for backporting?

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

[Zach O'Keefe]

[-- Attachment #1: Type: text/plain, Size: 441 bytes --]

On Fri, Dec 23, 2022 at 10:06 AM Andrew Morton <akpm@linux-foundation.org>
wrote:

> On Thu, 22 Dec 2022 17:33:00 -0800 "Zach O'Keefe" <zokeefe@google.com>
> wrote:
>
> > Andrew -- I should have clarified this question in the description --
> > apologies there.
>
> OK.  Could you please send along revised changelogs which make
> clearer our reasons for backporting?



Yep! Sounds good — will do this today. Thanks Andrew

[-- Attachment #2: Type: text/html, Size: 928 bytes --]