This series aims to fix a number of page reference counting issues in drivers
dealing with device private ZONE_DEVICE pages. These result in use-after-free
type bugs, either from accessing a struct page which no longer exists because it
has been removed or accessing fields within the struct page which are no longer
valid because the page has been freed.
During normal usage it is unlikely these will cause any problems. However
without these fixes it is possible to crash the kernel from userspace. These
crashes can be triggered either by unloading the kernel module or unbinding the
device from the driver prior to a userspace task exiting. In modules such as
Nouveau it is also possible to trigger some of these issues by explicitly
closing the device file-descriptor prior to the task exiting and then accessing
device private memory.
This involves changes to both PowerPC and AMD GPU code. Unfortunately I lack the
hardware to test on either of these so would appreciate it if someone with
access could test those.
Alistair Popple (7):
mm/memory.c: Fix race when faulting a device private page
mm: Free device private pages have zero refcount
mm/migrate_device.c: Refactor migrate_vma and migrate_deivce_coherent_page()
mm/migrate_device.c: Add migrate_device_range()
nouveau/dmem: Refactor nouveau_dmem_fault_copy_one()
nouveau/dmem: Evict device private memory during release
hmm-tests: Add test for migrate_device_range()
arch/powerpc/kvm/book3s_hv_uvmem.c | 16 +-
drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 18 +-
drivers/gpu/drm/amd/amdkfd/kfd_migrate.h | 2 +-
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 11 +-
drivers/gpu/drm/nouveau/nouveau_dmem.c | 108 +++++++----
include/linux/migrate.h | 15 ++-
lib/test_hmm.c | 127 ++++++++++---
lib/test_hmm_uapi.h | 1 +-
mm/memory.c | 16 +-
mm/memremap.c | 5 +-
mm/migrate.c | 34 +--
mm/migrate_device.c | 239 +++++++++++++++++-------
mm/page_alloc.c | 6 +-
tools/testing/selftests/vm/hmm-tests.c | 49 +++++-
14 files changed, 487 insertions(+), 160 deletions(-)
base-commit: 088b8aa537c2c767765f1c19b555f21ffe555786
--
git-series 0.9.1
Device drivers can use the migrate_vma family of functions to migrate
existing private anonymous mappings to device private pages. These pages
are backed by memory on the device with drivers being responsible for
copying data to and from device memory.
Device private pages are freed via the pgmap->page_free() callback when
they are unmapped and their refcount drops to zero. Alternatively they
may be freed indirectly via migration back to CPU memory in response to
a pgmap->migrate_to_ram() callback called whenever the CPU accesses
an address mapped to a device private page.
In other words drivers cannot control the lifetime of data allocated on
the devices and must wait until these pages are freed from userspace.
This causes issues when memory needs to reclaimed on the device, either
because the device is going away due to a ->release() callback or
because another user needs to use the memory.
Drivers could use the existing migrate_vma functions to migrate data off
the device. However this would require them to track the mappings of
each page which is both complicated and not always possible. Instead
drivers need to be able to migrate device pages directly so they can
free up device memory.
To allow that this patch introduces the migrate_device family of
functions which are functionally similar to migrate_vma but which skips
the initial lookup based on mapping.
Signed-off-by: Alistair Popple <[email protected]>
---
include/linux/migrate.h | 7 +++-
mm/migrate_device.c | 89 ++++++++++++++++++++++++++++++++++++++----
2 files changed, 89 insertions(+), 7 deletions(-)
diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index 82ffa47..582cdc7 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -225,6 +225,13 @@ struct migrate_vma {
int migrate_vma_setup(struct migrate_vma *args);
void migrate_vma_pages(struct migrate_vma *migrate);
void migrate_vma_finalize(struct migrate_vma *migrate);
+int migrate_device_range(unsigned long *src_pfns, unsigned long start,
+ unsigned long npages);
+void migrate_device_pages(unsigned long *src_pfns, unsigned long *dst_pfns,
+ unsigned long npages);
+void migrate_device_finalize(unsigned long *src_pfns,
+ unsigned long *dst_pfns, unsigned long npages);
+
#endif /* CONFIG_MIGRATION */
#endif /* _LINUX_MIGRATE_H */
diff --git a/mm/migrate_device.c b/mm/migrate_device.c
index ba479b5..824860a 100644
--- a/mm/migrate_device.c
+++ b/mm/migrate_device.c
@@ -681,7 +681,7 @@ static void migrate_vma_insert_page(struct migrate_vma *migrate,
*src &= ~MIGRATE_PFN_MIGRATE;
}
-static void migrate_device_pages(unsigned long *src_pfns,
+static void __migrate_device_pages(unsigned long *src_pfns,
unsigned long *dst_pfns, unsigned long npages,
struct migrate_vma *migrate)
{
@@ -703,6 +703,9 @@ static void migrate_device_pages(unsigned long *src_pfns,
if (!page) {
unsigned long addr;
+ if (!(src_pfns[i] & MIGRATE_PFN_MIGRATE))
+ continue;
+
/*
* The only time there is no vma is when called from
* migrate_device_coherent_page(). However this isn't
@@ -710,8 +713,6 @@ static void migrate_device_pages(unsigned long *src_pfns,
*/
VM_BUG_ON(!migrate);
addr = migrate->start + i*PAGE_SIZE;
- if (!(src_pfns[i] & MIGRATE_PFN_MIGRATE))
- continue;
if (!notified) {
notified = true;
@@ -767,6 +768,22 @@ static void migrate_device_pages(unsigned long *src_pfns,
}
/**
+ * migrate_device_pages() - migrate meta-data from src page to dst page
+ * @src_pfns: src_pfns returned from migrate_device_range()
+ * @dst_pfns: array of pfns allocated by the driver to migrate memory to
+ * @npages: number of pages in the range
+ *
+ * Equivalent to migrate_vma_pages(). This is called to migrate struct page
+ * meta-data from source struct page to destination.
+ */
+void migrate_device_pages(unsigned long *src_pfns, unsigned long *dst_pfns,
+ unsigned long npages)
+{
+ __migrate_device_pages(src_pfns, dst_pfns, npages, NULL);
+}
+EXPORT_SYMBOL(migrate_device_pages);
+
+/**
* migrate_vma_pages() - migrate meta-data from src page to dst page
* @migrate: migrate struct containing all migration information
*
@@ -776,12 +793,22 @@ static void migrate_device_pages(unsigned long *src_pfns,
*/
void migrate_vma_pages(struct migrate_vma *migrate)
{
- migrate_device_pages(migrate->src, migrate->dst, migrate->npages, migrate);
+ __migrate_device_pages(migrate->src, migrate->dst, migrate->npages, migrate);
}
EXPORT_SYMBOL(migrate_vma_pages);
-static void migrate_device_finalize(unsigned long *src_pfns,
- unsigned long *dst_pfns, unsigned long npages)
+/*
+ * migrate_device_finalize() - complete page migration
+ * @src_pfns: src_pfns returned from migrate_device_range()
+ * @dst_pfns: array of pfns allocated by the driver to migrate memory to
+ * @npages: number of pages in the range
+ *
+ * Completes migration of the page by removing special migration entries.
+ * Drivers must ensure copying of page data is complete and visible to the CPU
+ * before calling this.
+ */
+void migrate_device_finalize(unsigned long *src_pfns,
+ unsigned long *dst_pfns, unsigned long npages)
{
unsigned long i;
@@ -825,6 +852,7 @@ static void migrate_device_finalize(unsigned long *src_pfns,
}
}
}
+EXPORT_SYMBOL(migrate_device_finalize);
/**
* migrate_vma_finalize() - restore CPU page table entry
@@ -843,6 +871,53 @@ void migrate_vma_finalize(struct migrate_vma *migrate)
}
EXPORT_SYMBOL(migrate_vma_finalize);
+/**
+ * migrate_device_range() - migrate device private pfns to normal memory.
+ * @src_pfns: array large enough to hold migrating source device private pfns.
+ * @start: starting pfn in the range to migrate.
+ * @npages: number of pages to migrate.
+ *
+ * migrate_vma_setup() is similar in concept to migrate_vma_setup() except that
+ * instead of looking up pages based on virtual address mappings a range of
+ * device pfns that should be migrated to system memory is used instead.
+ *
+ * This is useful when a driver needs to free device memory but doesn't know the
+ * virtual mappings of every page that may be in device memory. For example this
+ * is often the case when a driver is being unloaded or unbound from a device.
+ *
+ * Like migrate_vma_setup() this function will take a reference and lock any
+ * migrating pages that aren't free before unmapping them. Drivers may then
+ * allocate destination pages and start copying data from the device to CPU
+ * memory before calling migrate_device_pages().
+ */
+int migrate_device_range(unsigned long *src_pfns, unsigned long start,
+ unsigned long npages)
+{
+ unsigned long i, pfn;
+
+ for (pfn = start, i = 0; i < npages; pfn++, i++) {
+ struct page *page = pfn_to_page(pfn);
+
+ if (!get_page_unless_zero(page)) {
+ src_pfns[i] = 0;
+ continue;
+ }
+
+ if (!trylock_page(page)) {
+ src_pfns[i] = 0;
+ put_page(page);
+ continue;
+ }
+
+ src_pfns[i] = migrate_pfn(pfn) | MIGRATE_PFN_MIGRATE;
+ }
+
+ migrate_device_unmap(src_pfns, npages, NULL);
+
+ return 0;
+}
+EXPORT_SYMBOL(migrate_device_range);
+
/*
* Migrate a device coherent page back to normal memory. The caller should have
* a reference on page which will be copied to the new page if migration is
@@ -873,7 +948,7 @@ int migrate_device_coherent_page(struct page *page)
dst_pfn = migrate_pfn(page_to_pfn(dpage));
}
- migrate_device_pages(&src_pfn, &dst_pfn, 1, NULL);
+ migrate_device_pages(&src_pfn, &dst_pfn, 1);
if (src_pfn & MIGRATE_PFN_MIGRATE)
copy_highpage(dpage, page);
migrate_device_finalize(&src_pfn, &dst_pfn, 1);
--
git-series 0.9.1
migrate_device_coherent_page() reuses the existing migrate_vma family of
functions to migrate a specific page without providing a valid mapping
or vma. This looks a bit odd because it means we are calling
migrate_vma_*() without setting a valid vma, however it was considered
acceptable at the time because the details were internal to
migrate_device.c and there was only a single user.
One of the reasons the details could be kept internal was that this was
strictly for migrating device coherent memory. Such memory can be copied
directly by the CPU without intervention from a driver. However this
isn't true for device private memory, and a future change requires
similar functionality for device private memory. So refactor the code
into something more sensible for migrating device memory without a vma.
Signed-off-by: Alistair Popple <[email protected]>
---
mm/migrate_device.c | 150 +++++++++++++++++++++++++--------------------
1 file changed, 85 insertions(+), 65 deletions(-)
diff --git a/mm/migrate_device.c b/mm/migrate_device.c
index f756c00..ba479b5 100644
--- a/mm/migrate_device.c
+++ b/mm/migrate_device.c
@@ -345,26 +345,20 @@ static bool migrate_vma_check_page(struct page *page, struct page *fault_page)
}
/*
- * migrate_vma_unmap() - replace page mapping with special migration pte entry
- * @migrate: migrate struct containing all migration information
- *
- * Isolate pages from the LRU and replace mappings (CPU page table pte) with a
- * special migration pte entry and check if it has been pinned. Pinned pages are
- * restored because we cannot migrate them.
- *
- * This is the last step before we call the device driver callback to allocate
- * destination memory and copy contents of original page over to new page.
+ * Unmaps pages for migration. Returns number of unmapped pages.
*/
-static void migrate_vma_unmap(struct migrate_vma *migrate)
+static unsigned long migrate_device_unmap(unsigned long *src_pfns,
+ unsigned long npages,
+ struct page *fault_page)
{
- const unsigned long npages = migrate->npages;
unsigned long i, restore = 0;
bool allow_drain = true;
+ unsigned long unmapped = 0;
lru_add_drain();
for (i = 0; i < npages; i++) {
- struct page *page = migrate_pfn_to_page(migrate->src[i]);
+ struct page *page = migrate_pfn_to_page(src_pfns[i]);
struct folio *folio;
if (!page)
@@ -379,8 +373,7 @@ static void migrate_vma_unmap(struct migrate_vma *migrate)
}
if (isolate_lru_page(page)) {
- migrate->src[i] &= ~MIGRATE_PFN_MIGRATE;
- migrate->cpages--;
+ src_pfns[i] &= ~MIGRATE_PFN_MIGRATE;
restore++;
continue;
}
@@ -394,34 +387,54 @@ static void migrate_vma_unmap(struct migrate_vma *migrate)
try_to_migrate(folio, 0);
if (page_mapped(page) ||
- !migrate_vma_check_page(page, migrate->fault_page)) {
+ !migrate_vma_check_page(page, fault_page)) {
if (!is_zone_device_page(page)) {
get_page(page);
putback_lru_page(page);
}
- migrate->src[i] &= ~MIGRATE_PFN_MIGRATE;
- migrate->cpages--;
+ src_pfns[i] &= ~MIGRATE_PFN_MIGRATE;
restore++;
continue;
}
+
+ unmapped++;
}
for (i = 0; i < npages && restore; i++) {
- struct page *page = migrate_pfn_to_page(migrate->src[i]);
+ struct page *page = migrate_pfn_to_page(src_pfns[i]);
struct folio *folio;
- if (!page || (migrate->src[i] & MIGRATE_PFN_MIGRATE))
+ if (!page || (src_pfns[i] & MIGRATE_PFN_MIGRATE))
continue;
folio = page_folio(page);
remove_migration_ptes(folio, folio, false);
- migrate->src[i] = 0;
+ src_pfns[i] = 0;
folio_unlock(folio);
folio_put(folio);
restore--;
}
+
+ return unmapped;
+}
+
+/*
+ * migrate_vma_unmap() - replace page mapping with special migration pte entry
+ * @migrate: migrate struct containing all migration information
+ *
+ * Isolate pages from the LRU and replace mappings (CPU page table pte) with a
+ * special migration pte entry and check if it has been pinned. Pinned pages are
+ * restored because we cannot migrate them.
+ *
+ * This is the last step before we call the device driver callback to allocate
+ * destination memory and copy contents of original page over to new page.
+ */
+static void migrate_vma_unmap(struct migrate_vma *migrate)
+{
+ migrate->cpages = migrate_device_unmap(migrate->src, migrate->npages,
+ migrate->fault_page);
}
/**
@@ -668,41 +681,36 @@ static void migrate_vma_insert_page(struct migrate_vma *migrate,
*src &= ~MIGRATE_PFN_MIGRATE;
}
-/**
- * migrate_vma_pages() - migrate meta-data from src page to dst page
- * @migrate: migrate struct containing all migration information
- *
- * This migrates struct page meta-data from source struct page to destination
- * struct page. This effectively finishes the migration from source page to the
- * destination page.
- */
-void migrate_vma_pages(struct migrate_vma *migrate)
+static void migrate_device_pages(unsigned long *src_pfns,
+ unsigned long *dst_pfns, unsigned long npages,
+ struct migrate_vma *migrate)
{
- const unsigned long npages = migrate->npages;
- const unsigned long start = migrate->start;
struct mmu_notifier_range range;
- unsigned long addr, i;
+ unsigned long i;
bool notified = false;
- for (i = 0, addr = start; i < npages; addr += PAGE_SIZE, i++) {
- struct page *newpage = migrate_pfn_to_page(migrate->dst[i]);
- struct page *page = migrate_pfn_to_page(migrate->src[i]);
+ for (i = 0; i < npages; i++) {
+ struct page *newpage = migrate_pfn_to_page(dst_pfns[i]);
+ struct page *page = migrate_pfn_to_page(src_pfns[i]);
struct address_space *mapping;
int r;
if (!newpage) {
- migrate->src[i] &= ~MIGRATE_PFN_MIGRATE;
+ src_pfns[i] &= ~MIGRATE_PFN_MIGRATE;
continue;
}
if (!page) {
+ unsigned long addr;
+
/*
* The only time there is no vma is when called from
* migrate_device_coherent_page(). However this isn't
* called if the page could not be unmapped.
*/
- VM_BUG_ON(!migrate->vma);
- if (!(migrate->src[i] & MIGRATE_PFN_MIGRATE))
+ VM_BUG_ON(!migrate);
+ addr = migrate->start + i*PAGE_SIZE;
+ if (!(src_pfns[i] & MIGRATE_PFN_MIGRATE))
continue;
if (!notified) {
notified = true;
@@ -714,7 +722,7 @@ void migrate_vma_pages(struct migrate_vma *migrate)
mmu_notifier_invalidate_range_start(&range);
}
migrate_vma_insert_page(migrate, addr, newpage,
- &migrate->src[i]);
+ &src_pfns[i]);
continue;
}
@@ -727,18 +735,18 @@ void migrate_vma_pages(struct migrate_vma *migrate)
* device private or coherent memory.
*/
if (mapping) {
- migrate->src[i] &= ~MIGRATE_PFN_MIGRATE;
+ src_pfns[i] &= ~MIGRATE_PFN_MIGRATE;
continue;
}
} else if (is_zone_device_page(newpage)) {
/*
* Other types of ZONE_DEVICE page are not supported.
*/
- migrate->src[i] &= ~MIGRATE_PFN_MIGRATE;
+ src_pfns[i] &= ~MIGRATE_PFN_MIGRATE;
continue;
}
- if (migrate->fault_page == page)
+ if (migrate && migrate->fault_page == page)
r = migrate_folio_extra(mapping, page_folio(newpage),
page_folio(page),
MIGRATE_SYNC_NO_COPY, 1);
@@ -746,7 +754,7 @@ void migrate_vma_pages(struct migrate_vma *migrate)
r = migrate_folio(mapping, page_folio(newpage),
page_folio(page), MIGRATE_SYNC_NO_COPY);
if (r != MIGRATEPAGE_SUCCESS)
- migrate->src[i] &= ~MIGRATE_PFN_MIGRATE;
+ src_pfns[i] &= ~MIGRATE_PFN_MIGRATE;
}
/*
@@ -757,28 +765,30 @@ void migrate_vma_pages(struct migrate_vma *migrate)
if (notified)
mmu_notifier_invalidate_range_only_end(&range);
}
-EXPORT_SYMBOL(migrate_vma_pages);
/**
- * migrate_vma_finalize() - restore CPU page table entry
+ * migrate_vma_pages() - migrate meta-data from src page to dst page
* @migrate: migrate struct containing all migration information
*
- * This replaces the special migration pte entry with either a mapping to the
- * new page if migration was successful for that page, or to the original page
- * otherwise.
- *
- * This also unlocks the pages and puts them back on the lru, or drops the extra
- * refcount, for device pages.
+ * This migrates struct page meta-data from source struct page to destination
+ * struct page. This effectively finishes the migration from source page to the
+ * destination page.
*/
-void migrate_vma_finalize(struct migrate_vma *migrate)
+void migrate_vma_pages(struct migrate_vma *migrate)
+{
+ migrate_device_pages(migrate->src, migrate->dst, migrate->npages, migrate);
+}
+EXPORT_SYMBOL(migrate_vma_pages);
+
+static void migrate_device_finalize(unsigned long *src_pfns,
+ unsigned long *dst_pfns, unsigned long npages)
{
- const unsigned long npages = migrate->npages;
unsigned long i;
for (i = 0; i < npages; i++) {
struct folio *dst, *src;
- struct page *newpage = migrate_pfn_to_page(migrate->dst[i]);
- struct page *page = migrate_pfn_to_page(migrate->src[i]);
+ struct page *newpage = migrate_pfn_to_page(dst_pfns[i]);
+ struct page *page = migrate_pfn_to_page(src_pfns[i]);
if (!page) {
if (newpage) {
@@ -788,7 +798,7 @@ void migrate_vma_finalize(struct migrate_vma *migrate)
continue;
}
- if (!(migrate->src[i] & MIGRATE_PFN_MIGRATE) || !newpage) {
+ if (!(src_pfns[i] & MIGRATE_PFN_MIGRATE) || !newpage) {
if (newpage) {
unlock_page(newpage);
put_page(newpage);
@@ -815,6 +825,22 @@ void migrate_vma_finalize(struct migrate_vma *migrate)
}
}
}
+
+/**
+ * migrate_vma_finalize() - restore CPU page table entry
+ * @migrate: migrate struct containing all migration information
+ *
+ * This replaces the special migration pte entry with either a mapping to the
+ * new page if migration was successful for that page, or to the original page
+ * otherwise.
+ *
+ * This also unlocks the pages and puts them back on the lru, or drops the extra
+ * refcount, for device pages.
+ */
+void migrate_vma_finalize(struct migrate_vma *migrate)
+{
+ migrate_device_finalize(migrate->src, migrate->dst, migrate->npages);
+}
EXPORT_SYMBOL(migrate_vma_finalize);
/*
@@ -825,25 +851,19 @@ EXPORT_SYMBOL(migrate_vma_finalize);
int migrate_device_coherent_page(struct page *page)
{
unsigned long src_pfn, dst_pfn = 0;
- struct migrate_vma args;
struct page *dpage;
WARN_ON_ONCE(PageCompound(page));
lock_page(page);
src_pfn = migrate_pfn(page_to_pfn(page)) | MIGRATE_PFN_MIGRATE;
- args.src = &src_pfn;
- args.dst = &dst_pfn;
- args.cpages = 1;
- args.npages = 1;
- args.vma = NULL;
/*
* We don't have a VMA and don't need to walk the page tables to find
* the source page. So call migrate_vma_unmap() directly to unmap the
* page as migrate_vma_setup() will fail if args.vma == NULL.
*/
- migrate_vma_unmap(&args);
+ migrate_device_unmap(&src_pfn, 1, NULL);
if (!(src_pfn & MIGRATE_PFN_MIGRATE))
return -EBUSY;
@@ -853,10 +873,10 @@ int migrate_device_coherent_page(struct page *page)
dst_pfn = migrate_pfn(page_to_pfn(dpage));
}
- migrate_vma_pages(&args);
+ migrate_device_pages(&src_pfn, &dst_pfn, 1, NULL);
if (src_pfn & MIGRATE_PFN_MIGRATE)
copy_highpage(dpage, page);
- migrate_vma_finalize(&args);
+ migrate_device_finalize(&src_pfn, &dst_pfn, 1);
if (src_pfn & MIGRATE_PFN_MIGRATE)
return 0;
--
git-series 0.9.1
Since 27674ef6c73f ("mm: remove the extra ZONE_DEVICE struct page
refcount") device private pages have no longer had an extra reference
count when the page is in use. However before handing them back to the
owning device driver we add an extra reference count such that free
pages have a reference count of one.
This makes it difficult to tell if a page is free or not because both
free and in use pages will have a non-zero refcount. Instead we should
return pages to the drivers page allocator with a zero reference count.
Kernel code can then safely use kernel functions such as
get_page_unless_zero().
Signed-off-by: Alistair Popple <[email protected]>
---
arch/powerpc/kvm/book3s_hv_uvmem.c | 1 +
drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 1 +
drivers/gpu/drm/nouveau/nouveau_dmem.c | 1 +
lib/test_hmm.c | 1 +
mm/memremap.c | 5 -----
mm/page_alloc.c | 6 ++++++
6 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
index d4eacf4..08d2f7d 100644
--- a/arch/powerpc/kvm/book3s_hv_uvmem.c
+++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
@@ -718,6 +718,7 @@ static struct page *kvmppc_uvmem_get_page(unsigned long gpa, struct kvm *kvm)
dpage = pfn_to_page(uvmem_pfn);
dpage->zone_device_data = pvt;
+ set_page_count(dpage, 1);
lock_page(dpage);
return dpage;
out_clear:
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
index 776448b..05c2f4d 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
@@ -223,6 +223,7 @@ svm_migrate_get_vram_page(struct svm_range *prange, unsigned long pfn)
page = pfn_to_page(pfn);
svm_range_bo_ref(prange->svm_bo);
page->zone_device_data = prange->svm_bo;
+ set_page_count(page, 1);
lock_page(page);
}
diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c b/drivers/gpu/drm/nouveau/nouveau_dmem.c
index 1635661..f9234ed 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
@@ -326,6 +326,7 @@ nouveau_dmem_page_alloc_locked(struct nouveau_drm *drm)
return NULL;
}
+ set_page_count(page, 1);
lock_page(page);
return page;
}
diff --git a/lib/test_hmm.c b/lib/test_hmm.c
index 89463ff..2bd3a67 100644
--- a/lib/test_hmm.c
+++ b/lib/test_hmm.c
@@ -627,6 +627,7 @@ static struct page *dmirror_devmem_alloc_page(struct dmirror_device *mdevice)
goto error;
}
+ set_page_count(dpage, 1);
dpage->zone_device_data = rpage;
lock_page(dpage);
return dpage;
diff --git a/mm/memremap.c b/mm/memremap.c
index 25029a4..e065171 100644
--- a/mm/memremap.c
+++ b/mm/memremap.c
@@ -501,11 +501,6 @@ void free_zone_device_page(struct page *page)
*/
page->mapping = NULL;
page->pgmap->ops->page_free(page);
-
- /*
- * Reset the page count to 1 to prepare for handing out the page again.
- */
- set_page_count(page, 1);
}
#ifdef CONFIG_FS_DAX
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 9d49803..67eaab5 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -6744,6 +6744,12 @@ static void __ref __init_zone_device_page(struct page *page, unsigned long pfn,
set_pageblock_migratetype(page, MIGRATE_MOVABLE);
cond_resched();
}
+
+ /*
+ * ZONE_DEVICE pages are released directly to the driver page allocator
+ * which will set the page count to 1 when allocating the page.
+ */
+ set_page_count(page, 0);
}
/*
--
git-series 0.9.1
When the CPU tries to access a device private page the migrate_to_ram()
callback associated with the pgmap for the page is called. However no
reference is taken on the faulting page. Therefore a concurrent
migration of the device private page can free the page and possibly the
underlying pgmap. This results in a race which can crash the kernel due
to the migrate_to_ram() function pointer becoming invalid. It also means
drivers can't reliably read the zone_device_data field because the page
may have been freed with memunmap_pages().
Close the race by getting a reference on the page while holding the ptl
to ensure it has not been freed. Unfortunately the elevated reference
count will cause the migration required to handle the fault to fail. To
avoid this failure pass the faulting page into the migrate_vma functions
so that if an elevated reference count is found it can be checked to see
if it's expected or not.
Signed-off-by: Alistair Popple <[email protected]>
---
arch/powerpc/kvm/book3s_hv_uvmem.c | 15 ++++++-----
drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 17 +++++++------
drivers/gpu/drm/amd/amdkfd/kfd_migrate.h | 2 +-
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 11 +++++---
include/linux/migrate.h | 8 ++++++-
lib/test_hmm.c | 7 ++---
mm/memory.c | 16 +++++++++++-
mm/migrate.c | 34 ++++++++++++++-----------
mm/migrate_device.c | 18 +++++++++----
9 files changed, 87 insertions(+), 41 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
index 5980063..d4eacf4 100644
--- a/arch/powerpc/kvm/book3s_hv_uvmem.c
+++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
@@ -508,10 +508,10 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
static int __kvmppc_svm_page_out(struct vm_area_struct *vma,
unsigned long start,
unsigned long end, unsigned long page_shift,
- struct kvm *kvm, unsigned long gpa)
+ struct kvm *kvm, unsigned long gpa, struct page *fault_page)
{
unsigned long src_pfn, dst_pfn = 0;
- struct migrate_vma mig;
+ struct migrate_vma mig = { 0 };
struct page *dpage, *spage;
struct kvmppc_uvmem_page_pvt *pvt;
unsigned long pfn;
@@ -525,6 +525,7 @@ static int __kvmppc_svm_page_out(struct vm_area_struct *vma,
mig.dst = &dst_pfn;
mig.pgmap_owner = &kvmppc_uvmem_pgmap;
mig.flags = MIGRATE_VMA_SELECT_DEVICE_PRIVATE;
+ mig.fault_page = fault_page;
/* The requested page is already paged-out, nothing to do */
if (!kvmppc_gfn_is_uvmem_pfn(gpa >> page_shift, kvm, NULL))
@@ -580,12 +581,14 @@ static int __kvmppc_svm_page_out(struct vm_area_struct *vma,
static inline int kvmppc_svm_page_out(struct vm_area_struct *vma,
unsigned long start, unsigned long end,
unsigned long page_shift,
- struct kvm *kvm, unsigned long gpa)
+ struct kvm *kvm, unsigned long gpa,
+ struct page *fault_page)
{
int ret;
mutex_lock(&kvm->arch.uvmem_lock);
- ret = __kvmppc_svm_page_out(vma, start, end, page_shift, kvm, gpa);
+ ret = __kvmppc_svm_page_out(vma, start, end, page_shift, kvm, gpa,
+ fault_page);
mutex_unlock(&kvm->arch.uvmem_lock);
return ret;
@@ -736,7 +739,7 @@ static int kvmppc_svm_page_in(struct vm_area_struct *vma,
bool pagein)
{
unsigned long src_pfn, dst_pfn = 0;
- struct migrate_vma mig;
+ struct migrate_vma mig = { 0 };
struct page *spage;
unsigned long pfn;
struct page *dpage;
@@ -994,7 +997,7 @@ static vm_fault_t kvmppc_uvmem_migrate_to_ram(struct vm_fault *vmf)
if (kvmppc_svm_page_out(vmf->vma, vmf->address,
vmf->address + PAGE_SIZE, PAGE_SHIFT,
- pvt->kvm, pvt->gpa))
+ pvt->kvm, pvt->gpa, vmf->page))
return VM_FAULT_SIGBUS;
else
return 0;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
index b059a77..776448b 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
@@ -409,7 +409,7 @@ svm_migrate_vma_to_vram(struct amdgpu_device *adev, struct svm_range *prange,
uint64_t npages = (end - start) >> PAGE_SHIFT;
struct kfd_process_device *pdd;
struct dma_fence *mfence = NULL;
- struct migrate_vma migrate;
+ struct migrate_vma migrate = { 0 };
unsigned long cpages = 0;
dma_addr_t *scratch;
void *buf;
@@ -668,7 +668,7 @@ svm_migrate_copy_to_ram(struct amdgpu_device *adev, struct svm_range *prange,
static long
svm_migrate_vma_to_ram(struct amdgpu_device *adev, struct svm_range *prange,
struct vm_area_struct *vma, uint64_t start, uint64_t end,
- uint32_t trigger)
+ uint32_t trigger, struct page *fault_page)
{
struct kfd_process *p = container_of(prange->svms, struct kfd_process, svms);
uint64_t npages = (end - start) >> PAGE_SHIFT;
@@ -676,7 +676,7 @@ svm_migrate_vma_to_ram(struct amdgpu_device *adev, struct svm_range *prange,
unsigned long cpages = 0;
struct kfd_process_device *pdd;
struct dma_fence *mfence = NULL;
- struct migrate_vma migrate;
+ struct migrate_vma migrate = { 0 };
dma_addr_t *scratch;
void *buf;
int r = -ENOMEM;
@@ -699,6 +699,7 @@ svm_migrate_vma_to_ram(struct amdgpu_device *adev, struct svm_range *prange,
migrate.src = buf;
migrate.dst = migrate.src + npages;
+ migrate.fault_page = fault_page;
scratch = (dma_addr_t *)(migrate.dst + npages);
kfd_smi_event_migration_start(adev->kfd.dev, p->lead_thread->pid,
@@ -766,7 +767,7 @@ svm_migrate_vma_to_ram(struct amdgpu_device *adev, struct svm_range *prange,
* 0 - OK, otherwise error code
*/
int svm_migrate_vram_to_ram(struct svm_range *prange, struct mm_struct *mm,
- uint32_t trigger)
+ uint32_t trigger, struct page *fault_page)
{
struct amdgpu_device *adev;
struct vm_area_struct *vma;
@@ -807,7 +808,8 @@ int svm_migrate_vram_to_ram(struct svm_range *prange, struct mm_struct *mm,
}
next = min(vma->vm_end, end);
- r = svm_migrate_vma_to_ram(adev, prange, vma, addr, next, trigger);
+ r = svm_migrate_vma_to_ram(adev, prange, vma, addr, next, trigger,
+ fault_page);
if (r < 0) {
pr_debug("failed %ld to migrate prange %p\n", r, prange);
break;
@@ -851,7 +853,7 @@ svm_migrate_vram_to_vram(struct svm_range *prange, uint32_t best_loc,
pr_debug("from gpu 0x%x to gpu 0x%x\n", prange->actual_loc, best_loc);
do {
- r = svm_migrate_vram_to_ram(prange, mm, trigger);
+ r = svm_migrate_vram_to_ram(prange, mm, trigger, NULL);
if (r)
return r;
} while (prange->actual_loc && --retries);
@@ -938,7 +940,8 @@ static vm_fault_t svm_migrate_to_ram(struct vm_fault *vmf)
goto out_unlock_prange;
}
- r = svm_migrate_vram_to_ram(prange, mm, KFD_MIGRATE_TRIGGER_PAGEFAULT_CPU);
+ r = svm_migrate_vram_to_ram(prange, mm, KFD_MIGRATE_TRIGGER_PAGEFAULT_CPU,
+ vmf->page);
if (r)
pr_debug("failed %d migrate 0x%p [0x%lx 0x%lx] to ram\n", r,
prange, prange->start, prange->last);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.h b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.h
index b3f0754..a5d7e6d 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.h
@@ -43,7 +43,7 @@ enum MIGRATION_COPY_DIR {
int svm_migrate_to_vram(struct svm_range *prange, uint32_t best_loc,
struct mm_struct *mm, uint32_t trigger);
int svm_migrate_vram_to_ram(struct svm_range *prange, struct mm_struct *mm,
- uint32_t trigger);
+ uint32_t trigger, struct page *fault_page);
unsigned long
svm_migrate_addr_to_pfn(struct amdgpu_device *adev, unsigned long addr);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 11074cc..9139e5a 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -2913,13 +2913,15 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
*/
if (prange->actual_loc)
r = svm_migrate_vram_to_ram(prange, mm,
- KFD_MIGRATE_TRIGGER_PAGEFAULT_GPU);
+ KFD_MIGRATE_TRIGGER_PAGEFAULT_GPU,
+ NULL);
else
r = 0;
}
} else {
r = svm_migrate_vram_to_ram(prange, mm,
- KFD_MIGRATE_TRIGGER_PAGEFAULT_GPU);
+ KFD_MIGRATE_TRIGGER_PAGEFAULT_GPU,
+ NULL);
}
if (r) {
pr_debug("failed %d to migrate svms %p [0x%lx 0x%lx]\n",
@@ -3242,7 +3244,8 @@ svm_range_trigger_migration(struct mm_struct *mm, struct svm_range *prange,
return 0;
if (!best_loc) {
- r = svm_migrate_vram_to_ram(prange, mm, KFD_MIGRATE_TRIGGER_PREFETCH);
+ r = svm_migrate_vram_to_ram(prange, mm,
+ KFD_MIGRATE_TRIGGER_PREFETCH, NULL);
*migrated = !r;
return r;
}
@@ -3303,7 +3306,7 @@ static void svm_range_evict_svm_bo_worker(struct work_struct *work)
mutex_lock(&prange->migrate_mutex);
do {
r = svm_migrate_vram_to_ram(prange, mm,
- KFD_MIGRATE_TRIGGER_TTM_EVICTION);
+ KFD_MIGRATE_TRIGGER_TTM_EVICTION, NULL);
} while (!r && prange->actual_loc && --retries);
if (!r && prange->actual_loc)
diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index 22c0a0c..82ffa47 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -62,6 +62,8 @@ extern const char *migrate_reason_names[MR_TYPES];
#ifdef CONFIG_MIGRATION
extern void putback_movable_pages(struct list_head *l);
+int migrate_folio_extra(struct address_space *mapping, struct folio *dst,
+ struct folio *src, enum migrate_mode mode, int extra_count);
int migrate_folio(struct address_space *mapping, struct folio *dst,
struct folio *src, enum migrate_mode mode);
extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free,
@@ -212,6 +214,12 @@ struct migrate_vma {
*/
void *pgmap_owner;
unsigned long flags;
+
+ /*
+ * Set to vmf->page if this is being called to migrate a page as part of
+ * a migrate_to_ram() callback.
+ */
+ struct page *fault_page;
};
int migrate_vma_setup(struct migrate_vma *args);
diff --git a/lib/test_hmm.c b/lib/test_hmm.c
index e3965ca..89463ff 100644
--- a/lib/test_hmm.c
+++ b/lib/test_hmm.c
@@ -907,7 +907,7 @@ static int dmirror_migrate_to_system(struct dmirror *dmirror,
struct vm_area_struct *vma;
unsigned long src_pfns[64] = { 0 };
unsigned long dst_pfns[64] = { 0 };
- struct migrate_vma args;
+ struct migrate_vma args = { 0 };
unsigned long next;
int ret;
@@ -968,7 +968,7 @@ static int dmirror_migrate_to_device(struct dmirror *dmirror,
unsigned long src_pfns[64] = { 0 };
unsigned long dst_pfns[64] = { 0 };
struct dmirror_bounce bounce;
- struct migrate_vma args;
+ struct migrate_vma args = { 0 };
unsigned long next;
int ret;
@@ -1334,7 +1334,7 @@ static void dmirror_devmem_free(struct page *page)
static vm_fault_t dmirror_devmem_fault(struct vm_fault *vmf)
{
- struct migrate_vma args;
+ struct migrate_vma args = { 0 };
unsigned long src_pfns = 0;
unsigned long dst_pfns = 0;
struct page *rpage;
@@ -1357,6 +1357,7 @@ static vm_fault_t dmirror_devmem_fault(struct vm_fault *vmf)
args.dst = &dst_pfns;
args.pgmap_owner = dmirror->mdevice;
args.flags = dmirror_select_device(dmirror);
+ args.fault_page = vmf->page;
if (migrate_vma_setup(&args))
return VM_FAULT_SIGBUS;
diff --git a/mm/memory.c b/mm/memory.c
index b994784..65d3977 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3742,7 +3742,21 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
ret = remove_device_exclusive_entry(vmf);
} else if (is_device_private_entry(entry)) {
vmf->page = pfn_swap_entry_to_page(entry);
- ret = vmf->page->pgmap->ops->migrate_to_ram(vmf);
+ vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
+ vmf->address, &vmf->ptl);
+ if (unlikely(!pte_same(*vmf->pte, vmf->orig_pte))) {
+ spin_unlock(vmf->ptl);
+ goto out;
+ }
+
+ /*
+ * Get a page reference while we know the page can't be
+ * freed.
+ */
+ get_page(vmf->page);
+ pte_unmap_unlock(vmf->pte, vmf->ptl);
+ vmf->page->pgmap->ops->migrate_to_ram(vmf);
+ put_page(vmf->page);
} else if (is_hwpoison_entry(entry)) {
ret = VM_FAULT_HWPOISON;
} else if (is_swapin_error_entry(entry)) {
diff --git a/mm/migrate.c b/mm/migrate.c
index ce6a58f..e3f78a7 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -620,6 +620,25 @@ EXPORT_SYMBOL(folio_migrate_copy);
* Migration functions
***********************************************************/
+int migrate_folio_extra(struct address_space *mapping, struct folio *dst,
+ struct folio *src, enum migrate_mode mode, int extra_count)
+{
+ int rc;
+
+ BUG_ON(folio_test_writeback(src)); /* Writeback must be complete */
+
+ rc = folio_migrate_mapping(mapping, dst, src, extra_count);
+
+ if (rc != MIGRATEPAGE_SUCCESS)
+ return rc;
+
+ if (mode != MIGRATE_SYNC_NO_COPY)
+ folio_migrate_copy(dst, src);
+ else
+ folio_migrate_flags(dst, src);
+ return MIGRATEPAGE_SUCCESS;
+}
+
/**
* migrate_folio() - Simple folio migration.
* @mapping: The address_space containing the folio.
@@ -635,20 +654,7 @@ EXPORT_SYMBOL(folio_migrate_copy);
int migrate_folio(struct address_space *mapping, struct folio *dst,
struct folio *src, enum migrate_mode mode)
{
- int rc;
-
- BUG_ON(folio_test_writeback(src)); /* Writeback must be complete */
-
- rc = folio_migrate_mapping(mapping, dst, src, 0);
-
- if (rc != MIGRATEPAGE_SUCCESS)
- return rc;
-
- if (mode != MIGRATE_SYNC_NO_COPY)
- folio_migrate_copy(dst, src);
- else
- folio_migrate_flags(dst, src);
- return MIGRATEPAGE_SUCCESS;
+ return migrate_folio_extra(mapping, dst, src, mode, 0);
}
EXPORT_SYMBOL(migrate_folio);
diff --git a/mm/migrate_device.c b/mm/migrate_device.c
index 7235424..f756c00 100644
--- a/mm/migrate_device.c
+++ b/mm/migrate_device.c
@@ -313,14 +313,14 @@ static void migrate_vma_collect(struct migrate_vma *migrate)
* folio_migrate_mapping(), except that here we allow migration of a
* ZONE_DEVICE page.
*/
-static bool migrate_vma_check_page(struct page *page)
+static bool migrate_vma_check_page(struct page *page, struct page *fault_page)
{
/*
* One extra ref because caller holds an extra reference, either from
* isolate_lru_page() for a regular page, or migrate_vma_collect() for
* a device page.
*/
- int extra = 1;
+ int extra = 1 + (page == fault_page);
/*
* FIXME support THP (transparent huge page), it is bit more complex to
@@ -393,7 +393,8 @@ static void migrate_vma_unmap(struct migrate_vma *migrate)
if (folio_mapped(folio))
try_to_migrate(folio, 0);
- if (page_mapped(page) || !migrate_vma_check_page(page)) {
+ if (page_mapped(page) ||
+ !migrate_vma_check_page(page, migrate->fault_page)) {
if (!is_zone_device_page(page)) {
get_page(page);
putback_lru_page(page);
@@ -505,6 +506,8 @@ int migrate_vma_setup(struct migrate_vma *args)
return -EINVAL;
if (!args->src || !args->dst)
return -EINVAL;
+ if (args->fault_page && !is_device_private_page(args->fault_page))
+ return -EINVAL;
memset(args->src, 0, sizeof(*args->src) * nr_pages);
args->cpages = 0;
@@ -735,8 +738,13 @@ void migrate_vma_pages(struct migrate_vma *migrate)
continue;
}
- r = migrate_folio(mapping, page_folio(newpage),
- page_folio(page), MIGRATE_SYNC_NO_COPY);
+ if (migrate->fault_page == page)
+ r = migrate_folio_extra(mapping, page_folio(newpage),
+ page_folio(page),
+ MIGRATE_SYNC_NO_COPY, 1);
+ else
+ r = migrate_folio(mapping, page_folio(newpage),
+ page_folio(page), MIGRATE_SYNC_NO_COPY);
if (r != MIGRATEPAGE_SUCCESS)
migrate->src[i] &= ~MIGRATE_PFN_MIGRATE;
}
--
git-series 0.9.1
Signed-off-by: Alistair Popple <[email protected]>
---
lib/test_hmm.c | 119 +++++++++++++++++++++-----
lib/test_hmm_uapi.h | 1 +-
tools/testing/selftests/vm/hmm-tests.c | 49 +++++++++++-
3 files changed, 148 insertions(+), 21 deletions(-)
diff --git a/lib/test_hmm.c b/lib/test_hmm.c
index 2bd3a67..d2821dd 100644
--- a/lib/test_hmm.c
+++ b/lib/test_hmm.c
@@ -100,6 +100,7 @@ struct dmirror {
struct dmirror_chunk {
struct dev_pagemap pagemap;
struct dmirror_device *mdevice;
+ bool remove;
};
/*
@@ -192,11 +193,15 @@ static int dmirror_fops_release(struct inode *inode, struct file *filp)
return 0;
}
+static struct dmirror_chunk *dmirror_page_to_chunk(struct page *page)
+{
+ return container_of(page->pgmap, struct dmirror_chunk, pagemap);
+}
+
static struct dmirror_device *dmirror_page_to_device(struct page *page)
{
- return container_of(page->pgmap, struct dmirror_chunk,
- pagemap)->mdevice;
+ return dmirror_page_to_chunk(page)->mdevice;
}
static int dmirror_do_fault(struct dmirror *dmirror, struct hmm_range *range)
@@ -1219,6 +1224,84 @@ static int dmirror_snapshot(struct dmirror *dmirror,
return ret;
}
+static void dmirror_device_evict_chunk(struct dmirror_chunk *chunk)
+{
+ unsigned long start_pfn = chunk->pagemap.range.start >> PAGE_SHIFT;
+ unsigned long end_pfn = chunk->pagemap.range.end >> PAGE_SHIFT;
+ unsigned long npages = end_pfn - start_pfn + 1;
+ unsigned long i;
+ unsigned long *src_pfns;
+ unsigned long *dst_pfns;
+
+ src_pfns = kcalloc(npages, sizeof(*src_pfns), GFP_KERNEL);
+ dst_pfns = kcalloc(npages, sizeof(*dst_pfns), GFP_KERNEL);
+
+ migrate_device_range(src_pfns, start_pfn, npages);
+ for (i = 0; i < npages; i++) {
+ struct page *dpage, *spage;
+
+ spage = migrate_pfn_to_page(src_pfns[i]);
+ if (!spage || !(src_pfns[i] & MIGRATE_PFN_MIGRATE))
+ continue;
+
+ if (WARN_ON(!is_device_private_page(spage) &&
+ !is_device_coherent_page(spage)))
+ continue;
+ spage = BACKING_PAGE(spage);
+ dpage = alloc_page(GFP_HIGHUSER_MOVABLE | __GFP_NOFAIL);
+ lock_page(dpage);
+ copy_highpage(dpage, spage);
+ dst_pfns[i] = migrate_pfn(page_to_pfn(dpage));
+ if (src_pfns[i] & MIGRATE_PFN_WRITE)
+ dst_pfns[i] |= MIGRATE_PFN_WRITE;
+ }
+ migrate_device_pages(src_pfns, dst_pfns, npages);
+ migrate_device_finalize(src_pfns, dst_pfns, npages);
+ kfree(src_pfns);
+ kfree(dst_pfns);
+}
+
+/* Removes free pages from the free list so they can't be re-allocated */
+static void dmirror_remove_free_pages(struct dmirror_chunk *devmem)
+{
+ struct dmirror_device *mdevice = devmem->mdevice;
+ struct page *page;
+
+ for (page = mdevice->free_pages; page; page = page->zone_device_data)
+ if (dmirror_page_to_chunk(page) == devmem)
+ mdevice->free_pages = page->zone_device_data;
+}
+
+static void dmirror_device_remove_chunks(struct dmirror_device *mdevice)
+{
+ unsigned int i;
+
+ mutex_lock(&mdevice->devmem_lock);
+ if (mdevice->devmem_chunks) {
+ for (i = 0; i < mdevice->devmem_count; i++) {
+ struct dmirror_chunk *devmem =
+ mdevice->devmem_chunks[i];
+
+ spin_lock(&mdevice->lock);
+ devmem->remove = true;
+ dmirror_remove_free_pages(devmem);
+ spin_unlock(&mdevice->lock);
+
+ dmirror_device_evict_chunk(devmem);
+ memunmap_pages(&devmem->pagemap);
+ if (devmem->pagemap.type == MEMORY_DEVICE_PRIVATE)
+ release_mem_region(devmem->pagemap.range.start,
+ range_len(&devmem->pagemap.range));
+ kfree(devmem);
+ }
+ mdevice->devmem_count = 0;
+ mdevice->devmem_capacity = 0;
+ mdevice->free_pages = NULL;
+ kfree(mdevice->devmem_chunks);
+ }
+ mutex_unlock(&mdevice->devmem_lock);
+}
+
static long dmirror_fops_unlocked_ioctl(struct file *filp,
unsigned int command,
unsigned long arg)
@@ -1273,6 +1356,11 @@ static long dmirror_fops_unlocked_ioctl(struct file *filp,
ret = dmirror_snapshot(dmirror, &cmd);
break;
+ case HMM_DMIRROR_RELEASE:
+ dmirror_device_remove_chunks(dmirror->mdevice);
+ ret = 0;
+ break;
+
default:
return -EINVAL;
}
@@ -1327,9 +1415,13 @@ static void dmirror_devmem_free(struct page *page)
mdevice = dmirror_page_to_device(page);
spin_lock(&mdevice->lock);
- mdevice->cfree++;
- page->zone_device_data = mdevice->free_pages;
- mdevice->free_pages = page;
+
+ /* Return page to our allocator if not freeing the chunk */
+ if (!dmirror_page_to_chunk(page)->remove) {
+ mdevice->cfree++;
+ page->zone_device_data = mdevice->free_pages;
+ mdevice->free_pages = page;
+ }
spin_unlock(&mdevice->lock);
}
@@ -1402,22 +1494,7 @@ static int dmirror_device_init(struct dmirror_device *mdevice, int id)
static void dmirror_device_remove(struct dmirror_device *mdevice)
{
- unsigned int i;
-
- if (mdevice->devmem_chunks) {
- for (i = 0; i < mdevice->devmem_count; i++) {
- struct dmirror_chunk *devmem =
- mdevice->devmem_chunks[i];
-
- memunmap_pages(&devmem->pagemap);
- if (devmem->pagemap.type == MEMORY_DEVICE_PRIVATE)
- release_mem_region(devmem->pagemap.range.start,
- range_len(&devmem->pagemap.range));
- kfree(devmem);
- }
- kfree(mdevice->devmem_chunks);
- }
-
+ dmirror_device_remove_chunks(mdevice);
cdev_del(&mdevice->cdevice);
}
diff --git a/lib/test_hmm_uapi.h b/lib/test_hmm_uapi.h
index e31d58c..8c818a2 100644
--- a/lib/test_hmm_uapi.h
+++ b/lib/test_hmm_uapi.h
@@ -36,6 +36,7 @@ struct hmm_dmirror_cmd {
#define HMM_DMIRROR_SNAPSHOT _IOWR('H', 0x04, struct hmm_dmirror_cmd)
#define HMM_DMIRROR_EXCLUSIVE _IOWR('H', 0x05, struct hmm_dmirror_cmd)
#define HMM_DMIRROR_CHECK_EXCLUSIVE _IOWR('H', 0x06, struct hmm_dmirror_cmd)
+#define HMM_DMIRROR_RELEASE _IOWR('H', 0x07, struct hmm_dmirror_cmd)
/*
* Values returned in hmm_dmirror_cmd.ptr for HMM_DMIRROR_SNAPSHOT.
diff --git a/tools/testing/selftests/vm/hmm-tests.c b/tools/testing/selftests/vm/hmm-tests.c
index f2c2c97..28232ad 100644
--- a/tools/testing/selftests/vm/hmm-tests.c
+++ b/tools/testing/selftests/vm/hmm-tests.c
@@ -1054,6 +1054,55 @@ TEST_F(hmm, migrate_fault)
hmm_buffer_free(buffer);
}
+TEST_F(hmm, migrate_release)
+{
+ struct hmm_buffer *buffer;
+ unsigned long npages;
+ unsigned long size;
+ unsigned long i;
+ int *ptr;
+ int ret;
+
+ npages = ALIGN(HMM_BUFFER_SIZE, self->page_size) >> self->page_shift;
+ ASSERT_NE(npages, 0);
+ size = npages << self->page_shift;
+
+ buffer = malloc(sizeof(*buffer));
+ ASSERT_NE(buffer, NULL);
+
+ buffer->fd = -1;
+ buffer->size = size;
+ buffer->mirror = malloc(size);
+ ASSERT_NE(buffer->mirror, NULL);
+
+ buffer->ptr = mmap(NULL, size, PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANONYMOUS, buffer->fd, 0);
+ ASSERT_NE(buffer->ptr, MAP_FAILED);
+
+ /* Initialize buffer in system memory. */
+ for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i)
+ ptr[i] = i;
+
+ /* Migrate memory to device. */
+ ret = hmm_migrate_sys_to_dev(self->fd, buffer, npages);
+ ASSERT_EQ(ret, 0);
+ ASSERT_EQ(buffer->cpages, npages);
+
+ /* Check what the device read. */
+ for (i = 0, ptr = buffer->mirror; i < size / sizeof(*ptr); ++i)
+ ASSERT_EQ(ptr[i], i);
+
+ /* Release device memory. */
+ ret = hmm_dmirror_cmd(self->fd, HMM_DMIRROR_RELEASE, buffer, npages);
+ ASSERT_EQ(ret, 0);
+
+ /* Fault pages back to system memory and check them. */
+ for (i = 0, ptr = buffer->ptr; i < size / (2 * sizeof(*ptr)); ++i)
+ ASSERT_EQ(ptr[i], i);
+
+ hmm_buffer_free(buffer);
+}
+
/*
* Migrate anonymous shared memory to device private memory.
*/
--
git-series 0.9.1
When the module is unloaded or a GPU is unbound from the module it is
possible for device private pages to be left mapped in currently running
processes. This leads to a kernel crash when the pages are either freed
or accessed from the CPU because the GPU and associated data structures
and callbacks have all been freed.
Fix this by migrating any mappings back to normal CPU memory prior to
freeing the GPU memory chunks and associated device private pages.
Signed-off-by: Alistair Popple <[email protected]>
---
I assume the AMD driver might have a similar issue. However I can't see
where device private (or coherent) pages actually get unmapped/freed
during teardown as I couldn't find any relevant calls to
devm_memunmap(), memunmap(), devm_release_mem_region() or
release_mem_region(). So it appears that ZONE_DEVICE pages are not being
properly freed during module unload, unless I'm missing something?
---
drivers/gpu/drm/nouveau/nouveau_dmem.c | 48 +++++++++++++++++++++++++++-
1 file changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c b/drivers/gpu/drm/nouveau/nouveau_dmem.c
index 66ebbd4..3b247b8 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
@@ -369,6 +369,52 @@ nouveau_dmem_suspend(struct nouveau_drm *drm)
mutex_unlock(&drm->dmem->mutex);
}
+/*
+ * Evict all pages mapping a chunk.
+ */
+void
+nouveau_dmem_evict_chunk(struct nouveau_dmem_chunk *chunk)
+{
+ unsigned long i, npages = range_len(&chunk->pagemap.range) >> PAGE_SHIFT;
+ unsigned long *src_pfns, *dst_pfns;
+ dma_addr_t *dma_addrs;
+ struct nouveau_fence *fence;
+
+ src_pfns = kcalloc(npages, sizeof(*src_pfns), GFP_KERNEL);
+ dst_pfns = kcalloc(npages, sizeof(*dst_pfns), GFP_KERNEL);
+ dma_addrs = kcalloc(npages, sizeof(*dma_addrs), GFP_KERNEL);
+
+ migrate_device_range(src_pfns, chunk->pagemap.range.start >> PAGE_SHIFT,
+ npages);
+
+ for (i = 0; i < npages; i++) {
+ if (src_pfns[i] & MIGRATE_PFN_MIGRATE) {
+ struct page *dpage;
+
+ /*
+ * _GFP_NOFAIL because the GPU is going away and there
+ * is nothing sensible we can do if we can't copy the
+ * data back.
+ */
+ dpage = alloc_page(GFP_HIGHUSER | __GFP_NOFAIL);
+ dst_pfns[i] = migrate_pfn(page_to_pfn(dpage));
+ nouveau_dmem_copy_one(chunk->drm,
+ migrate_pfn_to_page(src_pfns[i]), dpage,
+ &dma_addrs[i]);
+ }
+ }
+
+ nouveau_fence_new(chunk->drm->dmem->migrate.chan, false, &fence);
+ migrate_device_pages(src_pfns, dst_pfns, npages);
+ nouveau_dmem_fence_done(&fence);
+ migrate_device_finalize(src_pfns, dst_pfns, npages);
+ kfree(src_pfns);
+ kfree(dst_pfns);
+ for (i = 0; i < npages; i++)
+ dma_unmap_page(chunk->drm->dev->dev, dma_addrs[i], PAGE_SIZE, DMA_BIDIRECTIONAL);
+ kfree(dma_addrs);
+}
+
void
nouveau_dmem_fini(struct nouveau_drm *drm)
{
@@ -380,8 +426,10 @@ nouveau_dmem_fini(struct nouveau_drm *drm)
mutex_lock(&drm->dmem->mutex);
list_for_each_entry_safe(chunk, tmp, &drm->dmem->chunks, list) {
+ nouveau_dmem_evict_chunk(chunk);
nouveau_bo_unpin(chunk->bo);
nouveau_bo_ref(NULL, &chunk->bo);
+ WARN_ON(chunk->callocated);
list_del(&chunk->list);
memunmap_pages(&chunk->pagemap);
release_mem_region(chunk->pagemap.range.start,
--
git-series 0.9.1
nouveau_dmem_fault_copy_one() is used during handling of CPU faults via
the migrate_to_ram() callback and is used to copy data from GPU to CPU
memory. It is currently specific to fault handling, however a future
patch implementing eviction of data during teardown needs similar
functionality.
Refactor out the core functionality so that it is not specific to fault
handling.
Signed-off-by: Alistair Popple <[email protected]>
---
drivers/gpu/drm/nouveau/nouveau_dmem.c | 59 +++++++++++++--------------
1 file changed, 29 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c b/drivers/gpu/drm/nouveau/nouveau_dmem.c
index f9234ed..66ebbd4 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
@@ -139,44 +139,25 @@ static void nouveau_dmem_fence_done(struct nouveau_fence **fence)
}
}
-static vm_fault_t nouveau_dmem_fault_copy_one(struct nouveau_drm *drm,
- struct vm_fault *vmf, struct migrate_vma *args,
- dma_addr_t *dma_addr)
+static int nouveau_dmem_copy_one(struct nouveau_drm *drm, struct page *spage,
+ struct page *dpage, dma_addr_t *dma_addr)
{
struct device *dev = drm->dev->dev;
- struct page *dpage, *spage;
- struct nouveau_svmm *svmm;
-
- spage = migrate_pfn_to_page(args->src[0]);
- if (!spage || !(args->src[0] & MIGRATE_PFN_MIGRATE))
- return 0;
- dpage = alloc_page_vma(GFP_HIGHUSER, vmf->vma, vmf->address);
- if (!dpage)
- return VM_FAULT_SIGBUS;
lock_page(dpage);
*dma_addr = dma_map_page(dev, dpage, 0, PAGE_SIZE, DMA_BIDIRECTIONAL);
if (dma_mapping_error(dev, *dma_addr))
- goto error_free_page;
+ return -EIO;
- svmm = spage->zone_device_data;
- mutex_lock(&svmm->mutex);
- nouveau_svmm_invalidate(svmm, args->start, args->end);
if (drm->dmem->migrate.copy_func(drm, 1, NOUVEAU_APER_HOST, *dma_addr,
- NOUVEAU_APER_VRAM, nouveau_dmem_page_addr(spage)))
- goto error_dma_unmap;
- mutex_unlock(&svmm->mutex);
+ NOUVEAU_APER_VRAM,
+ nouveau_dmem_page_addr(spage))) {
+ dma_unmap_page(dev, *dma_addr, PAGE_SIZE, DMA_BIDIRECTIONAL);
+ return -EIO;
+ }
- args->dst[0] = migrate_pfn(page_to_pfn(dpage));
return 0;
-
-error_dma_unmap:
- mutex_unlock(&svmm->mutex);
- dma_unmap_page(dev, *dma_addr, PAGE_SIZE, DMA_BIDIRECTIONAL);
-error_free_page:
- __free_page(dpage);
- return VM_FAULT_SIGBUS;
}
static vm_fault_t nouveau_dmem_migrate_to_ram(struct vm_fault *vmf)
@@ -184,9 +165,11 @@ static vm_fault_t nouveau_dmem_migrate_to_ram(struct vm_fault *vmf)
struct nouveau_drm *drm = page_to_drm(vmf->page);
struct nouveau_dmem *dmem = drm->dmem;
struct nouveau_fence *fence;
+ struct nouveau_svmm *svmm;
+ struct page *spage, *dpage;
unsigned long src = 0, dst = 0;
dma_addr_t dma_addr = 0;
- vm_fault_t ret;
+ vm_fault_t ret = 0;
struct migrate_vma args = {
.vma = vmf->vma,
.start = vmf->address,
@@ -207,9 +190,25 @@ static vm_fault_t nouveau_dmem_migrate_to_ram(struct vm_fault *vmf)
if (!args.cpages)
return 0;
- ret = nouveau_dmem_fault_copy_one(drm, vmf, &args, &dma_addr);
- if (ret || dst == 0)
+ spage = migrate_pfn_to_page(src);
+ if (!spage || !(src & MIGRATE_PFN_MIGRATE))
+ goto done;
+
+ dpage = alloc_page_vma(GFP_HIGHUSER, vmf->vma, vmf->address);
+ if (!dpage)
+ goto done;
+
+ dst = migrate_pfn(page_to_pfn(dpage));
+
+ svmm = spage->zone_device_data;
+ mutex_lock(&svmm->mutex);
+ nouveau_svmm_invalidate(svmm, args.start, args.end);
+ ret = nouveau_dmem_copy_one(drm, spage, dpage, &dma_addr);
+ mutex_unlock(&svmm->mutex);
+ if (ret) {
+ ret = VM_FAULT_SIGBUS;
goto done;
+ }
nouveau_fence_new(dmem->migrate.chan, false, &fence);
migrate_vma_pages(&args);
--
git-series 0.9.1
Hi Alistair,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on 088b8aa537c2c767765f1c19b555f21ffe555786]
url: https://github.com/intel-lab-lkp/linux/commits/Alistair-Popple/Fix-several-device-private-page-reference-counting-issues/20220926-140640
base: 088b8aa537c2c767765f1c19b555f21ffe555786
config: arm64-allyesconfig
compiler: aarch64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/e99dcd1c3eb07d0787b26d7df178659aec9e3c74
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Alistair-Popple/Fix-several-device-private-page-reference-counting-issues/20220926-140640
git checkout e99dcd1c3eb07d0787b26d7df178659aec9e3c74
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/gpu/drm/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/nouveau/nouveau_dmem.c:376:1: warning: no previous prototype for 'nouveau_dmem_evict_chunk' [-Wmissing-prototypes]
376 | nouveau_dmem_evict_chunk(struct nouveau_dmem_chunk *chunk)
| ^~~~~~~~~~~~~~~~~~~~~~~~
vim +/nouveau_dmem_evict_chunk +376 drivers/gpu/drm/nouveau/nouveau_dmem.c
371
372 /*
373 * Evict all pages mapping a chunk.
374 */
375 void
> 376 nouveau_dmem_evict_chunk(struct nouveau_dmem_chunk *chunk)
377 {
378 unsigned long i, npages = range_len(&chunk->pagemap.range) >> PAGE_SHIFT;
379 unsigned long *src_pfns, *dst_pfns;
380 dma_addr_t *dma_addrs;
381 struct nouveau_fence *fence;
382
383 src_pfns = kcalloc(npages, sizeof(*src_pfns), GFP_KERNEL);
384 dst_pfns = kcalloc(npages, sizeof(*dst_pfns), GFP_KERNEL);
385 dma_addrs = kcalloc(npages, sizeof(*dma_addrs), GFP_KERNEL);
386
387 migrate_device_range(src_pfns, chunk->pagemap.range.start >> PAGE_SHIFT,
388 npages);
389
390 for (i = 0; i < npages; i++) {
391 if (src_pfns[i] & MIGRATE_PFN_MIGRATE) {
392 struct page *dpage;
393
394 /*
395 * _GFP_NOFAIL because the GPU is going away and there
396 * is nothing sensible we can do if we can't copy the
397 * data back.
398 */
399 dpage = alloc_page(GFP_HIGHUSER | __GFP_NOFAIL);
400 dst_pfns[i] = migrate_pfn(page_to_pfn(dpage));
401 nouveau_dmem_copy_one(chunk->drm,
402 migrate_pfn_to_page(src_pfns[i]), dpage,
403 &dma_addrs[i]);
404 }
405 }
406
407 nouveau_fence_new(chunk->drm->dmem->migrate.chan, false, &fence);
408 migrate_device_pages(src_pfns, dst_pfns, npages);
409 nouveau_dmem_fence_done(&fence);
410 migrate_device_finalize(src_pfns, dst_pfns, npages);
411 kfree(src_pfns);
412 kfree(dst_pfns);
413 for (i = 0; i < npages; i++)
414 dma_unmap_page(chunk->drm->dev->dev, dma_addrs[i], PAGE_SIZE, DMA_BIDIRECTIONAL);
415 kfree(dma_addrs);
416 }
417
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Mon, Sep 26, 2022 at 04:03:06PM +1000, Alistair Popple wrote:
> Since 27674ef6c73f ("mm: remove the extra ZONE_DEVICE struct page
> refcount") device private pages have no longer had an extra reference
> count when the page is in use. However before handing them back to the
> owning device driver we add an extra reference count such that free
> pages have a reference count of one.
>
> This makes it difficult to tell if a page is free or not because both
> free and in use pages will have a non-zero refcount. Instead we should
> return pages to the drivers page allocator with a zero reference count.
> Kernel code can then safely use kernel functions such as
> get_page_unless_zero().
>
> Signed-off-by: Alistair Popple <[email protected]>
> ---
> arch/powerpc/kvm/book3s_hv_uvmem.c | 1 +
> drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 1 +
> drivers/gpu/drm/nouveau/nouveau_dmem.c | 1 +
> lib/test_hmm.c | 1 +
> mm/memremap.c | 5 -----
> mm/page_alloc.c | 6 ++++++
> 6 files changed, 10 insertions(+), 5 deletions(-)
I think this is a great idea, but I'm surprised no dax stuff is
touched here?
Jason
On Mon, 2022-09-26 at 16:03 +1000, Alistair Popple wrote:
> nouveau_dmem_fault_copy_one() is used during handling of CPU faults via
> the migrate_to_ram() callback and is used to copy data from GPU to CPU
> memory. It is currently specific to fault handling, however a future
> patch implementing eviction of data during teardown needs similar
> functionality.
>
> Refactor out the core functionality so that it is not specific to fault
> handling.
>
> Signed-off-by: Alistair Popple <[email protected]>
> ---
> drivers/gpu/drm/nouveau/nouveau_dmem.c | 59 +++++++++++++--------------
> 1 file changed, 29 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c b/drivers/gpu/drm/nouveau/nouveau_dmem.c
> index f9234ed..66ebbd4 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
> @@ -139,44 +139,25 @@ static void nouveau_dmem_fence_done(struct nouveau_fence **fence)
> }
> }
>
> -static vm_fault_t nouveau_dmem_fault_copy_one(struct nouveau_drm *drm,
> - struct vm_fault *vmf, struct migrate_vma *args,
> - dma_addr_t *dma_addr)
> +static int nouveau_dmem_copy_one(struct nouveau_drm *drm, struct page *spage,
> + struct page *dpage, dma_addr_t *dma_addr)
> {
> struct device *dev = drm->dev->dev;
> - struct page *dpage, *spage;
> - struct nouveau_svmm *svmm;
> -
> - spage = migrate_pfn_to_page(args->src[0]);
> - if (!spage || !(args->src[0] & MIGRATE_PFN_MIGRATE))
> - return 0;
>
> - dpage = alloc_page_vma(GFP_HIGHUSER, vmf->vma, vmf->address);
> - if (!dpage)
> - return VM_FAULT_SIGBUS;
> lock_page(dpage);
>
> *dma_addr = dma_map_page(dev, dpage, 0, PAGE_SIZE, DMA_BIDIRECTIONAL);
> if (dma_mapping_error(dev, *dma_addr))
> - goto error_free_page;
> + return -EIO;
>
> - svmm = spage->zone_device_data;
> - mutex_lock(&svmm->mutex);
> - nouveau_svmm_invalidate(svmm, args->start, args->end);
> if (drm->dmem->migrate.copy_func(drm, 1, NOUVEAU_APER_HOST, *dma_addr,
> - NOUVEAU_APER_VRAM, nouveau_dmem_page_addr(spage)))
> - goto error_dma_unmap;
> - mutex_unlock(&svmm->mutex);
> + NOUVEAU_APER_VRAM,
> + nouveau_dmem_page_addr(spage))) {
> + dma_unmap_page(dev, *dma_addr, PAGE_SIZE, DMA_BIDIRECTIONAL);
> + return -EIO;
> + }
Feel free to just align this with the starting (, as long as it doesn't go
above 100 characters it doesn't really matter imho and would look nicer that
way.
Otherwise:
Reviewed-by: Lyude Paul <[email protected]>
Will look at the other patch in a moment
>
> - args->dst[0] = migrate_pfn(page_to_pfn(dpage));
> return 0;
> -
> -error_dma_unmap:
> - mutex_unlock(&svmm->mutex);
> - dma_unmap_page(dev, *dma_addr, PAGE_SIZE, DMA_BIDIRECTIONAL);
> -error_free_page:
> - __free_page(dpage);
> - return VM_FAULT_SIGBUS;
> }
>
> static vm_fault_t nouveau_dmem_migrate_to_ram(struct vm_fault *vmf)
> @@ -184,9 +165,11 @@ static vm_fault_t nouveau_dmem_migrate_to_ram(struct vm_fault *vmf)
> struct nouveau_drm *drm = page_to_drm(vmf->page);
> struct nouveau_dmem *dmem = drm->dmem;
> struct nouveau_fence *fence;
> + struct nouveau_svmm *svmm;
> + struct page *spage, *dpage;
> unsigned long src = 0, dst = 0;
> dma_addr_t dma_addr = 0;
> - vm_fault_t ret;
> + vm_fault_t ret = 0;
> struct migrate_vma args = {
> .vma = vmf->vma,
> .start = vmf->address,
> @@ -207,9 +190,25 @@ static vm_fault_t nouveau_dmem_migrate_to_ram(struct vm_fault *vmf)
> if (!args.cpages)
> return 0;
>
> - ret = nouveau_dmem_fault_copy_one(drm, vmf, &args, &dma_addr);
> - if (ret || dst == 0)
> + spage = migrate_pfn_to_page(src);
> + if (!spage || !(src & MIGRATE_PFN_MIGRATE))
> + goto done;
> +
> + dpage = alloc_page_vma(GFP_HIGHUSER, vmf->vma, vmf->address);
> + if (!dpage)
> + goto done;
> +
> + dst = migrate_pfn(page_to_pfn(dpage));
> +
> + svmm = spage->zone_device_data;
> + mutex_lock(&svmm->mutex);
> + nouveau_svmm_invalidate(svmm, args.start, args.end);
> + ret = nouveau_dmem_copy_one(drm, spage, dpage, &dma_addr);
> + mutex_unlock(&svmm->mutex);
> + if (ret) {
> + ret = VM_FAULT_SIGBUS;
> goto done;
> + }
>
> nouveau_fence_new(dmem->migrate.chan, false, &fence);
> migrate_vma_pages(&args);
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
On Mon, 2022-09-26 at 16:03 +1000, Alistair Popple wrote:
> When the module is unloaded or a GPU is unbound from the module it is
> possible for device private pages to be left mapped in currently running
> processes. This leads to a kernel crash when the pages are either freed
> or accessed from the CPU because the GPU and associated data structures
> and callbacks have all been freed.
>
> Fix this by migrating any mappings back to normal CPU memory prior to
> freeing the GPU memory chunks and associated device private pages.
>
> Signed-off-by: Alistair Popple <[email protected]>
>
> ---
>
> I assume the AMD driver might have a similar issue. However I can't see
> where device private (or coherent) pages actually get unmapped/freed
> during teardown as I couldn't find any relevant calls to
> devm_memunmap(), memunmap(), devm_release_mem_region() or
> release_mem_region(). So it appears that ZONE_DEVICE pages are not being
> properly freed during module unload, unless I'm missing something?
I've got no idea, will poke Ben to see if they know the answer to this
> ---
> drivers/gpu/drm/nouveau/nouveau_dmem.c | 48 +++++++++++++++++++++++++++-
> 1 file changed, 48 insertions(+)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c b/drivers/gpu/drm/nouveau/nouveau_dmem.c
> index 66ebbd4..3b247b8 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
> @@ -369,6 +369,52 @@ nouveau_dmem_suspend(struct nouveau_drm *drm)
> mutex_unlock(&drm->dmem->mutex);
> }
>
> +/*
> + * Evict all pages mapping a chunk.
> + */
> +void
> +nouveau_dmem_evict_chunk(struct nouveau_dmem_chunk *chunk)
> +{
> + unsigned long i, npages = range_len(&chunk->pagemap.range) >> PAGE_SHIFT;
> + unsigned long *src_pfns, *dst_pfns;
> + dma_addr_t *dma_addrs;
> + struct nouveau_fence *fence;
> +
> + src_pfns = kcalloc(npages, sizeof(*src_pfns), GFP_KERNEL);
> + dst_pfns = kcalloc(npages, sizeof(*dst_pfns), GFP_KERNEL);
> + dma_addrs = kcalloc(npages, sizeof(*dma_addrs), GFP_KERNEL);
> +
> + migrate_device_range(src_pfns, chunk->pagemap.range.start >> PAGE_SHIFT,
> + npages);
> +
> + for (i = 0; i < npages; i++) {
> + if (src_pfns[i] & MIGRATE_PFN_MIGRATE) {
> + struct page *dpage;
> +
> + /*
> + * _GFP_NOFAIL because the GPU is going away and there
> + * is nothing sensible we can do if we can't copy the
> + * data back.
> + */
You'll have to excuse me for a moment since this area of nouveau isn't one of
my strongpoints, but are we sure about this? IIRC __GFP_NOFAIL means infinite
retry, in the case of a GPU hotplug event I would assume we would rather just
stop trying to migrate things to the GPU and just drop the data instead of
hanging on infinite retries.
> + dpage = alloc_page(GFP_HIGHUSER | __GFP_NOFAIL);
> + dst_pfns[i] = migrate_pfn(page_to_pfn(dpage));
> + nouveau_dmem_copy_one(chunk->drm,
> + migrate_pfn_to_page(src_pfns[i]), dpage,
> + &dma_addrs[i]);
> + }
> + }
> +
> + nouveau_fence_new(chunk->drm->dmem->migrate.chan, false, &fence);
> + migrate_device_pages(src_pfns, dst_pfns, npages);
> + nouveau_dmem_fence_done(&fence);
> + migrate_device_finalize(src_pfns, dst_pfns, npages);
> + kfree(src_pfns);
> + kfree(dst_pfns);
> + for (i = 0; i < npages; i++)
> + dma_unmap_page(chunk->drm->dev->dev, dma_addrs[i], PAGE_SIZE, DMA_BIDIRECTIONAL);
> + kfree(dma_addrs);
> +}
> +
> void
> nouveau_dmem_fini(struct nouveau_drm *drm)
> {
> @@ -380,8 +426,10 @@ nouveau_dmem_fini(struct nouveau_drm *drm)
> mutex_lock(&drm->dmem->mutex);
>
> list_for_each_entry_safe(chunk, tmp, &drm->dmem->chunks, list) {
> + nouveau_dmem_evict_chunk(chunk);
> nouveau_bo_unpin(chunk->bo);
> nouveau_bo_ref(NULL, &chunk->bo);
> + WARN_ON(chunk->callocated);
> list_del(&chunk->list);
> memunmap_pages(&chunk->pagemap);
> release_mem_region(chunk->pagemap.range.start,
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
On 9/26/22 14:35, Lyude Paul wrote:
>> + for (i = 0; i < npages; i++) {
>> + if (src_pfns[i] & MIGRATE_PFN_MIGRATE) {
>> + struct page *dpage;
>> +
>> + /*
>> + * _GFP_NOFAIL because the GPU is going away and there
>> + * is nothing sensible we can do if we can't copy the
>> + * data back.
>> + */
>
> You'll have to excuse me for a moment since this area of nouveau isn't one of
> my strongpoints, but are we sure about this? IIRC __GFP_NOFAIL means infinite
> retry, in the case of a GPU hotplug event I would assume we would rather just
> stop trying to migrate things to the GPU and just drop the data instead of
> hanging on infinite retries.
>
Hi Lyude!
Actually, I really think it's better in this case to keep trying
(presumably not necessarily infinitely, but only until memory becomes
available), rather than failing out and corrupting data.
That's because I'm not sure it's completely clear that this memory is
discardable. And at some point, we're going to make this all work with
file-backed memory, which will *definitely* not be discardable--I
realize that we're not there yet, of course.
But here, it's reasonable to commit to just retrying indefinitely,
really. Memory should eventually show up. And if it doesn't, then
restarting the machine is better than corrupting data, generally.
thanks,
--
John Hubbard
NVIDIA
On 2022-09-26 17:35, Lyude Paul wrote:
> On Mon, 2022-09-26 at 16:03 +1000, Alistair Popple wrote:
>> When the module is unloaded or a GPU is unbound from the module it is
>> possible for device private pages to be left mapped in currently running
>> processes. This leads to a kernel crash when the pages are either freed
>> or accessed from the CPU because the GPU and associated data structures
>> and callbacks have all been freed.
>>
>> Fix this by migrating any mappings back to normal CPU memory prior to
>> freeing the GPU memory chunks and associated device private pages.
>>
>> Signed-off-by: Alistair Popple <[email protected]>
>>
>> ---
>>
>> I assume the AMD driver might have a similar issue. However I can't see
>> where device private (or coherent) pages actually get unmapped/freed
>> during teardown as I couldn't find any relevant calls to
>> devm_memunmap(), memunmap(), devm_release_mem_region() or
>> release_mem_region(). So it appears that ZONE_DEVICE pages are not being
>> properly freed during module unload, unless I'm missing something?
> I've got no idea, will poke Ben to see if they know the answer to this
I guess we're relying on devm to release the region. Isn't the whole
point of using devm_request_free_mem_region that we don't have to
remember to explicitly release it when the device gets destroyed? I
believe we had an explicit free call at some point by mistake, and that
caused a double-free during module unload. See this commit for reference:
commit 22f4f4faf337d5fb2d2750aff13215726814273e
Author: Philip Yang <[email protected]>
Date: Mon Sep 20 17:25:52 2021 -0400
drm/amdkfd: fix svm_migrate_fini warning
Device manager releases device-specific resources when a driver
disconnects from a device, devm_memunmap_pages and
devm_release_mem_region calls in svm_migrate_fini are redundant.
It causes below warning trace after patch "drm/amdgpu: Split
amdgpu_device_fini into early and late", so remove function
svm_migrate_fini.
BUG: https://gitlab.freedesktop.org/drm/amd/-/issues/1718
WARNING: CPU: 1 PID: 3646 at drivers/base/devres.c:795
devm_release_action+0x51/0x60
Call Trace:
? memunmap_pages+0x360/0x360
svm_migrate_fini+0x2d/0x60 [amdgpu]
kgd2kfd_device_exit+0x23/0xa0 [amdgpu]
amdgpu_amdkfd_device_fini_sw+0x1d/0x30 [amdgpu]
amdgpu_device_fini_sw+0x45/0x290 [amdgpu]
amdgpu_driver_release_kms+0x12/0x30 [amdgpu]
drm_dev_release+0x20/0x40 [drm]
release_nodes+0x196/0x1e0
device_release_driver_internal+0x104/0x1d0
driver_detach+0x47/0x90
bus_remove_driver+0x7a/0xd0
pci_unregister_driver+0x3d/0x90
amdgpu_exit+0x11/0x20 [amdgpu]
Signed-off-by: Philip Yang <[email protected]>
Reviewed-by: Felix Kuehling <[email protected]>
Signed-off-by: Alex Deucher <[email protected]>
Furthermore, I guess we are assuming that nobody is using the GPU when
the module is unloaded. As long as any processes have /dev/kfd open, you
won't be able to unload the module (except by force-unload). I suppose
with ZONE_DEVICE memory, we can have references to device memory pages
even when user mode has closed /dev/kfd. We do have a cleanup handler
that runs in an MMU-free-notifier. In theory that should run after all
the pages in the mm_struct have been freed. It releases all sorts of
other device resources and needs the driver to still be there. I'm not
sure if there is anything preventing a module unload before the
free-notifier runs. I'll look into that.
Regards,
Felix
>
>> ---
>> drivers/gpu/drm/nouveau/nouveau_dmem.c | 48 +++++++++++++++++++++++++++-
>> 1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c b/drivers/gpu/drm/nouveau/nouveau_dmem.c
>> index 66ebbd4..3b247b8 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
>> @@ -369,6 +369,52 @@ nouveau_dmem_suspend(struct nouveau_drm *drm)
>> mutex_unlock(&drm->dmem->mutex);
>> }
>>
>> +/*
>> + * Evict all pages mapping a chunk.
>> + */
>> +void
>> +nouveau_dmem_evict_chunk(struct nouveau_dmem_chunk *chunk)
>> +{
>> + unsigned long i, npages = range_len(&chunk->pagemap.range) >> PAGE_SHIFT;
>> + unsigned long *src_pfns, *dst_pfns;
>> + dma_addr_t *dma_addrs;
>> + struct nouveau_fence *fence;
>> +
>> + src_pfns = kcalloc(npages, sizeof(*src_pfns), GFP_KERNEL);
>> + dst_pfns = kcalloc(npages, sizeof(*dst_pfns), GFP_KERNEL);
>> + dma_addrs = kcalloc(npages, sizeof(*dma_addrs), GFP_KERNEL);
>> +
>> + migrate_device_range(src_pfns, chunk->pagemap.range.start >> PAGE_SHIFT,
>> + npages);
>> +
>> + for (i = 0; i < npages; i++) {
>> + if (src_pfns[i] & MIGRATE_PFN_MIGRATE) {
>> + struct page *dpage;
>> +
>> + /*
>> + * _GFP_NOFAIL because the GPU is going away and there
>> + * is nothing sensible we can do if we can't copy the
>> + * data back.
>> + */
> You'll have to excuse me for a moment since this area of nouveau isn't one of
> my strongpoints, but are we sure about this? IIRC __GFP_NOFAIL means infinite
> retry, in the case of a GPU hotplug event I would assume we would rather just
> stop trying to migrate things to the GPU and just drop the data instead of
> hanging on infinite retries.
>
>> + dpage = alloc_page(GFP_HIGHUSER | __GFP_NOFAIL);
>> + dst_pfns[i] = migrate_pfn(page_to_pfn(dpage));
>> + nouveau_dmem_copy_one(chunk->drm,
>> + migrate_pfn_to_page(src_pfns[i]), dpage,
>> + &dma_addrs[i]);
>> + }
>> + }
>> +
>> + nouveau_fence_new(chunk->drm->dmem->migrate.chan, false, &fence);
>> + migrate_device_pages(src_pfns, dst_pfns, npages);
>> + nouveau_dmem_fence_done(&fence);
>> + migrate_device_finalize(src_pfns, dst_pfns, npages);
>> + kfree(src_pfns);
>> + kfree(dst_pfns);
>> + for (i = 0; i < npages; i++)
>> + dma_unmap_page(chunk->drm->dev->dev, dma_addrs[i], PAGE_SIZE, DMA_BIDIRECTIONAL);
>> + kfree(dma_addrs);
>> +}
>> +
>> void
>> nouveau_dmem_fini(struct nouveau_drm *drm)
>> {
>> @@ -380,8 +426,10 @@ nouveau_dmem_fini(struct nouveau_drm *drm)
>> mutex_lock(&drm->dmem->mutex);
>>
>> list_for_each_entry_safe(chunk, tmp, &drm->dmem->chunks, list) {
>> + nouveau_dmem_evict_chunk(chunk);
>> nouveau_bo_unpin(chunk->bo);
>> nouveau_bo_ref(NULL, &chunk->bo);
>> + WARN_ON(chunk->callocated);
>> list_del(&chunk->list);
>> memunmap_pages(&chunk->pagemap);
>> release_mem_region(chunk->pagemap.range.start,
John Hubbard <[email protected]> writes:
> On 9/26/22 14:35, Lyude Paul wrote:
>>> + for (i = 0; i < npages; i++) {
>>> + if (src_pfns[i] & MIGRATE_PFN_MIGRATE) {
>>> + struct page *dpage;
>>> +
>>> + /*
>>> + * _GFP_NOFAIL because the GPU is going away and there
>>> + * is nothing sensible we can do if we can't copy the
>>> + * data back.
>>> + */
>>
>> You'll have to excuse me for a moment since this area of nouveau isn't one of
>> my strongpoints, but are we sure about this? IIRC __GFP_NOFAIL means infinite
>> retry, in the case of a GPU hotplug event I would assume we would rather just
>> stop trying to migrate things to the GPU and just drop the data instead of
>> hanging on infinite retries.
>>
No problem, thanks for taking a look!
> Hi Lyude!
>
> Actually, I really think it's better in this case to keep trying
> (presumably not necessarily infinitely, but only until memory becomes
> available), rather than failing out and corrupting data.
>
> That's because I'm not sure it's completely clear that this memory is
> discardable. And at some point, we're going to make this all work with
> file-backed memory, which will *definitely* not be discardable--I
> realize that we're not there yet, of course.
>
> But here, it's reasonable to commit to just retrying indefinitely,
> really. Memory should eventually show up. And if it doesn't, then
> restarting the machine is better than corrupting data, generally.
The memory is definitely not discardable here if the migration failed
because that implies it is still mapped into some userspace process.
We could avoid restarting the machine by doing something similar to what
happens during memory failure and killing every process that maps the
page(s). But overall I think it's better to retry until memory is
available, because that allows things like reclaim to work and in the
worst case allows the OOM killer to select an appropriate task to kill.
It also won't cause data corruption if/when we have file-backed memory.
> thanks,
Felix Kuehling <[email protected]> writes:
> On 2022-09-26 17:35, Lyude Paul wrote:
>> On Mon, 2022-09-26 at 16:03 +1000, Alistair Popple wrote:
>>> When the module is unloaded or a GPU is unbound from the module it is
>>> possible for device private pages to be left mapped in currently running
>>> processes. This leads to a kernel crash when the pages are either freed
>>> or accessed from the CPU because the GPU and associated data structures
>>> and callbacks have all been freed.
>>>
>>> Fix this by migrating any mappings back to normal CPU memory prior to
>>> freeing the GPU memory chunks and associated device private pages.
>>>
>>> Signed-off-by: Alistair Popple <[email protected]>
>>>
>>> ---
>>>
>>> I assume the AMD driver might have a similar issue. However I can't see
>>> where device private (or coherent) pages actually get unmapped/freed
>>> during teardown as I couldn't find any relevant calls to
>>> devm_memunmap(), memunmap(), devm_release_mem_region() or
>>> release_mem_region(). So it appears that ZONE_DEVICE pages are not being
>>> properly freed during module unload, unless I'm missing something?
>> I've got no idea, will poke Ben to see if they know the answer to this
>
> I guess we're relying on devm to release the region. Isn't the whole point of
> using devm_request_free_mem_region that we don't have to remember to explicitly
> release it when the device gets destroyed? I believe we had an explicit free
> call at some point by mistake, and that caused a double-free during module
> unload. See this commit for reference:
Argh, thanks for that pointer. I was not so familiar with
devm_request_free_mem_region()/devm_memremap_pages() as currently
Nouveau explicitly manages that itself.
> commit 22f4f4faf337d5fb2d2750aff13215726814273e
> Author: Philip Yang <[email protected]>
> Date: Mon Sep 20 17:25:52 2021 -0400
>
> drm/amdkfd: fix svm_migrate_fini warning
> Device manager releases device-specific resources when a driver
> disconnects from a device, devm_memunmap_pages and
> devm_release_mem_region calls in svm_migrate_fini are redundant.
> It causes below warning trace after patch "drm/amdgpu: Split
> amdgpu_device_fini into early and late", so remove function
> svm_migrate_fini.
> BUG: https://gitlab.freedesktop.org/drm/amd/-/issues/1718
> WARNING: CPU: 1 PID: 3646 at drivers/base/devres.c:795
> devm_release_action+0x51/0x60
> Call Trace:
> ? memunmap_pages+0x360/0x360
> svm_migrate_fini+0x2d/0x60 [amdgpu]
> kgd2kfd_device_exit+0x23/0xa0 [amdgpu]
> amdgpu_amdkfd_device_fini_sw+0x1d/0x30 [amdgpu]
> amdgpu_device_fini_sw+0x45/0x290 [amdgpu]
> amdgpu_driver_release_kms+0x12/0x30 [amdgpu]
> drm_dev_release+0x20/0x40 [drm]
> release_nodes+0x196/0x1e0
> device_release_driver_internal+0x104/0x1d0
> driver_detach+0x47/0x90
> bus_remove_driver+0x7a/0xd0
> pci_unregister_driver+0x3d/0x90
> amdgpu_exit+0x11/0x20 [amdgpu]
> Signed-off-by: Philip Yang <[email protected]>
> Reviewed-by: Felix Kuehling <[email protected]>
> Signed-off-by: Alex Deucher <[email protected]>
>
> Furthermore, I guess we are assuming that nobody is using the GPU when the
> module is unloaded. As long as any processes have /dev/kfd open, you won't be
> able to unload the module (except by force-unload). I suppose with ZONE_DEVICE
> memory, we can have references to device memory pages even when user mode has
> closed /dev/kfd. We do have a cleanup handler that runs in an MMU-free-notifier.
> In theory that should run after all the pages in the mm_struct have been freed.
> It releases all sorts of other device resources and needs the driver to still be
> there. I'm not sure if there is anything preventing a module unload before the
> free-notifier runs. I'll look into that.
Right - module unload (or device unbind) is one of the other ways we can
hit this issue in Nouveau at least. You can end up with ZONE_DEVICE
pages mapped in a running process after the module has unloaded.
Although now you mention it that seems a bit wrong - the pgmap refcount
should provide some protection against that. Will have to look into
that too.
> Regards,
> Felix
>
>
>>
>>> ---
>>> drivers/gpu/drm/nouveau/nouveau_dmem.c | 48 +++++++++++++++++++++++++++-
>>> 1 file changed, 48 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c b/drivers/gpu/drm/nouveau/nouveau_dmem.c
>>> index 66ebbd4..3b247b8 100644
>>> --- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
>>> +++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
>>> @@ -369,6 +369,52 @@ nouveau_dmem_suspend(struct nouveau_drm *drm)
>>> mutex_unlock(&drm->dmem->mutex);
>>> }
>>> +/*
>>> + * Evict all pages mapping a chunk.
>>> + */
>>> +void
>>> +nouveau_dmem_evict_chunk(struct nouveau_dmem_chunk *chunk)
>>> +{
>>> + unsigned long i, npages = range_len(&chunk->pagemap.range) >> PAGE_SHIFT;
>>> + unsigned long *src_pfns, *dst_pfns;
>>> + dma_addr_t *dma_addrs;
>>> + struct nouveau_fence *fence;
>>> +
>>> + src_pfns = kcalloc(npages, sizeof(*src_pfns), GFP_KERNEL);
>>> + dst_pfns = kcalloc(npages, sizeof(*dst_pfns), GFP_KERNEL);
>>> + dma_addrs = kcalloc(npages, sizeof(*dma_addrs), GFP_KERNEL);
>>> +
>>> + migrate_device_range(src_pfns, chunk->pagemap.range.start >> PAGE_SHIFT,
>>> + npages);
>>> +
>>> + for (i = 0; i < npages; i++) {
>>> + if (src_pfns[i] & MIGRATE_PFN_MIGRATE) {
>>> + struct page *dpage;
>>> +
>>> + /*
>>> + * _GFP_NOFAIL because the GPU is going away and there
>>> + * is nothing sensible we can do if we can't copy the
>>> + * data back.
>>> + */
>> You'll have to excuse me for a moment since this area of nouveau isn't one of
>> my strongpoints, but are we sure about this? IIRC __GFP_NOFAIL means infinite
>> retry, in the case of a GPU hotplug event I would assume we would rather just
>> stop trying to migrate things to the GPU and just drop the data instead of
>> hanging on infinite retries.
>>
>>> + dpage = alloc_page(GFP_HIGHUSER | __GFP_NOFAIL);
>>> + dst_pfns[i] = migrate_pfn(page_to_pfn(dpage));
>>> + nouveau_dmem_copy_one(chunk->drm,
>>> + migrate_pfn_to_page(src_pfns[i]), dpage,
>>> + &dma_addrs[i]);
>>> + }
>>> + }
>>> +
>>> + nouveau_fence_new(chunk->drm->dmem->migrate.chan, false, &fence);
>>> + migrate_device_pages(src_pfns, dst_pfns, npages);
>>> + nouveau_dmem_fence_done(&fence);
>>> + migrate_device_finalize(src_pfns, dst_pfns, npages);
>>> + kfree(src_pfns);
>>> + kfree(dst_pfns);
>>> + for (i = 0; i < npages; i++)
>>> + dma_unmap_page(chunk->drm->dev->dev, dma_addrs[i], PAGE_SIZE, DMA_BIDIRECTIONAL);
>>> + kfree(dma_addrs);
>>> +}
>>> +
>>> void
>>> nouveau_dmem_fini(struct nouveau_drm *drm)
>>> {
>>> @@ -380,8 +426,10 @@ nouveau_dmem_fini(struct nouveau_drm *drm)
>>> mutex_lock(&drm->dmem->mutex);
>>> list_for_each_entry_safe(chunk, tmp, &drm->dmem->chunks, list) {
>>> + nouveau_dmem_evict_chunk(chunk);
>>> nouveau_bo_unpin(chunk->bo);
>>> nouveau_bo_ref(NULL, &chunk->bo);
>>> + WARN_ON(chunk->callocated);
>>> list_del(&chunk->list);
>>> memunmap_pages(&chunk->pagemap);
>>> release_mem_region(chunk->pagemap.range.start,
Jason Gunthorpe <[email protected]> writes:
> On Mon, Sep 26, 2022 at 04:03:06PM +1000, Alistair Popple wrote:
>> Since 27674ef6c73f ("mm: remove the extra ZONE_DEVICE struct page
>> refcount") device private pages have no longer had an extra reference
>> count when the page is in use. However before handing them back to the
>> owning device driver we add an extra reference count such that free
>> pages have a reference count of one.
>>
>> This makes it difficult to tell if a page is free or not because both
>> free and in use pages will have a non-zero refcount. Instead we should
>> return pages to the drivers page allocator with a zero reference count.
>> Kernel code can then safely use kernel functions such as
>> get_page_unless_zero().
>>
>> Signed-off-by: Alistair Popple <[email protected]>
>> ---
>> arch/powerpc/kvm/book3s_hv_uvmem.c | 1 +
>> drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 1 +
>> drivers/gpu/drm/nouveau/nouveau_dmem.c | 1 +
>> lib/test_hmm.c | 1 +
>> mm/memremap.c | 5 -----
>> mm/page_alloc.c | 6 ++++++
>> 6 files changed, 10 insertions(+), 5 deletions(-)
>
> I think this is a great idea, but I'm surprised no dax stuff is
> touched here?
free_zone_device_page() shouldn't be called for pgmap->type ==
MEMORY_DEVICE_FS_DAX so I don't think we should have to worry about DAX
there. Except that the folio code looks like it might have introduced a
bug. AFAICT put_page() always calls
put_devmap_managed_page(&folio->page) but folio_put() does not (although
folios_put() does!). So it seems folio_put() won't end up calling
__put_devmap_managed_page_refs() as I think it should.
I think you're right about the change to __init_zone_device_page() - I
should limit it to DEVICE_PRIVATE/COHERENT pages only. But I need to
look at Dan's patch series more closely as I suspect it might be better
to rebase this patch on top of that.
> Jason
Lyude Paul <[email protected]> writes:
> On Mon, 2022-09-26 at 16:03 +1000, Alistair Popple wrote:
>> nouveau_dmem_fault_copy_one() is used during handling of CPU faults via
>> the migrate_to_ram() callback and is used to copy data from GPU to CPU
>> memory. It is currently specific to fault handling, however a future
>> patch implementing eviction of data during teardown needs similar
>> functionality.
>>
>> Refactor out the core functionality so that it is not specific to fault
>> handling.
>>
>> Signed-off-by: Alistair Popple <[email protected]>
>> ---
>> drivers/gpu/drm/nouveau/nouveau_dmem.c | 59 +++++++++++++--------------
>> 1 file changed, 29 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c b/drivers/gpu/drm/nouveau/nouveau_dmem.c
>> index f9234ed..66ebbd4 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
>> @@ -139,44 +139,25 @@ static void nouveau_dmem_fence_done(struct nouveau_fence **fence)
>> }
>> }
>>
>> -static vm_fault_t nouveau_dmem_fault_copy_one(struct nouveau_drm *drm,
>> - struct vm_fault *vmf, struct migrate_vma *args,
>> - dma_addr_t *dma_addr)
>> +static int nouveau_dmem_copy_one(struct nouveau_drm *drm, struct page *spage,
>> + struct page *dpage, dma_addr_t *dma_addr)
>> {
>> struct device *dev = drm->dev->dev;
>> - struct page *dpage, *spage;
>> - struct nouveau_svmm *svmm;
>> -
>> - spage = migrate_pfn_to_page(args->src[0]);
>> - if (!spage || !(args->src[0] & MIGRATE_PFN_MIGRATE))
>> - return 0;
>>
>> - dpage = alloc_page_vma(GFP_HIGHUSER, vmf->vma, vmf->address);
>> - if (!dpage)
>> - return VM_FAULT_SIGBUS;
>> lock_page(dpage);
>>
>> *dma_addr = dma_map_page(dev, dpage, 0, PAGE_SIZE, DMA_BIDIRECTIONAL);
>> if (dma_mapping_error(dev, *dma_addr))
>> - goto error_free_page;
>> + return -EIO;
>>
>> - svmm = spage->zone_device_data;
>> - mutex_lock(&svmm->mutex);
>> - nouveau_svmm_invalidate(svmm, args->start, args->end);
>> if (drm->dmem->migrate.copy_func(drm, 1, NOUVEAU_APER_HOST, *dma_addr,
>> - NOUVEAU_APER_VRAM, nouveau_dmem_page_addr(spage)))
>> - goto error_dma_unmap;
>> - mutex_unlock(&svmm->mutex);
>> + NOUVEAU_APER_VRAM,
>> + nouveau_dmem_page_addr(spage))) {
>> + dma_unmap_page(dev, *dma_addr, PAGE_SIZE, DMA_BIDIRECTIONAL);
>> + return -EIO;
>> + }
>
> Feel free to just align this with the starting (, as long as it doesn't go
> above 100 characters it doesn't really matter imho and would look nicer that
> way.
>
> Otherwise:
>
> Reviewed-by: Lyude Paul <[email protected]>
Thanks! I'm not sure I precisely understood your alignment comment above
but feel free to let me know if I got it wrong in v2.
> Will look at the other patch in a moment
>
>>
>> - args->dst[0] = migrate_pfn(page_to_pfn(dpage));
>> return 0;
>> -
>> -error_dma_unmap:
>> - mutex_unlock(&svmm->mutex);
>> - dma_unmap_page(dev, *dma_addr, PAGE_SIZE, DMA_BIDIRECTIONAL);
>> -error_free_page:
>> - __free_page(dpage);
>> - return VM_FAULT_SIGBUS;
>> }
>>
>> static vm_fault_t nouveau_dmem_migrate_to_ram(struct vm_fault *vmf)
>> @@ -184,9 +165,11 @@ static vm_fault_t nouveau_dmem_migrate_to_ram(struct vm_fault *vmf)
>> struct nouveau_drm *drm = page_to_drm(vmf->page);
>> struct nouveau_dmem *dmem = drm->dmem;
>> struct nouveau_fence *fence;
>> + struct nouveau_svmm *svmm;
>> + struct page *spage, *dpage;
>> unsigned long src = 0, dst = 0;
>> dma_addr_t dma_addr = 0;
>> - vm_fault_t ret;
>> + vm_fault_t ret = 0;
>> struct migrate_vma args = {
>> .vma = vmf->vma,
>> .start = vmf->address,
>> @@ -207,9 +190,25 @@ static vm_fault_t nouveau_dmem_migrate_to_ram(struct vm_fault *vmf)
>> if (!args.cpages)
>> return 0;
>>
>> - ret = nouveau_dmem_fault_copy_one(drm, vmf, &args, &dma_addr);
>> - if (ret || dst == 0)
>> + spage = migrate_pfn_to_page(src);
>> + if (!spage || !(src & MIGRATE_PFN_MIGRATE))
>> + goto done;
>> +
>> + dpage = alloc_page_vma(GFP_HIGHUSER, vmf->vma, vmf->address);
>> + if (!dpage)
>> + goto done;
>> +
>> + dst = migrate_pfn(page_to_pfn(dpage));
>> +
>> + svmm = spage->zone_device_data;
>> + mutex_lock(&svmm->mutex);
>> + nouveau_svmm_invalidate(svmm, args.start, args.end);
>> + ret = nouveau_dmem_copy_one(drm, spage, dpage, &dma_addr);
>> + mutex_unlock(&svmm->mutex);
>> + if (ret) {
>> + ret = VM_FAULT_SIGBUS;
>> goto done;
>> + }
>>
>> nouveau_fence_new(dmem->migrate.chan, false, &fence);
>> migrate_vma_pages(&args);
On Tue, 2022-09-27 at 11:39 +1000, Alistair Popple wrote:
> Felix Kuehling <[email protected]> writes:
>
> > On 2022-09-26 17:35, Lyude Paul wrote:
> > > On Mon, 2022-09-26 at 16:03 +1000, Alistair Popple wrote:
> > > > When the module is unloaded or a GPU is unbound from the module it is
> > > > possible for device private pages to be left mapped in currently running
> > > > processes. This leads to a kernel crash when the pages are either freed
> > > > or accessed from the CPU because the GPU and associated data structures
> > > > and callbacks have all been freed.
> > > >
> > > > Fix this by migrating any mappings back to normal CPU memory prior to
> > > > freeing the GPU memory chunks and associated device private pages.
> > > >
> > > > Signed-off-by: Alistair Popple <[email protected]>
> > > >
> > > > ---
> > > >
> > > > I assume the AMD driver might have a similar issue. However I can't see
> > > > where device private (or coherent) pages actually get unmapped/freed
> > > > during teardown as I couldn't find any relevant calls to
> > > > devm_memunmap(), memunmap(), devm_release_mem_region() or
> > > > release_mem_region(). So it appears that ZONE_DEVICE pages are not being
> > > > properly freed during module unload, unless I'm missing something?
> > > I've got no idea, will poke Ben to see if they know the answer to this
> >
> > I guess we're relying on devm to release the region. Isn't the whole point of
> > using devm_request_free_mem_region that we don't have to remember to explicitly
> > release it when the device gets destroyed? I believe we had an explicit free
> > call at some point by mistake, and that caused a double-free during module
> > unload. See this commit for reference:
>
> Argh, thanks for that pointer. I was not so familiar with
> devm_request_free_mem_region()/devm_memremap_pages() as currently
> Nouveau explicitly manages that itself.
Mhm, TBH I feel like this was going to happen eventually anyway but there's
another reason for nouveau to start using the managed versions of these
functions at some point
>
> > commit 22f4f4faf337d5fb2d2750aff13215726814273e
> > Author: Philip Yang <[email protected]>
> > Date: Mon Sep 20 17:25:52 2021 -0400
> >
> > drm/amdkfd: fix svm_migrate_fini warning
> > Device manager releases device-specific resources when a driver
> > disconnects from a device, devm_memunmap_pages and
> > devm_release_mem_region calls in svm_migrate_fini are redundant.
> > It causes below warning trace after patch "drm/amdgpu: Split
> > amdgpu_device_fini into early and late", so remove function
> > svm_migrate_fini.
> > BUG: https://gitlab.freedesktop.org/drm/amd/-/issues/1718
> > WARNING: CPU: 1 PID: 3646 at drivers/base/devres.c:795
> > devm_release_action+0x51/0x60
> > Call Trace:
> > ? memunmap_pages+0x360/0x360
> > svm_migrate_fini+0x2d/0x60 [amdgpu]
> > kgd2kfd_device_exit+0x23/0xa0 [amdgpu]
> > amdgpu_amdkfd_device_fini_sw+0x1d/0x30 [amdgpu]
> > amdgpu_device_fini_sw+0x45/0x290 [amdgpu]
> > amdgpu_driver_release_kms+0x12/0x30 [amdgpu]
> > drm_dev_release+0x20/0x40 [drm]
> > release_nodes+0x196/0x1e0
> > device_release_driver_internal+0x104/0x1d0
> > driver_detach+0x47/0x90
> > bus_remove_driver+0x7a/0xd0
> > pci_unregister_driver+0x3d/0x90
> > amdgpu_exit+0x11/0x20 [amdgpu]
> > Signed-off-by: Philip Yang <[email protected]>
> > Reviewed-by: Felix Kuehling <[email protected]>
> > Signed-off-by: Alex Deucher <[email protected]>
> >
> > Furthermore, I guess we are assuming that nobody is using the GPU when the
> > module is unloaded. As long as any processes have /dev/kfd open, you won't be
> > able to unload the module (except by force-unload). I suppose with ZONE_DEVICE
> > memory, we can have references to device memory pages even when user mode has
> > closed /dev/kfd. We do have a cleanup handler that runs in an MMU-free-notifier.
> > In theory that should run after all the pages in the mm_struct have been freed.
> > It releases all sorts of other device resources and needs the driver to still be
> > there. I'm not sure if there is anything preventing a module unload before the
> > free-notifier runs. I'll look into that.
>
> Right - module unload (or device unbind) is one of the other ways we can
> hit this issue in Nouveau at least. You can end up with ZONE_DEVICE
> pages mapped in a running process after the module has unloaded.
> Although now you mention it that seems a bit wrong - the pgmap refcount
> should provide some protection against that. Will have to look into
> that too.
>
> > Regards,
> > Felix
> >
> >
> > >
> > > > ---
> > > > drivers/gpu/drm/nouveau/nouveau_dmem.c | 48 +++++++++++++++++++++++++++-
> > > > 1 file changed, 48 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c b/drivers/gpu/drm/nouveau/nouveau_dmem.c
> > > > index 66ebbd4..3b247b8 100644
> > > > --- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
> > > > +++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
> > > > @@ -369,6 +369,52 @@ nouveau_dmem_suspend(struct nouveau_drm *drm)
> > > > mutex_unlock(&drm->dmem->mutex);
> > > > }
> > > > +/*
> > > > + * Evict all pages mapping a chunk.
> > > > + */
> > > > +void
> > > > +nouveau_dmem_evict_chunk(struct nouveau_dmem_chunk *chunk)
> > > > +{
> > > > + unsigned long i, npages = range_len(&chunk->pagemap.range) >> PAGE_SHIFT;
> > > > + unsigned long *src_pfns, *dst_pfns;
> > > > + dma_addr_t *dma_addrs;
> > > > + struct nouveau_fence *fence;
> > > > +
> > > > + src_pfns = kcalloc(npages, sizeof(*src_pfns), GFP_KERNEL);
> > > > + dst_pfns = kcalloc(npages, sizeof(*dst_pfns), GFP_KERNEL);
> > > > + dma_addrs = kcalloc(npages, sizeof(*dma_addrs), GFP_KERNEL);
> > > > +
> > > > + migrate_device_range(src_pfns, chunk->pagemap.range.start >> PAGE_SHIFT,
> > > > + npages);
> > > > +
> > > > + for (i = 0; i < npages; i++) {
> > > > + if (src_pfns[i] & MIGRATE_PFN_MIGRATE) {
> > > > + struct page *dpage;
> > > > +
> > > > + /*
> > > > + * _GFP_NOFAIL because the GPU is going away and there
> > > > + * is nothing sensible we can do if we can't copy the
> > > > + * data back.
> > > > + */
> > > You'll have to excuse me for a moment since this area of nouveau isn't one of
> > > my strongpoints, but are we sure about this? IIRC __GFP_NOFAIL means infinite
> > > retry, in the case of a GPU hotplug event I would assume we would rather just
> > > stop trying to migrate things to the GPU and just drop the data instead of
> > > hanging on infinite retries.
> > >
> > > > + dpage = alloc_page(GFP_HIGHUSER | __GFP_NOFAIL);
> > > > + dst_pfns[i] = migrate_pfn(page_to_pfn(dpage));
> > > > + nouveau_dmem_copy_one(chunk->drm,
> > > > + migrate_pfn_to_page(src_pfns[i]), dpage,
> > > > + &dma_addrs[i]);
> > > > + }
> > > > + }
> > > > +
> > > > + nouveau_fence_new(chunk->drm->dmem->migrate.chan, false, &fence);
> > > > + migrate_device_pages(src_pfns, dst_pfns, npages);
> > > > + nouveau_dmem_fence_done(&fence);
> > > > + migrate_device_finalize(src_pfns, dst_pfns, npages);
> > > > + kfree(src_pfns);
> > > > + kfree(dst_pfns);
> > > > + for (i = 0; i < npages; i++)
> > > > + dma_unmap_page(chunk->drm->dev->dev, dma_addrs[i], PAGE_SIZE, DMA_BIDIRECTIONAL);
> > > > + kfree(dma_addrs);
> > > > +}
> > > > +
> > > > void
> > > > nouveau_dmem_fini(struct nouveau_drm *drm)
> > > > {
> > > > @@ -380,8 +426,10 @@ nouveau_dmem_fini(struct nouveau_drm *drm)
> > > > mutex_lock(&drm->dmem->mutex);
> > > > list_for_each_entry_safe(chunk, tmp, &drm->dmem->chunks, list) {
> > > > + nouveau_dmem_evict_chunk(chunk);
> > > > nouveau_bo_unpin(chunk->bo);
> > > > nouveau_bo_ref(NULL, &chunk->bo);
> > > > + WARN_ON(chunk->callocated);
> > > > list_del(&chunk->list);
> > > > memunmap_pages(&chunk->pagemap);
> > > > release_mem_region(chunk->pagemap.range.start,
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
Re comments about infinite retry: gotcha, makes sense to me.
On Tue, 2022-09-27 at 09:45 +1000, Alistair Popple wrote:
> John Hubbard <[email protected]> writes:
>
> > On 9/26/22 14:35, Lyude Paul wrote:
> > > > + for (i = 0; i < npages; i++) {
> > > > + if (src_pfns[i] & MIGRATE_PFN_MIGRATE) {
> > > > + struct page *dpage;
> > > > +
> > > > + /*
> > > > + * _GFP_NOFAIL because the GPU is going away and there
> > > > + * is nothing sensible we can do if we can't copy the
> > > > + * data back.
> > > > + */
> > >
> > > You'll have to excuse me for a moment since this area of nouveau isn't one of
> > > my strongpoints, but are we sure about this? IIRC __GFP_NOFAIL means infinite
> > > retry, in the case of a GPU hotplug event I would assume we would rather just
> > > stop trying to migrate things to the GPU and just drop the data instead of
> > > hanging on infinite retries.
> > >
>
> No problem, thanks for taking a look!
>
> > Hi Lyude!
> >
> > Actually, I really think it's better in this case to keep trying
> > (presumably not necessarily infinitely, but only until memory becomes
> > available), rather than failing out and corrupting data.
> >
> > That's because I'm not sure it's completely clear that this memory is
> > discardable. And at some point, we're going to make this all work with
> > file-backed memory, which will *definitely* not be discardable--I
> > realize that we're not there yet, of course.
> >
> > But here, it's reasonable to commit to just retrying indefinitely,
> > really. Memory should eventually show up. And if it doesn't, then
> > restarting the machine is better than corrupting data, generally.
>
> The memory is definitely not discardable here if the migration failed
> because that implies it is still mapped into some userspace process.
>
> We could avoid restarting the machine by doing something similar to what
> happens during memory failure and killing every process that maps the
> page(s). But overall I think it's better to retry until memory is
> available, because that allows things like reclaim to work and in the
> worst case allows the OOM killer to select an appropriate task to kill.
> It also won't cause data corruption if/when we have file-backed memory.
>
> > thanks,
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
Alistair Popple <[email protected]> writes:
> When the CPU tries to access a device private page the migrate_to_ram()
> callback associated with the pgmap for the page is called. However no
> reference is taken on the faulting page. Therefore a concurrent
> migration of the device private page can free the page and possibly the
> underlying pgmap. This results in a race which can crash the kernel due
> to the migrate_to_ram() function pointer becoming invalid. It also means
> drivers can't reliably read the zone_device_data field because the page
> may have been freed with memunmap_pages().
>
> Close the race by getting a reference on the page while holding the ptl
> to ensure it has not been freed. Unfortunately the elevated reference
> count will cause the migration required to handle the fault to fail. To
> avoid this failure pass the faulting page into the migrate_vma functions
> so that if an elevated reference count is found it can be checked to see
> if it's expected or not.
>
> Signed-off-by: Alistair Popple <[email protected]>
> ---
> arch/powerpc/kvm/book3s_hv_uvmem.c | 15 ++++++-----
> drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 17 +++++++------
> drivers/gpu/drm/amd/amdkfd/kfd_migrate.h | 2 +-
> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 11 +++++---
> include/linux/migrate.h | 8 ++++++-
> lib/test_hmm.c | 7 ++---
> mm/memory.c | 16 +++++++++++-
> mm/migrate.c | 34 ++++++++++++++-----------
> mm/migrate_device.c | 18 +++++++++----
> 9 files changed, 87 insertions(+), 41 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
> index 5980063..d4eacf4 100644
> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c
> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
> @@ -508,10 +508,10 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
> static int __kvmppc_svm_page_out(struct vm_area_struct *vma,
> unsigned long start,
> unsigned long end, unsigned long page_shift,
> - struct kvm *kvm, unsigned long gpa)
> + struct kvm *kvm, unsigned long gpa, struct page *fault_page)
> {
> unsigned long src_pfn, dst_pfn = 0;
> - struct migrate_vma mig;
> + struct migrate_vma mig = { 0 };
> struct page *dpage, *spage;
> struct kvmppc_uvmem_page_pvt *pvt;
> unsigned long pfn;
> @@ -525,6 +525,7 @@ static int __kvmppc_svm_page_out(struct vm_area_struct *vma,
> mig.dst = &dst_pfn;
> mig.pgmap_owner = &kvmppc_uvmem_pgmap;
> mig.flags = MIGRATE_VMA_SELECT_DEVICE_PRIVATE;
> + mig.fault_page = fault_page;
>
> /* The requested page is already paged-out, nothing to do */
> if (!kvmppc_gfn_is_uvmem_pfn(gpa >> page_shift, kvm, NULL))
> @@ -580,12 +581,14 @@ static int __kvmppc_svm_page_out(struct vm_area_struct *vma,
> static inline int kvmppc_svm_page_out(struct vm_area_struct *vma,
> unsigned long start, unsigned long end,
> unsigned long page_shift,
> - struct kvm *kvm, unsigned long gpa)
> + struct kvm *kvm, unsigned long gpa,
> + struct page *fault_page)
> {
> int ret;
>
> mutex_lock(&kvm->arch.uvmem_lock);
> - ret = __kvmppc_svm_page_out(vma, start, end, page_shift, kvm, gpa);
> + ret = __kvmppc_svm_page_out(vma, start, end, page_shift, kvm, gpa,
> + fault_page);
> mutex_unlock(&kvm->arch.uvmem_lock);
>
> return ret;
> @@ -736,7 +739,7 @@ static int kvmppc_svm_page_in(struct vm_area_struct *vma,
> bool pagein)
> {
> unsigned long src_pfn, dst_pfn = 0;
> - struct migrate_vma mig;
> + struct migrate_vma mig = { 0 };
> struct page *spage;
> unsigned long pfn;
> struct page *dpage;
> @@ -994,7 +997,7 @@ static vm_fault_t kvmppc_uvmem_migrate_to_ram(struct vm_fault *vmf)
>
> if (kvmppc_svm_page_out(vmf->vma, vmf->address,
> vmf->address + PAGE_SIZE, PAGE_SHIFT,
> - pvt->kvm, pvt->gpa))
> + pvt->kvm, pvt->gpa, vmf->page))
> return VM_FAULT_SIGBUS;
> else
> return 0;
I don't have a UV test system, but as-is it doesn't even compile :)
kvmppc_svm_page_out() is called via some paths other than the
migrate_to_ram callback.
I think it's correct to just pass fault_page = NULL when it's not called
from the migrate_to_ram callback?
Incremental diff below.
cheers
diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
index d4eacf410956..965c9e9e500b 100644
--- a/arch/powerpc/kvm/book3s_hv_uvmem.c
+++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
@@ -637,7 +637,7 @@ void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *slot,
pvt->remove_gfn = true;
if (__kvmppc_svm_page_out(vma, addr, addr + PAGE_SIZE,
- PAGE_SHIFT, kvm, pvt->gpa))
+ PAGE_SHIFT, kvm, pvt->gpa, NULL))
pr_err("Can't page out gpa:0x%lx addr:0x%lx\n",
pvt->gpa, addr);
} else {
@@ -1068,7 +1068,7 @@ kvmppc_h_svm_page_out(struct kvm *kvm, unsigned long gpa,
if (!vma || vma->vm_start > start || vma->vm_end < end)
goto out;
- if (!kvmppc_svm_page_out(vma, start, end, page_shift, kvm, gpa))
+ if (!kvmppc_svm_page_out(vma, start, end, page_shift, kvm, gpa, NULL))
ret = H_SUCCESS;
out:
mmap_read_unlock(kvm->mm);
Michael Ellerman <[email protected]> writes:
> Alistair Popple <[email protected]> writes:
>> When the CPU tries to access a device private page the migrate_to_ram()
>> callback associated with the pgmap for the page is called. However no
>> reference is taken on the faulting page. Therefore a concurrent
>> migration of the device private page can free the page and possibly the
>> underlying pgmap. This results in a race which can crash the kernel due
>> to the migrate_to_ram() function pointer becoming invalid. It also means
>> drivers can't reliably read the zone_device_data field because the page
>> may have been freed with memunmap_pages().
>>
>> Close the race by getting a reference on the page while holding the ptl
>> to ensure it has not been freed. Unfortunately the elevated reference
>> count will cause the migration required to handle the fault to fail. To
>> avoid this failure pass the faulting page into the migrate_vma functions
>> so that if an elevated reference count is found it can be checked to see
>> if it's expected or not.
>>
>> Signed-off-by: Alistair Popple <[email protected]>
>> ---
>> arch/powerpc/kvm/book3s_hv_uvmem.c | 15 ++++++-----
>> drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 17 +++++++------
>> drivers/gpu/drm/amd/amdkfd/kfd_migrate.h | 2 +-
>> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 11 +++++---
>> include/linux/migrate.h | 8 ++++++-
>> lib/test_hmm.c | 7 ++---
>> mm/memory.c | 16 +++++++++++-
>> mm/migrate.c | 34 ++++++++++++++-----------
>> mm/migrate_device.c | 18 +++++++++----
>> 9 files changed, 87 insertions(+), 41 deletions(-)
>>
>> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
>> index 5980063..d4eacf4 100644
>> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c
>> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
>> @@ -508,10 +508,10 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
>> static int __kvmppc_svm_page_out(struct vm_area_struct *vma,
>> unsigned long start,
>> unsigned long end, unsigned long page_shift,
>> - struct kvm *kvm, unsigned long gpa)
>> + struct kvm *kvm, unsigned long gpa, struct page *fault_page)
>> {
>> unsigned long src_pfn, dst_pfn = 0;
>> - struct migrate_vma mig;
>> + struct migrate_vma mig = { 0 };
>> struct page *dpage, *spage;
>> struct kvmppc_uvmem_page_pvt *pvt;
>> unsigned long pfn;
>> @@ -525,6 +525,7 @@ static int __kvmppc_svm_page_out(struct vm_area_struct *vma,
>> mig.dst = &dst_pfn;
>> mig.pgmap_owner = &kvmppc_uvmem_pgmap;
>> mig.flags = MIGRATE_VMA_SELECT_DEVICE_PRIVATE;
>> + mig.fault_page = fault_page;
>>
>> /* The requested page is already paged-out, nothing to do */
>> if (!kvmppc_gfn_is_uvmem_pfn(gpa >> page_shift, kvm, NULL))
>> @@ -580,12 +581,14 @@ static int __kvmppc_svm_page_out(struct vm_area_struct *vma,
>> static inline int kvmppc_svm_page_out(struct vm_area_struct *vma,
>> unsigned long start, unsigned long end,
>> unsigned long page_shift,
>> - struct kvm *kvm, unsigned long gpa)
>> + struct kvm *kvm, unsigned long gpa,
>> + struct page *fault_page)
>> {
>> int ret;
>>
>> mutex_lock(&kvm->arch.uvmem_lock);
>> - ret = __kvmppc_svm_page_out(vma, start, end, page_shift, kvm, gpa);
>> + ret = __kvmppc_svm_page_out(vma, start, end, page_shift, kvm, gpa,
>> + fault_page);
>> mutex_unlock(&kvm->arch.uvmem_lock);
>>
>> return ret;
>> @@ -736,7 +739,7 @@ static int kvmppc_svm_page_in(struct vm_area_struct *vma,
>> bool pagein)
>> {
>> unsigned long src_pfn, dst_pfn = 0;
>> - struct migrate_vma mig;
>> + struct migrate_vma mig = { 0 };
>> struct page *spage;
>> unsigned long pfn;
>> struct page *dpage;
>> @@ -994,7 +997,7 @@ static vm_fault_t kvmppc_uvmem_migrate_to_ram(struct vm_fault *vmf)
>>
>> if (kvmppc_svm_page_out(vmf->vma, vmf->address,
>> vmf->address + PAGE_SIZE, PAGE_SHIFT,
>> - pvt->kvm, pvt->gpa))
>> + pvt->kvm, pvt->gpa, vmf->page))
>> return VM_FAULT_SIGBUS;
>> else
>> return 0;
>
> I don't have a UV test system, but as-is it doesn't even compile :)
Ugh, thanks. I did get as far as installing a PPC cross-compiler and
building a kernel. Apparently I did not get as far as enabling
CONFIG_PPC_UV :)
> kvmppc_svm_page_out() is called via some paths other than the
> migrate_to_ram callback.
>
> I think it's correct to just pass fault_page = NULL when it's not called
> from the migrate_to_ram callback?
>
> Incremental diff below.
>
> cheers
>
>
> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
> index d4eacf410956..965c9e9e500b 100644
> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c
> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
> @@ -637,7 +637,7 @@ void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *slot,
> pvt->remove_gfn = true;
>
> if (__kvmppc_svm_page_out(vma, addr, addr + PAGE_SIZE,
> - PAGE_SHIFT, kvm, pvt->gpa))
> + PAGE_SHIFT, kvm, pvt->gpa, NULL))
> pr_err("Can't page out gpa:0x%lx addr:0x%lx\n",
> pvt->gpa, addr);
> } else {
> @@ -1068,7 +1068,7 @@ kvmppc_h_svm_page_out(struct kvm *kvm, unsigned long gpa,
> if (!vma || vma->vm_start > start || vma->vm_end < end)
> goto out;
>
> - if (!kvmppc_svm_page_out(vma, start, end, page_shift, kvm, gpa))
> + if (!kvmppc_svm_page_out(vma, start, end, page_shift, kvm, gpa, NULL))
> ret = H_SUCCESS;
> out:
> mmap_read_unlock(kvm->mm);
Alistair Popple <[email protected]> writes:
> Michael Ellerman <[email protected]> writes:
>> Alistair Popple <[email protected]> writes:
>>> When the CPU tries to access a device private page the migrate_to_ram()
>>> callback associated with the pgmap for the page is called. However no
>>> reference is taken on the faulting page. Therefore a concurrent
>>> migration of the device private page can free the page and possibly the
>>> underlying pgmap. This results in a race which can crash the kernel due
>>> to the migrate_to_ram() function pointer becoming invalid. It also means
>>> drivers can't reliably read the zone_device_data field because the page
>>> may have been freed with memunmap_pages().
>>>
>>> Close the race by getting a reference on the page while holding the ptl
>>> to ensure it has not been freed. Unfortunately the elevated reference
>>> count will cause the migration required to handle the fault to fail. To
>>> avoid this failure pass the faulting page into the migrate_vma functions
>>> so that if an elevated reference count is found it can be checked to see
>>> if it's expected or not.
>>>
>>> Signed-off-by: Alistair Popple <[email protected]>
>>> ---
>>> arch/powerpc/kvm/book3s_hv_uvmem.c | 15 ++++++-----
>>> drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 17 +++++++------
>>> drivers/gpu/drm/amd/amdkfd/kfd_migrate.h | 2 +-
>>> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 11 +++++---
>>> include/linux/migrate.h | 8 ++++++-
>>> lib/test_hmm.c | 7 ++---
>>> mm/memory.c | 16 +++++++++++-
>>> mm/migrate.c | 34 ++++++++++++++-----------
>>> mm/migrate_device.c | 18 +++++++++----
>>> 9 files changed, 87 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
>>> index 5980063..d4eacf4 100644
>>> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c
>>> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
>>> @@ -508,10 +508,10 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
...
>>> @@ -994,7 +997,7 @@ static vm_fault_t kvmppc_uvmem_migrate_to_ram(struct vm_fault *vmf)
>>>
>>> if (kvmppc_svm_page_out(vmf->vma, vmf->address,
>>> vmf->address + PAGE_SIZE, PAGE_SHIFT,
>>> - pvt->kvm, pvt->gpa))
>>> + pvt->kvm, pvt->gpa, vmf->page))
>>> return VM_FAULT_SIGBUS;
>>> else
>>> return 0;
>>
>> I don't have a UV test system, but as-is it doesn't even compile :)
>
> Ugh, thanks. I did get as far as installing a PPC cross-compiler and
> building a kernel. Apparently I did not get as far as enabling
> CONFIG_PPC_UV :)
No worries, that's really on us. If we're going to keep the code in the
tree then it should really be enabled in at least one of our defconfigs.
cheers
Alistair Popple wrote:
>
> Jason Gunthorpe <[email protected]> writes:
>
> > On Mon, Sep 26, 2022 at 04:03:06PM +1000, Alistair Popple wrote:
> >> Since 27674ef6c73f ("mm: remove the extra ZONE_DEVICE struct page
> >> refcount") device private pages have no longer had an extra reference
> >> count when the page is in use. However before handing them back to the
> >> owning device driver we add an extra reference count such that free
> >> pages have a reference count of one.
> >>
> >> This makes it difficult to tell if a page is free or not because both
> >> free and in use pages will have a non-zero refcount. Instead we should
> >> return pages to the drivers page allocator with a zero reference count.
> >> Kernel code can then safely use kernel functions such as
> >> get_page_unless_zero().
> >>
> >> Signed-off-by: Alistair Popple <[email protected]>
> >> ---
> >> arch/powerpc/kvm/book3s_hv_uvmem.c | 1 +
> >> drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 1 +
> >> drivers/gpu/drm/nouveau/nouveau_dmem.c | 1 +
> >> lib/test_hmm.c | 1 +
> >> mm/memremap.c | 5 -----
> >> mm/page_alloc.c | 6 ++++++
> >> 6 files changed, 10 insertions(+), 5 deletions(-)
> >
> > I think this is a great idea, but I'm surprised no dax stuff is
> > touched here?
>
> free_zone_device_page() shouldn't be called for pgmap->type ==
> MEMORY_DEVICE_FS_DAX so I don't think we should have to worry about DAX
> there. Except that the folio code looks like it might have introduced a
> bug. AFAICT put_page() always calls
> put_devmap_managed_page(&folio->page) but folio_put() does not (although
> folios_put() does!). So it seems folio_put() won't end up calling
> __put_devmap_managed_page_refs() as I think it should.
>
> I think you're right about the change to __init_zone_device_page() - I
> should limit it to DEVICE_PRIVATE/COHERENT pages only. But I need to
> look at Dan's patch series more closely as I suspect it might be better
> to rebase this patch on top of that.
Apologies for the delay I was travelling the past few days. Yes, I think
this patch slots in nicely to avoid the introduction of an init_mode
[1]:
https://lore.kernel.org/nvdimm/166329940343.2786261.6047770378829215962.stgit@dwillia2-xfh.jf.intel.com/
Mind if I steal it into my series?
Dan Williams <[email protected]> writes:
> Alistair Popple wrote:
>>
>> Jason Gunthorpe <[email protected]> writes:
>>
>> > On Mon, Sep 26, 2022 at 04:03:06PM +1000, Alistair Popple wrote:
>> >> Since 27674ef6c73f ("mm: remove the extra ZONE_DEVICE struct page
>> >> refcount") device private pages have no longer had an extra reference
>> >> count when the page is in use. However before handing them back to the
>> >> owning device driver we add an extra reference count such that free
>> >> pages have a reference count of one.
>> >>
>> >> This makes it difficult to tell if a page is free or not because both
>> >> free and in use pages will have a non-zero refcount. Instead we should
>> >> return pages to the drivers page allocator with a zero reference count.
>> >> Kernel code can then safely use kernel functions such as
>> >> get_page_unless_zero().
>> >>
>> >> Signed-off-by: Alistair Popple <[email protected]>
>> >> ---
>> >> arch/powerpc/kvm/book3s_hv_uvmem.c | 1 +
>> >> drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 1 +
>> >> drivers/gpu/drm/nouveau/nouveau_dmem.c | 1 +
>> >> lib/test_hmm.c | 1 +
>> >> mm/memremap.c | 5 -----
>> >> mm/page_alloc.c | 6 ++++++
>> >> 6 files changed, 10 insertions(+), 5 deletions(-)
>> >
>> > I think this is a great idea, but I'm surprised no dax stuff is
>> > touched here?
>>
>> free_zone_device_page() shouldn't be called for pgmap->type ==
>> MEMORY_DEVICE_FS_DAX so I don't think we should have to worry about DAX
>> there. Except that the folio code looks like it might have introduced a
>> bug. AFAICT put_page() always calls
>> put_devmap_managed_page(&folio->page) but folio_put() does not (although
>> folios_put() does!). So it seems folio_put() won't end up calling
>> __put_devmap_managed_page_refs() as I think it should.
>>
>> I think you're right about the change to __init_zone_device_page() - I
>> should limit it to DEVICE_PRIVATE/COHERENT pages only. But I need to
>> look at Dan's patch series more closely as I suspect it might be better
>> to rebase this patch on top of that.
>
> Apologies for the delay I was travelling the past few days. Yes, I think
> this patch slots in nicely to avoid the introduction of an init_mode
> [1]:
>
> https://lore.kernel.org/nvdimm/166329940343.2786261.6047770378829215962.stgit@dwillia2-xfh.jf.intel.com/
>
> Mind if I steal it into my series?
No problem, although I notice Andrew has already merged it into
mm-unstable. If you end up rebasing your series on top of mine I think
all that's needed is a patch somewhere in your series to drop the
various `if (pgmap->type == MEMORY_DEVICE_*)` I added to (hopefully)
avoid breaking DAX. Assuming DAX takes a pagemap reference on struct
page allocation something like below.
---
diff --git a/mm/memremap.c b/mm/memremap.c
index 421bec3a29ee..da1a0e0abb8b 100644
--- a/mm/memremap.c
+++ b/mm/memremap.c
@@ -507,15 +507,7 @@ void free_zone_device_page(struct page *page)
page->mapping = NULL;
page->pgmap->ops->page_free(page);
- if (page->pgmap->type != MEMORY_DEVICE_PRIVATE &&
- page->pgmap->type != MEMORY_DEVICE_COHERENT)
- /*
- * Reset the page count to 1 to prepare for handing out the page
- * again.
- */
- set_page_count(page, 1);
- else
- put_dev_pagemap(page->pgmap);
+ put_dev_pagemap(page->pgmap);
}
void zone_device_page_init(struct page *page)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 014dbdf54d62..3e5ff06700ca 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -6816,9 +6816,7 @@ static void __ref __init_zone_device_page(struct page *page, unsigned long pfn,
* ZONE_DEVICE pages are released directly to the driver page allocator
* which will set the page count to 1 when allocating the page.
*/
- if (pgmap->type == MEMORY_DEVICE_PRIVATE ||
- pgmap->type == MEMORY_DEVICE_COHERENT)
- set_page_count(page, 0);
+ set_page_count(page, 0);
}
/*
Alistair Popple wrote:
>
> Dan Williams <[email protected]> writes:
>
> > Alistair Popple wrote:
> >>
> >> Jason Gunthorpe <[email protected]> writes:
> >>
> >> > On Mon, Sep 26, 2022 at 04:03:06PM +1000, Alistair Popple wrote:
> >> >> Since 27674ef6c73f ("mm: remove the extra ZONE_DEVICE struct page
> >> >> refcount") device private pages have no longer had an extra reference
> >> >> count when the page is in use. However before handing them back to the
> >> >> owning device driver we add an extra reference count such that free
> >> >> pages have a reference count of one.
> >> >>
> >> >> This makes it difficult to tell if a page is free or not because both
> >> >> free and in use pages will have a non-zero refcount. Instead we should
> >> >> return pages to the drivers page allocator with a zero reference count.
> >> >> Kernel code can then safely use kernel functions such as
> >> >> get_page_unless_zero().
> >> >>
> >> >> Signed-off-by: Alistair Popple <[email protected]>
> >> >> ---
> >> >> arch/powerpc/kvm/book3s_hv_uvmem.c | 1 +
> >> >> drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 1 +
> >> >> drivers/gpu/drm/nouveau/nouveau_dmem.c | 1 +
> >> >> lib/test_hmm.c | 1 +
> >> >> mm/memremap.c | 5 -----
> >> >> mm/page_alloc.c | 6 ++++++
> >> >> 6 files changed, 10 insertions(+), 5 deletions(-)
> >> >
> >> > I think this is a great idea, but I'm surprised no dax stuff is
> >> > touched here?
> >>
> >> free_zone_device_page() shouldn't be called for pgmap->type ==
> >> MEMORY_DEVICE_FS_DAX so I don't think we should have to worry about DAX
> >> there. Except that the folio code looks like it might have introduced a
> >> bug. AFAICT put_page() always calls
> >> put_devmap_managed_page(&folio->page) but folio_put() does not (although
> >> folios_put() does!). So it seems folio_put() won't end up calling
> >> __put_devmap_managed_page_refs() as I think it should.
> >>
> >> I think you're right about the change to __init_zone_device_page() - I
> >> should limit it to DEVICE_PRIVATE/COHERENT pages only. But I need to
> >> look at Dan's patch series more closely as I suspect it might be better
> >> to rebase this patch on top of that.
> >
> > Apologies for the delay I was travelling the past few days. Yes, I think
> > this patch slots in nicely to avoid the introduction of an init_mode
> > [1]:
> >
> > https://lore.kernel.org/nvdimm/166329940343.2786261.6047770378829215962.stgit@dwillia2-xfh.jf.intel.com/
> >
> > Mind if I steal it into my series?
>
> No problem, although I notice Andrew has already merged it into
> mm-unstable. If you end up rebasing your series on top of mine I think
> all that's needed is a patch somewhere in your series to drop the
> various `if (pgmap->type == MEMORY_DEVICE_*)` I added to (hopefully)
> avoid breaking DAX. Assuming DAX takes a pagemap reference on struct
> page allocation something like below.
Yeah, I'll go that route and rebase on top of -mm.
Thanks again.