* [PATCH v4] mm, hugetlb: fix racy resv_huge_pages underflow on UFFDIO_COPY
@ 2021-05-28 0:50 Mina Almasry
2021-05-31 23:25 ` Andrew Morton
2021-06-04 21:41 ` Mike Kravetz
0 siblings, 2 replies; 12+ messages in thread
From: Mina Almasry @ 2021-05-28 0:50 UTC (permalink / raw)
Cc: Mina Almasry, Axel Rasmussen, Peter Xu, linux-mm, Mike Kravetz,
Andrew Morton, linux-kernel
On UFFDIO_COPY, if we fail to copy the page contents while holding the
hugetlb_fault_mutex, we will drop the mutex and return to the caller
after allocating a page that consumed a reservation. In this case there
may be a fault that double consumes the reservation. To handle this, we
free the allocated page, fix the reservations, and allocate a temporary
hugetlb page and return that to the caller. When the caller does the
copy outside of the lock, we again check the cache, and allocate a page
consuming the reservation, and copy over the contents.
Test:
Hacked the code locally such that resv_huge_pages underflows produce
a warning and the copy_huge_page_from_user() always fails, then:
./tools/testing/selftests/vm/userfaultfd hugetlb_shared 10
2 /tmp/kokonut_test/huge/userfaultfd_test && echo test success
./tools/testing/selftests/vm/userfaultfd hugetlb 10
2 /tmp/kokonut_test/huge/userfaultfd_test && echo test success
Both tests succeed and produce no warnings. After the
test runs number of free/resv hugepages is correct.
Signed-off-by: Mina Almasry <almasrymina@google.com>
Cc: Axel Rasmussen <axelrasmussen@google.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: linux-mm@kvack.org
Cc: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
include/linux/migrate.h | 4 ++++
mm/hugetlb.c | 48 +++++++++++++++++++++++++++++++++--------
mm/migrate.c | 4 ++--
mm/userfaultfd.c | 48 +----------------------------------------
4 files changed, 46 insertions(+), 58 deletions(-)
diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index 4bb4e519e3f5..4164c9ddd86e 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -51,6 +51,7 @@ extern int migrate_huge_page_move_mapping(struct address_space *mapping,
struct page *newpage, struct page *page);
extern int migrate_page_move_mapping(struct address_space *mapping,
struct page *newpage, struct page *page, int extra_count);
+extern void migrate_copy_huge_page(struct page *dst, struct page *src);
#else
static inline void putback_movable_pages(struct list_head *l) {}
@@ -77,6 +78,9 @@ static inline int migrate_huge_page_move_mapping(struct address_space *mapping,
return -ENOSYS;
}
+static inline void migrate_copy_huge_page(struct page *dst, struct page *src)
+{
+}
#endif /* CONFIG_MIGRATION */
#ifdef CONFIG_COMPACTION
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 76e2a6efc165..6072c9f82794 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -30,6 +30,7 @@
#include <linux/numa.h>
#include <linux/llist.h>
#include <linux/cma.h>
+#include <linux/migrate.h>
#include <asm/page.h>
#include <asm/pgalloc.h>
@@ -4905,20 +4906,17 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
struct page **pagep)
{
bool is_continue = (mode == MCOPY_ATOMIC_CONTINUE);
- struct address_space *mapping;
- pgoff_t idx;
+ struct hstate *h = hstate_vma(dst_vma);
+ struct address_space *mapping = dst_vma->vm_file->f_mapping;
+ pgoff_t idx = vma_hugecache_offset(h, dst_vma, dst_addr);
unsigned long size;
int vm_shared = dst_vma->vm_flags & VM_SHARED;
- struct hstate *h = hstate_vma(dst_vma);
pte_t _dst_pte;
spinlock_t *ptl;
- int ret;
+ int ret = -ENOMEM;
struct page *page;
int writable;
- mapping = dst_vma->vm_file->f_mapping;
- idx = vma_hugecache_offset(h, dst_vma, dst_addr);
-
if (is_continue) {
ret = -EFAULT;
page = find_lock_page(mapping, idx);
@@ -4947,12 +4945,44 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
/* fallback to copy_from_user outside mmap_lock */
if (unlikely(ret)) {
ret = -ENOENT;
+ /* Free the allocated page which may have
+ * consumed a reservation.
+ */
+ restore_reserve_on_error(h, dst_vma, dst_addr, page);
+ put_page(page);
+
+ /* Allocate a temporary page to hold the copied
+ * contents.
+ */
+ page = alloc_huge_page_vma(h, dst_vma, dst_addr);
+ if (IS_ERR(page)) {
+ ret = -ENOMEM;
+ goto out;
+ }
*pagep = page;
- /* don't free the page */
+ /* Set the outparam pagep and return to the caller to
+ * copy the contents outside the lock. Don't free the
+ * page.
+ */
goto out;
}
} else {
- page = *pagep;
+ if (vm_shared &&
+ hugetlbfs_pagecache_present(h, dst_vma, dst_addr)) {
+ put_page(*pagep);
+ ret = -EEXIST;
+ *pagep = NULL;
+ goto out;
+ }
+
+ page = alloc_huge_page(dst_vma, dst_addr, 0);
+ if (IS_ERR(page)) {
+ ret = -ENOMEM;
+ *pagep = NULL;
+ goto out;
+ }
+ migrate_copy_huge_page(page, *pagep);
+ put_page(*pagep);
*pagep = NULL;
}
diff --git a/mm/migrate.c b/mm/migrate.c
index b234c3f3acb7..3bfe1f7d127d 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -550,7 +550,7 @@ static void __copy_gigantic_page(struct page *dst, struct page *src,
}
}
-static void copy_huge_page(struct page *dst, struct page *src)
+void migrate_copy_huge_page(struct page *dst, struct page *src)
{
int i;
int nr_pages;
@@ -652,7 +652,7 @@ EXPORT_SYMBOL(migrate_page_states);
void migrate_page_copy(struct page *newpage, struct page *page)
{
if (PageHuge(page) || PageTransHuge(page))
- copy_huge_page(newpage, page);
+ migrate_copy_huge_page(newpage, page);
else
copy_highpage(newpage, page);
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 2d6a3a36f6ce..e13a0492b7ba 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -346,54 +346,8 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
out_unlock:
mmap_read_unlock(dst_mm);
out:
- if (page) {
- /*
- * We encountered an error and are about to free a newly
- * allocated huge page.
- *
- * Reservation handling is very subtle, and is different for
- * private and shared mappings. See the routine
- * restore_reserve_on_error for details. Unfortunately, we
- * can not call restore_reserve_on_error now as it would
- * require holding mmap_lock.
- *
- * If a reservation for the page existed in the reservation
- * map of a private mapping, the map was modified to indicate
- * the reservation was consumed when the page was allocated.
- * We clear the HPageRestoreRsvCnt flag now so that the global
- * reserve count will not be incremented in free_huge_page.
- * The reservation map will still indicate the reservation
- * was consumed and possibly prevent later page allocation.
- * This is better than leaking a global reservation. If no
- * reservation existed, it is still safe to clear
- * HPageRestoreRsvCnt as no adjustments to reservation counts
- * were made during allocation.
- *
- * The reservation map for shared mappings indicates which
- * pages have reservations. When a huge page is allocated
- * for an address with a reservation, no change is made to
- * the reserve map. In this case HPageRestoreRsvCnt will be
- * set to indicate that the global reservation count should be
- * incremented when the page is freed. This is the desired
- * behavior. However, when a huge page is allocated for an
- * address without a reservation a reservation entry is added
- * to the reservation map, and HPageRestoreRsvCnt will not be
- * set. When the page is freed, the global reserve count will
- * NOT be incremented and it will appear as though we have
- * leaked reserved page. In this case, set HPageRestoreRsvCnt
- * so that the global reserve count will be incremented to
- * match the reservation map entry which was created.
- *
- * Note that vm_alloc_shared is based on the flags of the vma
- * for which the page was originally allocated. dst_vma could
- * be different or NULL on error.
- */
- if (vm_alloc_shared)
- SetHPageRestoreRsvCnt(page);
- else
- ClearHPageRestoreRsvCnt(page);
+ if (page)
put_page(page);
- }
BUG_ON(copied < 0);
BUG_ON(err > 0);
BUG_ON(!copied && !err);
--
2.32.0.rc0.204.g9fa02ecfa5-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4] mm, hugetlb: fix racy resv_huge_pages underflow on UFFDIO_COPY
2021-05-28 0:50 [PATCH v4] mm, hugetlb: fix racy resv_huge_pages underflow on UFFDIO_COPY Mina Almasry
@ 2021-05-31 23:25 ` Andrew Morton
2021-06-01 0:11 ` Mina Almasry
2021-06-04 21:41 ` Mike Kravetz
1 sibling, 1 reply; 12+ messages in thread
From: Andrew Morton @ 2021-05-31 23:25 UTC (permalink / raw)
To: Mina Almasry
Cc: Axel Rasmussen, Peter Xu, linux-mm, Mike Kravetz, linux-kernel
On Thu, 27 May 2021 17:50:29 -0700 Mina Almasry <almasrymina@google.com> wrote:
> On UFFDIO_COPY, if we fail to copy the page contents while holding the
> hugetlb_fault_mutex, we will drop the mutex and return to the caller
> after allocating a page that consumed a reservation. In this case there
> may be a fault that double consumes the reservation. To handle this, we
> free the allocated page, fix the reservations, and allocate a temporary
> hugetlb page and return that to the caller. When the caller does the
> copy outside of the lock, we again check the cache, and allocate a page
> consuming the reservation, and copy over the contents.
>
> Test:
> Hacked the code locally such that resv_huge_pages underflows produce
> a warning and the copy_huge_page_from_user() always fails, then:
>
> ./tools/testing/selftests/vm/userfaultfd hugetlb_shared 10
> 2 /tmp/kokonut_test/huge/userfaultfd_test && echo test success
> ./tools/testing/selftests/vm/userfaultfd hugetlb 10
> 2 /tmp/kokonut_test/huge/userfaultfd_test && echo test success
>
> Both tests succeed and produce no warnings. After the
> test runs number of free/resv hugepages is correct.
Many conflicts here with material that is queued for 5.14-rc1.
How serious is this problem? Is a -stable backport warranted?
If we decide to get this into 5.13 (and perhaps -stable) then I can
take a look at reworking all the 5.14 material on top. If not very
serious then we could rework this on top of the already queued
material.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4] mm, hugetlb: fix racy resv_huge_pages underflow on UFFDIO_COPY
2021-05-31 23:25 ` Andrew Morton
@ 2021-06-01 0:11 ` Mina Almasry
2021-06-01 0:36 ` Andrew Morton
0 siblings, 1 reply; 12+ messages in thread
From: Mina Almasry @ 2021-06-01 0:11 UTC (permalink / raw)
To: Andrew Morton; +Cc: Axel Rasmussen, Peter Xu, Linux-MM, Mike Kravetz, open list
On Mon, May 31, 2021 at 4:25 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Thu, 27 May 2021 17:50:29 -0700 Mina Almasry <almasrymina@google.com> wrote:
>
> > On UFFDIO_COPY, if we fail to copy the page contents while holding the
> > hugetlb_fault_mutex, we will drop the mutex and return to the caller
> > after allocating a page that consumed a reservation. In this case there
> > may be a fault that double consumes the reservation. To handle this, we
> > free the allocated page, fix the reservations, and allocate a temporary
> > hugetlb page and return that to the caller. When the caller does the
> > copy outside of the lock, we again check the cache, and allocate a page
> > consuming the reservation, and copy over the contents.
> >
> > Test:
> > Hacked the code locally such that resv_huge_pages underflows produce
> > a warning and the copy_huge_page_from_user() always fails, then:
> >
> > ./tools/testing/selftests/vm/userfaultfd hugetlb_shared 10
> > 2 /tmp/kokonut_test/huge/userfaultfd_test && echo test success
> > ./tools/testing/selftests/vm/userfaultfd hugetlb 10
> > 2 /tmp/kokonut_test/huge/userfaultfd_test && echo test success
> >
> > Both tests succeed and produce no warnings. After the
> > test runs number of free/resv hugepages is correct.
>
> Many conflicts here with material that is queued for 5.14-rc1.
>
> How serious is this problem? Is a -stable backport warranted?
>
I've sent 2 similar patches to the list:
1. "[PATCH v4] mm, hugetlb: Fix simple resv_huge_pages underflow on UFFDIO_COPY"
This one is sent to -stable and linux-mm and is a fairly simple fix.
2. "[PATCH v4] mm, hugetlb: fix racy resv_huge_pages underflow on UFFDIO_COPY"
Which is this patch. It's a more complicated and not critical fix, so
not targeted for -stable. It's only sent to linux-mm.
> If we decide to get this into 5.13 (and perhaps -stable) then I can
> take a look at reworking all the 5.14 material on top. If not very
> serious then we could rework this on top of the already queued
> material.
>
I assume given the above we want to rework this on top of the already
queued material. I can upload a v5 that is rebased on top of your
branch. Note that you have an earlier version of this fix in your
branch, so really this patch will turn into a fix for that patch if I
rebase it (I assume that's fine).
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4] mm, hugetlb: fix racy resv_huge_pages underflow on UFFDIO_COPY
2021-06-01 0:11 ` Mina Almasry
@ 2021-06-01 0:36 ` Andrew Morton
2021-06-01 2:48 ` Mina Almasry
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Morton @ 2021-06-01 0:36 UTC (permalink / raw)
To: Mina Almasry; +Cc: Axel Rasmussen, Peter Xu, Linux-MM, Mike Kravetz, open list
On Mon, 31 May 2021 17:11:52 -0700 Mina Almasry <almasrymina@google.com> wrote:
> On Mon, May 31, 2021 at 4:25 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Thu, 27 May 2021 17:50:29 -0700 Mina Almasry <almasrymina@google.com> wrote:
> >
> > > On UFFDIO_COPY, if we fail to copy the page contents while holding the
> > > hugetlb_fault_mutex, we will drop the mutex and return to the caller
> > > after allocating a page that consumed a reservation. In this case there
> > > may be a fault that double consumes the reservation. To handle this, we
> > > free the allocated page, fix the reservations, and allocate a temporary
> > > hugetlb page and return that to the caller. When the caller does the
> > > copy outside of the lock, we again check the cache, and allocate a page
> > > consuming the reservation, and copy over the contents.
> > >
> > > Test:
> > > Hacked the code locally such that resv_huge_pages underflows produce
> > > a warning and the copy_huge_page_from_user() always fails, then:
> > >
> > > ./tools/testing/selftests/vm/userfaultfd hugetlb_shared 10
> > > 2 /tmp/kokonut_test/huge/userfaultfd_test && echo test success
> > > ./tools/testing/selftests/vm/userfaultfd hugetlb 10
> > > 2 /tmp/kokonut_test/huge/userfaultfd_test && echo test success
> > >
> > > Both tests succeed and produce no warnings. After the
> > > test runs number of free/resv hugepages is correct.
> >
> > Many conflicts here with material that is queued for 5.14-rc1.
> >
> > How serious is this problem? Is a -stable backport warranted?
> >
>
> I've sent 2 similar patches to the list:
>
> 1. "[PATCH v4] mm, hugetlb: Fix simple resv_huge_pages underflow on UFFDIO_COPY"
>
> This one is sent to -stable and linux-mm and is a fairly simple fix.
>
> 2. "[PATCH v4] mm, hugetlb: fix racy resv_huge_pages underflow on UFFDIO_COPY"
Ah, OK, the title of the first patch was changed, which threw me off.
I'd skipped "[PATCH v4] mm, hugetlb: Fix simple resv_huge_pages
underflow on UFFDIO_COPY" because Mike's comments appeared to require a
v5. I applied it and made Mike's changelog suggestions. Queued for
5.13 and -stable.
And I queued "[PATCH v4] mm, hugetlb: fix racy resv_huge_pages
underflow on UFFDIO_COPY" for 5.14.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4] mm, hugetlb: fix racy resv_huge_pages underflow on UFFDIO_COPY
2021-06-01 0:36 ` Andrew Morton
@ 2021-06-01 2:48 ` Mina Almasry
2021-06-01 17:09 ` Mike Kravetz
0 siblings, 1 reply; 12+ messages in thread
From: Mina Almasry @ 2021-06-01 2:48 UTC (permalink / raw)
To: Andrew Morton; +Cc: Axel Rasmussen, Peter Xu, Linux-MM, Mike Kravetz, open list
On Mon, May 31, 2021 at 5:36 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Mon, 31 May 2021 17:11:52 -0700 Mina Almasry <almasrymina@google.com> wrote:
>
> > On Mon, May 31, 2021 at 4:25 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > >
> > > On Thu, 27 May 2021 17:50:29 -0700 Mina Almasry <almasrymina@google.com> wrote:
> > >
> > > > On UFFDIO_COPY, if we fail to copy the page contents while holding the
> > > > hugetlb_fault_mutex, we will drop the mutex and return to the caller
> > > > after allocating a page that consumed a reservation. In this case there
> > > > may be a fault that double consumes the reservation. To handle this, we
> > > > free the allocated page, fix the reservations, and allocate a temporary
> > > > hugetlb page and return that to the caller. When the caller does the
> > > > copy outside of the lock, we again check the cache, and allocate a page
> > > > consuming the reservation, and copy over the contents.
> > > >
> > > > Test:
> > > > Hacked the code locally such that resv_huge_pages underflows produce
> > > > a warning and the copy_huge_page_from_user() always fails, then:
> > > >
> > > > ./tools/testing/selftests/vm/userfaultfd hugetlb_shared 10
> > > > 2 /tmp/kokonut_test/huge/userfaultfd_test && echo test success
> > > > ./tools/testing/selftests/vm/userfaultfd hugetlb 10
> > > > 2 /tmp/kokonut_test/huge/userfaultfd_test && echo test success
> > > >
> > > > Both tests succeed and produce no warnings. After the
> > > > test runs number of free/resv hugepages is correct.
> > >
> > > Many conflicts here with material that is queued for 5.14-rc1.
> > >
> > > How serious is this problem? Is a -stable backport warranted?
> > >
> >
> > I've sent 2 similar patches to the list:
> >
> > 1. "[PATCH v4] mm, hugetlb: Fix simple resv_huge_pages underflow on UFFDIO_COPY"
> >
> > This one is sent to -stable and linux-mm and is a fairly simple fix.
> >
> > 2. "[PATCH v4] mm, hugetlb: fix racy resv_huge_pages underflow on UFFDIO_COPY"
>
> Ah, OK, the title of the first patch was changed, which threw me off.
>
> I'd skipped "[PATCH v4] mm, hugetlb: Fix simple resv_huge_pages
> underflow on UFFDIO_COPY" because Mike's comments appeared to require a
> v5. I applied it and made Mike's changelog suggestions. Queued for
> 5.13 and -stable.
>
> And I queued "[PATCH v4] mm, hugetlb: fix racy resv_huge_pages
> underflow on UFFDIO_COPY" for 5.14.
>
>
Awesome, thanks! And sorry for the confusion!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4] mm, hugetlb: fix racy resv_huge_pages underflow on UFFDIO_COPY
2021-06-01 2:48 ` Mina Almasry
@ 2021-06-01 17:09 ` Mike Kravetz
2021-06-01 18:04 ` Mina Almasry
0 siblings, 1 reply; 12+ messages in thread
From: Mike Kravetz @ 2021-06-01 17:09 UTC (permalink / raw)
To: Mina Almasry, Andrew Morton; +Cc: Axel Rasmussen, Peter Xu, Linux-MM, open list
On 5/31/21 7:48 PM, Mina Almasry wrote:
> On Mon, May 31, 2021 at 5:36 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>> On Mon, 31 May 2021 17:11:52 -0700 Mina Almasry <almasrymina@google.com> wrote:
>>> On Mon, May 31, 2021 at 4:25 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>>>> On Thu, 27 May 2021 17:50:29 -0700 Mina Almasry <almasrymina@google.com> wrote:
>>> I've sent 2 similar patches to the list:
>>>
>>> 1. "[PATCH v4] mm, hugetlb: Fix simple resv_huge_pages underflow on UFFDIO_COPY"
>>>
>>> This one is sent to -stable and linux-mm and is a fairly simple fix.
>>>
>>> 2. "[PATCH v4] mm, hugetlb: fix racy resv_huge_pages underflow on UFFDIO_COPY"
>>
>> Ah, OK, the title of the first patch was changed, which threw me off.
>>
>> I'd skipped "[PATCH v4] mm, hugetlb: Fix simple resv_huge_pages
>> underflow on UFFDIO_COPY" because Mike's comments appeared to require a
>> v5. I applied it and made Mike's changelog suggestions. Queued for
>> 5.13 and -stable.
>>
>> And I queued "[PATCH v4] mm, hugetlb: fix racy resv_huge_pages
>> underflow on UFFDIO_COPY" for 5.14.
>>
>>
>
> Awesome, thanks! And sorry for the confusion!
>
Mina, does this patch depend on changes to restore_reserve_on_error()?
I am still working on those changes. It may be a few days before I can
have something finalized.
If this does depend on restore_reserve_on_error as I suspect, perhaps we
should send these together.
--
Mike Kravetz
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4] mm, hugetlb: fix racy resv_huge_pages underflow on UFFDIO_COPY
2021-06-01 17:09 ` Mike Kravetz
@ 2021-06-01 18:04 ` Mina Almasry
2021-06-01 18:05 ` Mina Almasry
0 siblings, 1 reply; 12+ messages in thread
From: Mina Almasry @ 2021-06-01 18:04 UTC (permalink / raw)
To: Mike Kravetz; +Cc: Andrew Morton, Axel Rasmussen, Peter Xu, Linux-MM, open list
On Tue, Jun 1, 2021 at 10:09 AM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>
> On 5/31/21 7:48 PM, Mina Almasry wrote:
> > On Mon, May 31, 2021 at 5:36 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >> On Mon, 31 May 2021 17:11:52 -0700 Mina Almasry <almasrymina@google.com> wrote:
> >>> On Mon, May 31, 2021 at 4:25 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >>>> On Thu, 27 May 2021 17:50:29 -0700 Mina Almasry <almasrymina@google.com> wrote:
> >>> I've sent 2 similar patches to the list:
> >>>
> >>> 1. "[PATCH v4] mm, hugetlb: Fix simple resv_huge_pages underflow on UFFDIO_COPY"
> >>>
> >>> This one is sent to -stable and linux-mm and is a fairly simple fix.
> >>>
> >>> 2. "[PATCH v4] mm, hugetlb: fix racy resv_huge_pages underflow on UFFDIO_COPY"
> >>
> >> Ah, OK, the title of the first patch was changed, which threw me off.
> >>
> >> I'd skipped "[PATCH v4] mm, hugetlb: Fix simple resv_huge_pages
> >> underflow on UFFDIO_COPY" because Mike's comments appeared to require a
> >> v5. I applied it and made Mike's changelog suggestions. Queued for
> >> 5.13 and -stable.
> >>
> >> And I queued "[PATCH v4] mm, hugetlb: fix racy resv_huge_pages
> >> underflow on UFFDIO_COPY" for 5.14.
> >>
> >>
> >
> > Awesome, thanks! And sorry for the confusion!
> >
>
> Mina, does this patch depend on changes to restore_reserve_on_error()?
>
Yes, this patch (and only this patch) depends on your changes for
complete and correct functionality. I'm not sure what's the impact
> I am still working on those changes. It may be a few days before I can
> have something finalized.
>
> If this does depend on restore_reserve_on_error as I suspect, perhaps we
> should send these together.
I was thinking it's fine to have my fix in Andrew's tree a few days
before yours, since the race is hard to reproduce and even if the race
reproduces the userfaultfd tests still pass, so I don't see any
disastrous consequences, but I'm happy to do whatever is appropriate
here.
> --
> Mike Kravetz
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4] mm, hugetlb: fix racy resv_huge_pages underflow on UFFDIO_COPY
2021-06-01 18:04 ` Mina Almasry
@ 2021-06-01 18:05 ` Mina Almasry
0 siblings, 0 replies; 12+ messages in thread
From: Mina Almasry @ 2021-06-01 18:05 UTC (permalink / raw)
To: Mike Kravetz; +Cc: Andrew Morton, Axel Rasmussen, Peter Xu, Linux-MM, open list
> > Mina, does this patch depend on changes to restore_reserve_on_error()?
> >
>
> Yes, this patch (and only this patch) depends on your changes for
> complete and correct functionality. I'm not sure what's the impact
...of missing your changes aside from the underflow. The userfaultfd
tests still pass.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4] mm, hugetlb: fix racy resv_huge_pages underflow on UFFDIO_COPY
2021-05-28 0:50 [PATCH v4] mm, hugetlb: fix racy resv_huge_pages underflow on UFFDIO_COPY Mina Almasry
2021-05-31 23:25 ` Andrew Morton
@ 2021-06-04 21:41 ` Mike Kravetz
2021-06-05 1:06 ` [PATCH] mm, hugetlb: fix allocation error check and copy func name Mina Almasry
1 sibling, 1 reply; 12+ messages in thread
From: Mike Kravetz @ 2021-06-04 21:41 UTC (permalink / raw)
To: Mina Almasry
Cc: Axel Rasmussen, Peter Xu, linux-mm, Andrew Morton, linux-kernel
On 5/27/21 5:50 PM, Mina Almasry wrote:
> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> index 4bb4e519e3f5..4164c9ddd86e 100644
> --- a/include/linux/migrate.h
> +++ b/include/linux/migrate.h
> @@ -51,6 +51,7 @@ extern int migrate_huge_page_move_mapping(struct address_space *mapping,
> struct page *newpage, struct page *page);
> extern int migrate_page_move_mapping(struct address_space *mapping,
> struct page *newpage, struct page *page, int extra_count);
> +extern void migrate_copy_huge_page(struct page *dst, struct page *src);
> #else
>
> static inline void putback_movable_pages(struct list_head *l) {}
> @@ -77,6 +78,9 @@ static inline int migrate_huge_page_move_mapping(struct address_space *mapping,
> return -ENOSYS;
> }
>
> +static inline void migrate_copy_huge_page(struct page *dst, struct page *src)
> +{
> +}
> #endif /* CONFIG_MIGRATION */
>
> #ifdef CONFIG_COMPACTION
I am not insisting, but it might be better to make the copy routine
available under the current name 'copy_huge_page'.
Why?
There is an existing migrate_page_copy() which not only copies the page
contents, but also page state/metadata. People could get confused that
'migrate_page_copy' and 'migrate_copy_huge_page' do not have the same
functionality. Of course, as soon as you look at the routines you can
see the difference.
Again, not necessary. Just something to consider. I suspect you
changed the name to 'migrate_copy_huge_page' mostly because it resides
in migrate.c?
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 76e2a6efc165..6072c9f82794 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -30,6 +30,7 @@
> #include <linux/numa.h>
> #include <linux/llist.h>
> #include <linux/cma.h>
> +#include <linux/migrate.h>
>
> #include <asm/page.h>
> #include <asm/pgalloc.h>
> @@ -4905,20 +4906,17 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
> struct page **pagep)
> {
> bool is_continue = (mode == MCOPY_ATOMIC_CONTINUE);
> - struct address_space *mapping;
> - pgoff_t idx;
> + struct hstate *h = hstate_vma(dst_vma);
> + struct address_space *mapping = dst_vma->vm_file->f_mapping;
> + pgoff_t idx = vma_hugecache_offset(h, dst_vma, dst_addr);
> unsigned long size;
> int vm_shared = dst_vma->vm_flags & VM_SHARED;
> - struct hstate *h = hstate_vma(dst_vma);
> pte_t _dst_pte;
> spinlock_t *ptl;
> - int ret;
> + int ret = -ENOMEM;
> struct page *page;
> int writable;
>
> - mapping = dst_vma->vm_file->f_mapping;
> - idx = vma_hugecache_offset(h, dst_vma, dst_addr);
> -
> if (is_continue) {
> ret = -EFAULT;
> page = find_lock_page(mapping, idx);
> @@ -4947,12 +4945,44 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
> /* fallback to copy_from_user outside mmap_lock */
> if (unlikely(ret)) {
> ret = -ENOENT;
> + /* Free the allocated page which may have
> + * consumed a reservation.
> + */
> + restore_reserve_on_error(h, dst_vma, dst_addr, page);
> + put_page(page);
> +
> + /* Allocate a temporary page to hold the copied
> + * contents.
> + */
> + page = alloc_huge_page_vma(h, dst_vma, dst_addr);
> + if (IS_ERR(page)) {
In v3 of the patch, alloc_migrate_huge_page was used to allocate the
temporary page and Dan Carpenter pointed out that the return value should
just be checked for NULL. I believe the same still applies to
alloc_huge_page_vma.
--
Mike Kravetz
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] mm, hugetlb: fix allocation error check and copy func name
2021-06-04 21:41 ` Mike Kravetz
@ 2021-06-05 1:06 ` Mina Almasry
2021-06-05 12:49 ` [External] " Muchun Song
2021-06-07 20:32 ` Mike Kravetz
0 siblings, 2 replies; 12+ messages in thread
From: Mina Almasry @ 2021-06-05 1:06 UTC (permalink / raw)
Cc: Mina Almasry, linux-mm, Mike Kravetz, Andrew Morton, linux-kernel
The alloc_huge_page_vma() doesn't return error pointers, it returns
NULL.
There is an existing migrate_page_copy() which not only copies the page
contents, but also page state/metadata. People could get confused that
'migrate_page_copy' and 'migrate_copy_huge_page' do not have the same
functionality. Rename to 'copy_huge_page()' to make it less likely
folks get confused.
Fixes: 6a0badc266df ("mm, hugetlb: fix racy resv_huge_pages underflow on
UFFDIO_COPY")
Signed-off-by: Mina Almasry <almasrymina@google.com>
Cc: linux-mm@kvack.org
Cc: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org
---
include/linux/migrate.h | 4 ++--
mm/hugetlb.c | 4 ++--
mm/migrate.c | 4 ++--
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index e0b98876a52e..9b7b7cd3bae9 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -51,7 +51,7 @@ extern int migrate_huge_page_move_mapping(struct address_space *mapping,
struct page *newpage, struct page *page);
extern int migrate_page_move_mapping(struct address_space *mapping,
struct page *newpage, struct page *page, int extra_count);
-extern void migrate_copy_huge_page(struct page *dst, struct page *src);
+extern void copy_huge_page(struct page *dst, struct page *src);
#else
static inline void putback_movable_pages(struct list_head *l) {}
@@ -78,7 +78,7 @@ static inline int migrate_huge_page_move_mapping(struct address_space *mapping,
return -ENOSYS;
}
-static inline void migrate_copy_huge_page(struct page *dst, struct page *src)
+static inline void copy_huge_page(struct page *dst, struct page *src)
{
}
#endif /* CONFIG_MIGRATION */
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 64a460ff2d5b..637395907908 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5130,7 +5130,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
* contents.
*/
page = alloc_huge_page_vma(h, dst_vma, dst_addr);
- if (IS_ERR(page)) {
+ if (!page) {
ret = -ENOMEM;
goto out;
}
@@ -5156,7 +5156,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
*pagep = NULL;
goto out;
}
- migrate_copy_huge_page(page, *pagep);
+ copy_huge_page(page, *pagep);
put_page(*pagep);
*pagep = NULL;
}
diff --git a/mm/migrate.c b/mm/migrate.c
index 2515210a9747..51f355757490 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -557,7 +557,7 @@ static void __copy_gigantic_page(struct page *dst, struct page *src,
}
}
-void migrate_copy_huge_page(struct page *dst, struct page *src)
+void copy_huge_page(struct page *dst, struct page *src)
{
int i;
int nr_pages;
@@ -662,7 +662,7 @@ EXPORT_SYMBOL(migrate_page_states);
void migrate_page_copy(struct page *newpage, struct page *page)
{
if (PageHuge(page) || PageTransHuge(page))
- migrate_copy_huge_page(newpage, page);
+ copy_huge_page(newpage, page);
else
copy_highpage(newpage, page);
--
2.32.0.rc1.229.g3e70b5a671-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [External] [PATCH] mm, hugetlb: fix allocation error check and copy func name
2021-06-05 1:06 ` [PATCH] mm, hugetlb: fix allocation error check and copy func name Mina Almasry
@ 2021-06-05 12:49 ` Muchun Song
2021-06-07 20:32 ` Mike Kravetz
1 sibling, 0 replies; 12+ messages in thread
From: Muchun Song @ 2021-06-05 12:49 UTC (permalink / raw)
To: Mina Almasry
Cc: Linux Memory Management List, Mike Kravetz, Andrew Morton, LKML
On Sat, Jun 5, 2021 at 9:07 AM Mina Almasry <almasrymina@google.com> wrote:
>
> The alloc_huge_page_vma() doesn't return error pointers, it returns
> NULL.
>
> There is an existing migrate_page_copy() which not only copies the page
> contents, but also page state/metadata. People could get confused that
> 'migrate_page_copy' and 'migrate_copy_huge_page' do not have the same
> functionality. Rename to 'copy_huge_page()' to make it less likely
> folks get confused.
>
> Fixes: 6a0badc266df ("mm, hugetlb: fix racy resv_huge_pages underflow on
> UFFDIO_COPY")
I think the commit id is not stable. So this really should squash
the commit of "mm, hugetlb: fix racy resv_huge_pages underflow
on UFFDIO_COPY". But anyway,
Reviewed-by: Muchun Song <songmuchun@bytedance.com>
>
> Signed-off-by: Mina Almasry <almasrymina@google.com>
> Cc: linux-mm@kvack.org
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-kernel@vger.kernel.org
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mm, hugetlb: fix allocation error check and copy func name
2021-06-05 1:06 ` [PATCH] mm, hugetlb: fix allocation error check and copy func name Mina Almasry
2021-06-05 12:49 ` [External] " Muchun Song
@ 2021-06-07 20:32 ` Mike Kravetz
1 sibling, 0 replies; 12+ messages in thread
From: Mike Kravetz @ 2021-06-07 20:32 UTC (permalink / raw)
To: Mina Almasry; +Cc: linux-mm, Andrew Morton, linux-kernel
On 6/4/21 6:06 PM, Mina Almasry wrote:
> The alloc_huge_page_vma() doesn't return error pointers, it returns
> NULL.
>
> There is an existing migrate_page_copy() which not only copies the page
> contents, but also page state/metadata. People could get confused that
> 'migrate_page_copy' and 'migrate_copy_huge_page' do not have the same
> functionality. Rename to 'copy_huge_page()' to make it less likely
> folks get confused.
>
> Fixes: 6a0badc266df ("mm, hugetlb: fix racy resv_huge_pages underflow on
> UFFDIO_COPY")
>
> Signed-off-by: Mina Almasry <almasrymina@google.com>
> Cc: linux-mm@kvack.org
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-kernel@vger.kernel.org
>
> ---
> include/linux/migrate.h | 4 ++--
> mm/hugetlb.c | 4 ++--
> mm/migrate.c | 4 ++--
> 3 files changed, 6 insertions(+), 6 deletions(-)
As mentioned by Muchun, Andrew should squash this and original patch.
With the combined patches,
Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
--
Mike Kravetz
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-06-07 20:32 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-28 0:50 [PATCH v4] mm, hugetlb: fix racy resv_huge_pages underflow on UFFDIO_COPY Mina Almasry
2021-05-31 23:25 ` Andrew Morton
2021-06-01 0:11 ` Mina Almasry
2021-06-01 0:36 ` Andrew Morton
2021-06-01 2:48 ` Mina Almasry
2021-06-01 17:09 ` Mike Kravetz
2021-06-01 18:04 ` Mina Almasry
2021-06-01 18:05 ` Mina Almasry
2021-06-04 21:41 ` Mike Kravetz
2021-06-05 1:06 ` [PATCH] mm, hugetlb: fix allocation error check and copy func name Mina Almasry
2021-06-05 12:49 ` [External] " Muchun Song
2021-06-07 20:32 ` Mike Kravetz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).