drm/xe: Userptr invalid page access fixes

Add proper #ifndef around the xe_hmm.h header, proper spacing and since the documentation mostly follows kerneldoc format, make it kerneldoc. Also prepare for upcoming -stable fixes.

Fixes 81e058a3e7fd ("drm/xe: Introduce helper to populate userptr")

Cc Oak Zeng <[email protected]>

Cc <[email protected]> # v6.10+

Signed-off-by Thomas Hellström <[email protected]>

---
 drivers/gpu/drm/xe/xe_hmm.c | 9 +++------
 drivers/gpu/drm/xe/xe_hmm.h | 5 +++++
 2 files changed, 8 insertions(+), 6 deletions(-)

drivers/gpu/drm/xe/xe_hmm.c +3 -6

index 089834467880..c56738fa713b 100644
@@ -19,11 +19,10 @@ static u64 xe_npages_in_range(unsigned long start, unsigned long end)
 	return (end - start) >> PAGE_SHIFT;
 }
 
-/*
+/**
  * xe_mark_range_accessed() - mark a range is accessed, so core mm
  * have such information for memory eviction or write back to
  * hard disk
- *
  * @range: the range to mark
  * @write: if write to this range, we mark pages in this range
  * as dirty

@@ -43,11 +42,10 @@ static void xe_mark_range_accessed(struct hmm_range *range, bool write)
 	}
 }
 
-/*
+/**
  * xe_build_sg() - build a scatter gather table for all the physical pages/pfn
  * in a hmm_range. dma-map pages if necessary. dma-address is save in sg table
  * and will be used to program GPU page table later.
- *
  * @xe: the xe device who will access the dma-address in sg table
  * @range: the hmm range that we build the sg table from. range->hmm_pfns[]
  * has the pfn numbers of pages that back up this hmm address range.

@@ -112,9 +110,8 @@ static int xe_build_sg(struct xe_device *xe, struct hmm_range *range,
 	return ret;
 }
 
-/*
+/**
  * xe_hmm_userptr_free_sg() - Free the scatter gather table of userptr
- *
  * @uvma: the userptr vma which hold the scatter gather table
  *
  * With function xe_userptr_populate_range, we allocate storage of

drivers/gpu/drm/xe/xe_hmm.h +5 -0

index 909dc2bdcd97..9602cb7d976d 100644
@@ -3,9 +3,14 @@
  * Copyright © 2024 Intel Corporation
  */
 
+#ifndef _XE_HMM_H_
+#define _XE_HMM_H_
+
 #include <linux/types.h>
 
 struct xe_userptr_vma;
 
 int xe_hmm_userptr_populate_range(struct xe_userptr_vma *uvma, bool is_mm_mmap_locked);
+
 void xe_hmm_userptr_free_sg(struct xe_userptr_vma *uvma);
+#endif

Add proper #ifndef around the xe_hmm.h header, proper spacing and since the documentation mostly follows kerneldoc format, make it kerneldoc. Also prepare for upcoming -stable fixes.

Fixes 81e058a3e7fd ("drm/xe: Introduce helper to populate userptr")

Cc Oak Zeng <[email protected]>

Cc <[email protected]> # v6.10+

Signed-off-by Thomas Hellström <[email protected]>

---
  drivers/gpu/drm/xe/xe_hmm.c | 9 +++------
  drivers/gpu/drm/xe/xe_hmm.h | 5 +++++
  2 files changed, 8 insertions(+), 6 deletions(-)

drivers/gpu/drm/xe/xe_hmm.c +3 -6

index 089834467880..c56738fa713b 100644
@@ -19,11 +19,10 @@ static u64 xe_npages_in_range(unsigned long start, unsigned long end)
  	return (end - start) >> PAGE_SHIFT;
  }
 
-/*
+/**
   * xe_mark_range_accessed() - mark a range is accessed, so core mm
   * have such information for memory eviction or write back to
   * hard disk
- *
   * @range: the range to mark
   * @write: if write to this range, we mark pages in this range
   * as dirty

@@ -43,11 +42,10 @@ static void xe_mark_range_accessed(struct hmm_range *range, bool write)
  	}
  }
 
-/*
+/**
   * xe_build_sg() - build a scatter gather table for all the physical pages/pfn
   * in a hmm_range. dma-map pages if necessary. dma-address is save in sg table
   * and will be used to program GPU page table later.
- *
   * @xe: the xe device who will access the dma-address in sg table
   * @range: the hmm range that we build the sg table from. range->hmm_pfns[]
   * has the pfn numbers of pages that back up this hmm address range.

@@ -112,9 +110,8 @@ static int xe_build_sg(struct xe_device *xe, struct hmm_range *range,
  	return ret;
  }
 
-/*
+/**
   * xe_hmm_userptr_free_sg() - Free the scatter gather table of userptr
- *
   * @uvma: the userptr vma which hold the scatter gather table
   *
   * With function xe_userptr_populate_range, we allocate storage of

drivers/gpu/drm/xe/xe_hmm.h +5 -0

index 909dc2bdcd97..9602cb7d976d 100644
@@ -3,9 +3,14 @@
   * Copyright © 2024 Intel Corporation
   */
 
+#ifndef _XE_HMM_H_
+#define _XE_HMM_H_
+
  #include <linux/types.h>
 
  struct xe_userptr_vma;
 
  int xe_hmm_userptr_populate_range(struct xe_userptr_vma *uvma, bool is_mm_mmap_locked);
+
  void xe_hmm_userptr_free_sg(struct xe_userptr_vma *uvma);
+#endif

The pnfs that we obtain from hmm_range_fault() point to pages that we don't have a reference on, and the guarantee that they are still in the cpu page-tables is that the notifier lock must be held and the notifier seqno is still valid.

So while building the sg table and marking the pages accesses / dirty we need to hold this lock with a validated seqno.

However, the lock is reclaim tainted which makes sg_alloc_table_from_pages_segment() unusable, since it internally allocates memory.

Instead build the sg-table manually. For the non-iommu case this might lead to fewer coalesces, but if that's a problem it can be fixed up later in the resource cursor code. For the iommu case, the whole sg-table may still be coalesced to a single contigous device va region.

This avoids marking pages that we don't own dirty and accessed, and it also avoid dereferencing struct pages that we don't own.

v2:
- Use assert to check whether hmm pfns are valid (Matthew Auld)
- Take into account that large pages may cross range boundaries (Matthew Auld)

Fixes 81e058a3e7fd ("drm/xe: Introduce helper to populate userptr")

Cc Oak Zeng <[email protected]>

Cc <[email protected]> # v6.10+

Signed-off-by Thomas Hellström <[email protected]>

---
  drivers/gpu/drm/xe/xe_hmm.c | 119 ++++++++++++++++++++++++++++--------
  1 file changed, 93 insertions(+), 26 deletions(-)

drivers/gpu/drm/xe/xe_hmm.c +3 -0

index c56738fa713b..93cce9e819a1 100644
@@ -42,6 +42,40 @@ static void xe_mark_range_accessed(struct hmm_range *range, bool write)
  	}
  }
 
+static int xe_alloc_sg(struct xe_device *xe, struct sg_table *st,
+		       struct hmm_range *range, struct rw_semaphore *notifier_sem)
+{

drivers/gpu/drm/xe/xe_hmm.c +49 -24

+		return -EAGAIN;
+
+	npages = xe_npages_in_range(range->start, range->end);
+	for (i = 0; i < npages;) {
+		unsigned long len;
+
+		hmm_pfn = range->hmm_pfns[i];
+		xe_assert(xe, hmm_pfn & HMM_PFN_VALID);
+
+		len = 1UL << hmm_pfn_to_map_order(hmm_pfn);
+
+		/* If order > 0 the page may extend beyond range->start */
+		len -= (hmm_pfn & ~HMM_PFN_FLAGS) & (len - 1);
+		i += len;
+		num_chunks++;
+	}
+	up_read(notifier_sem);
+
+	return sg_alloc_table(st, num_chunks, GFP_KERNEL);
+}
+
  /**
   * xe_build_sg() - build a scatter gather table for all the physical pages/pfn
   * in a hmm_range. dma-map pages if necessary. dma-address is save in sg table

@@ -50,6 +84,7 @@ static void xe_mark_range_accessed(struct hmm_range *range, bool write)
   * @range: the hmm range that we build the sg table from. range->hmm_pfns[]
   * has the pfn numbers of pages that back up this hmm address range.
   * @st: pointer to the sg table.
+ * @notifier_sem: The xe notifier lock.
   * @write: whether we write to this range. This decides dma map direction
   * for system pages. If write we map it bi-diretional; otherwise
   * DMA_TO_DEVICE

@@ -76,38 +111,41 @@ static void xe_mark_range_accessed(struct hmm_range *range, bool write)
   * Returns 0 if successful; -ENOMEM if fails to allocate memory
   */
  static int xe_build_sg(struct xe_device *xe, struct hmm_range *range,
-		       struct sg_table *st, bool write)
+		       struct sg_table *st,
+		       struct rw_semaphore *notifier_sem,
+		       bool write)
  {
+	unsigned long npages = xe_npages_in_range(range->start, range->end);
  	struct device *dev = xe->drm.dev;
-	struct page **pages;
-	u64 i, npages;
-	int ret;
+	struct scatterlist *sgl;
+	struct page *page;
+	unsigned long i, j;
 
-	npages = xe_npages_in_range(range->start, range->end);
-	pages = kvmalloc_array(npages, sizeof(*pages), GFP_KERNEL);
-	if (!pages)
-		return -ENOMEM;
+	lockdep_assert_held(notifier_sem);
 
-	for (i = 0; i < npages; i++) {
-		pages[i] = hmm_pfn_to_page(range->hmm_pfns[i]);
-		xe_assert(xe, !is_device_private_page(pages[i]));
-	}
+	i = 0;
+	for_each_sg(st->sgl, sgl, st->nents, j) {
+		unsigned long hmm_pfn, size;
 
-	ret = sg_alloc_table_from_pages_segment(st, pages, npages, 0, npages << PAGE_SHIFT,
-						xe_sg_segment_size(dev), GFP_KERNEL);
-	if (ret)
-		goto free_pages;
+		hmm_pfn = range->hmm_pfns[i];
+		page = hmm_pfn_to_page(hmm_pfn);
+		xe_assert(xe, !is_device_private_page(page));
 
-	ret = dma_map_sgtable(dev, st, write ? DMA_BIDIRECTIONAL : DMA_TO_DEVICE,
-			      DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_NO_KERNEL_MAPPING);
-	if (ret) {
-		sg_free_table(st);
-		st = NULL;
+		size = 1UL << hmm_pfn_to_map_order(hmm_pfn);
+		size -= page_to_pfn(page) & (size - 1);
+		i += size;
+
+		if (unlikely(j == st->nents - 1)) {
+			if (i > npages)
+				size -= (i - npages);
+			sg_mark_end(sgl);
+		}
+		sg_set_page(sgl, page, size << PAGE_SHIFT, 0);
  	}
+	xe_assert(xe, i == npages);
 
-free_pages:
-	kvfree(pages);
-	return ret;
+	return dma_map_sgtable(dev, st, write ? DMA_BIDIRECTIONAL : DMA_TO_DEVICE,
+			       DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_NO_KERNEL_MAPPING);
  }
 
  /**

drivers/gpu/drm/xe/xe_hmm.c +30 -1

+		ret = down_write_killable(&vm->userptr.notifier_lock);
+		if (ret)
+			goto free_pfns;
+
+		xe_hmm_userptr_free_sg(uvma);
+		up_write(&vm->userptr.notifier_lock);
+	}
+
+	ret = xe_alloc_sg(vm->xe, &userptr->sgt, &hmm_range, &vm->userptr.notifier_lock);
  	if (ret)
  		goto free_pfns;
 
+	ret = down_read_interruptible(&vm->userptr.notifier_lock);
+	if (ret)
+		goto free_st;
+
+	if (mmu_interval_read_retry(hmm_range.notifier, hmm_range.notifier_seq)) {
+		ret = -EAGAIN;
+		goto out_unlock;
+	}
+
+	ret = xe_build_sg(vm->xe, &hmm_range, &userptr->sgt,
+			  &vm->userptr.notifier_lock, write);
+	if (ret)
+		goto out_unlock;
+
  	xe_mark_range_accessed(&hmm_range, write);
  	userptr->sg = &userptr->sgt;
  	userptr->notifier_seq = hmm_range.notifier_seq;
+	up_read(&vm->userptr.notifier_lock);
+	kvfree(pfns);
+	return 0;
 
+out_unlock:
+	up_read(&vm->userptr.notifier_lock);
+free_st:
+	sg_free_table(&userptr->sgt);
  free_pfns:
  	kvfree(pfns);
  	return ret;
  }
-

Quoted 62 lines · Thomas Hellström

The pnfs that we obtain from hmm_range_fault() point to pages that we don't have a reference on, and the guarantee that they are still in the cpu page-tables is that the notifier lock must be held and the
notifier seqno is still valid.

So while building the sg table and marking the pages accesses / dirty
we need to hold this lock with a validated seqno.

However, the lock is reclaim tainted which makes sg_alloc_table_from_pages_segment() unusable, since it internally allocates memory.

Instead build the sg-table manually. For the non-iommu case this might lead to fewer coalesces, but if that's a problem it can be fixed up later in the resource cursor code. For the iommu case, the whole sg-table may still be coalesced to a single contigous device va region.

This avoids marking pages that we don't own dirty and accessed, and it also avoid dereferencing struct pages that we don't own.

v2:
- Use assert to check whether hmm pfns are valid (Matthew Auld)
- Take into account that large pages may cross range boundaries (Matthew Auld)

Fixes 81e058a3e7fd ("drm/xe: Introduce helper to populate

userptr")

Cc Oak Zeng <[email protected]>

Cc <[email protected]> # v6.10+

Signed-off-by Thomas Hellström <[email protected]>

---
  drivers/gpu/drm/xe/xe_hmm.c | 119 ++++++++++++++++++++++++++++---
-----

  1 file changed, 93 insertions(+), 26 deletions(-)

drivers/gpu/drm/xe/xe_hmm.c +0 -0

index c56738fa713b..93cce9e819a1 100644
@@ -42,6 +42,40 @@ static void xe_mark_range_accessed(struct
 hmm_range *range, bool write)
   	}
   }

drivers/gpu/drm/xe/xe_hmm.c +2 -0

+static int xe_alloc_sg(struct xe_device *xe, struct sg_table *st,
+		       struct hmm_range *range, struct

rw_semaphore *notifier_sem)

drivers/gpu/drm/xe/xe_hmm.c +11 -0

+{
+	unsigned long i, npages, hmm_pfn;
+	unsigned long num_chunks = 0;
+	int ret;
+
+	/* HMM docs says this is needed. */
+	ret = down_read_interruptible(notifier_sem);
+	if (ret)
+		return ret;
+
+	if (mmu_interval_read_retry(range->notifier, range-

Quoted 1 line

Quoted 1 line

notifier_seq))

up_read() ?

Quoted 119 lines

drivers/gpu/drm/xe/xe_hmm.c +12 -0

+		return -EAGAIN;
+
+	npages = xe_npages_in_range(range->start, range->end);
+	for (i = 0; i < npages;) {
+		unsigned long len;
+
+		hmm_pfn = range->hmm_pfns[i];
+		xe_assert(xe, hmm_pfn & HMM_PFN_VALID);
+
+		len = 1UL << hmm_pfn_to_map_order(hmm_pfn);
+
+		/* If order > 0 the page may extend beyond range-

Quoted 1 line

start */

drivers/gpu/drm/xe/xe_hmm.c +9 -0

+		len -= (hmm_pfn & ~HMM_PFN_FLAGS) & (len - 1);
+		i += len;
+		num_chunks++;
+	}
+	up_read(notifier_sem);
+
+	return sg_alloc_table(st, num_chunks, GFP_KERNEL);
+}
+

  /**
   * xe_build_sg() - build a scatter gather table for all the physical pages/pfn
   * in a hmm_range. dma-map pages if necessary. dma-address is save in sg table

drivers/gpu/drm/xe/xe_hmm.c +0 -0

@@ -50,6 +84,7 @@ static void xe_mark_range_accessed(struct
 hmm_range *range, bool write)
    * @range: the hmm range that we build the sg table from. range-

Quoted 1 line

hmm_pfns[]

   * has the pfn numbers of pages that back up this hmm address range.
   * @st: pointer to the sg table.
+ * @notifier_sem: The xe notifier lock.
   * @write: whether we write to this range. This decides dma map direction
   * for system pages. If write we map it bi-diretional; otherwise
   * DMA_TO_DEVICE

drivers/gpu/drm/xe/xe_hmm.c +0 -0

@@ -76,38 +111,41 @@ static void xe_mark_range_accessed(struct
 hmm_range *range, bool write)
    * Returns 0 if successful; -ENOMEM if fails to allocate memory
    */

  static int xe_build_sg(struct xe_device *xe, struct hmm_range *range,

drivers/gpu/drm/xe/xe_hmm.c +3 -1

-		       struct sg_table *st, bool write)
+		       struct sg_table *st,
+		       struct rw_semaphore *notifier_sem,
+		       bool write)

  {

drivers/gpu/drm/xe/xe_hmm.c +1 -0

+	unsigned long npages = xe_npages_in_range(range->start,

range->end);
   struct device *dev = xe->drm.dev;

drivers/gpu/drm/xe/xe_hmm.c +3 -3

-	struct page **pages;
-	u64 i, npages;
-	int ret;
+	struct scatterlist *sgl;
+	struct page *page;
+	unsigned long i, j;

drivers/gpu/drm/xe/xe_hmm.c +0 -2

-	npages = xe_npages_in_range(range->start, range->end);
-	pages = kvmalloc_array(npages, sizeof(*pages),

GFP_KERNEL);

drivers/gpu/drm/xe/xe_hmm.c +1 -2

-	if (!pages)
-		return -ENOMEM;
+	lockdep_assert_held(notifier_sem);

drivers/gpu/drm/xe/xe_hmm.c +3 -4

-	for (i = 0; i < npages; i++) {
-		pages[i] = hmm_pfn_to_page(range->hmm_pfns[i]);
-		xe_assert(xe, !is_device_private_page(pages[i]));
-	}
+	i = 0;
+	for_each_sg(st->sgl, sgl, st->nents, j) {
+		unsigned long hmm_pfn, size;

drivers/gpu/drm/xe/xe_hmm.c +0 -1

-	ret = sg_alloc_table_from_pages_segment(st, pages, npages,

0, npages << PAGE_SHIFT,
-

						xe_sg_segment_size(dev), GFP_KERNEL);

drivers/gpu/drm/xe/xe_hmm.c +3 -2

-	if (ret)
-		goto free_pages;
+		hmm_pfn = range->hmm_pfns[i];
+		page = hmm_pfn_to_page(hmm_pfn);
+		xe_assert(xe, !is_device_private_page(page));

drivers/gpu/drm/xe/xe_hmm.c +0 -1

-	ret = dma_map_sgtable(dev, st, write ? DMA_BIDIRECTIONAL :

DMA_TO_DEVICE,

drivers/gpu/drm/xe/xe_hmm.c +0 -1

-			      DMA_ATTR_SKIP_CPU_SYNC |

DMA_ATTR_NO_KERNEL_MAPPING);

drivers/gpu/drm/xe/xe_hmm.c +10 -3

-	if (ret) {
-		sg_free_table(st);
-		st = NULL;
+		size = 1UL << hmm_pfn_to_map_order(hmm_pfn);
+		size -= page_to_pfn(page) & (size - 1);
+		i += size;
+
+		if (unlikely(j == st->nents - 1)) {
+			if (i > npages)
+				size -= (i - npages);
+			sg_mark_end(sgl);
+		}
+		sg_set_page(sgl, page, size << PAGE_SHIFT, 0);

   }

drivers/gpu/drm/xe/xe_hmm.c +1 -0

+	xe_assert(xe, i == npages);

drivers/gpu/drm/xe/xe_hmm.c +1 -3

-free_pages:
-	kvfree(pages);
-	return ret;
+	return dma_map_sgtable(dev, st, write ? DMA_BIDIRECTIONAL

: DMA_TO_DEVICE,

drivers/gpu/drm/xe/xe_hmm.c +1 -0

+			       DMA_ATTR_SKIP_CPU_SYNC |

DMA_ATTR_NO_KERNEL_MAPPING);
  }

  /**

drivers/gpu/drm/xe/xe_hmm.c +0 -0

@@ -235,16 +273,45 @@ int xe_hmm_userptr_populate_range(struct
 xe_userptr_vma *uvma,
   	if (ret)
   		goto free_pfns;

drivers/gpu/drm/xe/xe_hmm.c int xe_hmm_userptr_populate_range(struct xe_userptr_vma *uvma, · line 276

Quoted 4 lines

drivers/gpu/drm/xe/xe_hmm.c +0 -1

-	ret = xe_build_sg(vm->xe, &hmm_range, &userptr->sgt,

write);

drivers/gpu/drm/xe/xe_hmm.c +1 -0

+	if (unlikely(userptr->sg)) {

Is it really possible to hit this? We did the unmap above also, although
that was outside of the notifier lock?

Otherwise,

Reviewed-by Matthew Auld <[email protected]>

Quoted 45 lines

drivers/gpu/drm/xe/xe_hmm.c +1 -0

+		ret = down_write_killable(&vm-

Quoted 1 line

userptr.notifier_lock);

drivers/gpu/drm/xe/xe_hmm.c +8 -0

+		if (ret)
+			goto free_pfns;
+
+		xe_hmm_userptr_free_sg(uvma);
+		up_write(&vm->userptr.notifier_lock);
+	}
+
+	ret = xe_alloc_sg(vm->xe, &userptr->sgt, &hmm_range, &vm-

Quoted 1 line

userptr.notifier_lock);

   if (ret)
   goto free_pfns;

drivers/gpu/drm/xe/xe_hmm.c +5 -0

+	ret = down_read_interruptible(&vm->userptr.notifier_lock);
+	if (ret)
+		goto free_st;
+
+	if (mmu_interval_read_retry(hmm_range.notifier,

hmm_range.notifier_seq)) {

drivers/gpu/drm/xe/xe_hmm.c +9 -0

+		ret = -EAGAIN;
+		goto out_unlock;
+	}
+
+	ret = xe_build_sg(vm->xe, &hmm_range, &userptr->sgt,
+			  &vm->userptr.notifier_lock, write);
+	if (ret)
+		goto out_unlock;
+

   xe_mark_range_accessed(&hmm_range, write); userptr->sg = &userptr->sgt; userptr->notifier_seq = hmm_range.notifier_seq;

drivers/gpu/drm/xe/xe_hmm.c +3 -0

+	up_read(&vm->userptr.notifier_lock);
+	kvfree(pfns);
+	return 0;

drivers/gpu/drm/xe/xe_hmm.c +4 -0

+out_unlock:
+	up_read(&vm->userptr.notifier_lock);
+free_st:
+	sg_free_table(&userptr->sgt);

  free_pfns:
   kvfree(pfns);
   return ret;
  }
-

If userptr pages are freed after a call to the xe mmu notifier, the device will not be blocked out from theoretically accessing these pages unless they are also unmapped from the iommu, and this violates some aspects of the iommu-imposed security.

Ensure that userptrs are unmapped in the mmu notifier to mitigate this. A naive attempt would try to free the sg table, but the sg table itself may be accessed by a concurrent bind operation, so settle for unly unmapping.

Fixes 81e058a3e7fd ("drm/xe: Introduce helper to populate userptr")

Cc Oak Zeng <[email protected]>

Cc <[email protected]> # v6.10+

Signed-off-by Thomas Hellström <[email protected]>