| Message ID | 20221223003953.2795313-1-zokeefe@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/MADV_COLLAPSE: don't expand collapse when vm_end is past requested end | expand |
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?
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
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
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?
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
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));
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(-)