[PATCH 1/5] drm/i915/gtt: Add read only pages to gen8_pte_encode

[PATCH 1/5] drm/i915/gtt: Add read only pages to gen8_pte_encode

[Chris Wilson]

From: Jon Bloomfield <jon.bloomfield@intel.com>

We can set a bit inside the ppGTT PTE to indicate a page is read-only;
writes from the GPU will be discarded. We can use this to protect pages
and in particular support read-only userptr mappings (necessary for
importing PROT_READ vma).

Signed-off-by: Jon Bloomfield <jon.bloomfield@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Matthew Auld <matthew.william.auld@gmail.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 7f4def556f40..5936f0bfdd19 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -244,10 +244,13 @@ static void clear_pages(struct i915_vma *vma)
 }
 
 static gen8_pte_t gen8_pte_encode(dma_addr_t addr,
-				  enum i915_cache_level level)
+				  enum i915_cache_level level,
+				  u32 flags)
 {
-	gen8_pte_t pte = _PAGE_PRESENT | _PAGE_RW;
-	pte |= addr;
+	gen8_pte_t pte = addr | _PAGE_PRESENT | _PAGE_RW;
+
+	if (unlikely(flags & PTE_READ_ONLY))
+		pte &= ~_PAGE_RW;
 
 	switch (level) {
 	case I915_CACHE_NONE:
@@ -637,7 +640,7 @@ static void gen8_initialize_pt(struct i915_address_space *vm,
 			       struct i915_page_table *pt)
 {
 	fill_px(vm, pt,
-		gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC));
+		gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC, 0));
 }
 
 static void gen6_initialize_pt(struct i915_address_space *vm,
@@ -833,7 +836,7 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
 	unsigned int pte = gen8_pte_index(start);
 	unsigned int pte_end = pte + num_entries;
 	const gen8_pte_t scratch_pte =
-		gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC);
+		gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC, 0);
 	gen8_pte_t *vaddr;
 
 	GEM_BUG_ON(num_entries > pt->used_ptes);
@@ -1008,7 +1011,7 @@ gen8_ppgtt_insert_pte_entries(struct i915_hw_ppgtt *ppgtt,
 			      enum i915_cache_level cache_level)
 {
 	struct i915_page_directory *pd;
-	const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level);
+	const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, 0);
 	gen8_pte_t *vaddr;
 	bool ret;
 
@@ -1076,7 +1079,7 @@ static void gen8_ppgtt_insert_huge_entries(struct i915_vma *vma,
 					   struct sgt_dma *iter,
 					   enum i915_cache_level cache_level)
 {
-	const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level);
+	const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, 0);
 	u64 start = vma->node.start;
 	dma_addr_t rem = iter->sg->length;
 
@@ -1542,7 +1545,7 @@ static void gen8_dump_ppgtt(struct i915_hw_ppgtt *ppgtt, struct seq_file *m)
 {
 	struct i915_address_space *vm = &ppgtt->base;
 	const gen8_pte_t scratch_pte =
-		gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC);
+		gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC, 0);
 	u64 start = 0, length = ppgtt->base.total;
 
 	if (use_4lvl(vm)) {
@@ -2419,7 +2422,7 @@ static void gen8_ggtt_insert_page(struct i915_address_space *vm,
 	gen8_pte_t __iomem *pte =
 		(gen8_pte_t __iomem *)ggtt->gsm + (offset >> PAGE_SHIFT);
 
-	gen8_set_pte(pte, gen8_pte_encode(addr, level));
+	gen8_set_pte(pte, gen8_pte_encode(addr, level, 0));
 
 	ggtt->invalidate(vm->i915);
 }
@@ -2432,7 +2435,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
 	struct sgt_iter sgt_iter;
 	gen8_pte_t __iomem *gtt_entries;
-	const gen8_pte_t pte_encode = gen8_pte_encode(0, level);
+	const gen8_pte_t pte_encode = gen8_pte_encode(0, level, 0);
 	dma_addr_t addr;
 
 	gtt_entries = (gen8_pte_t __iomem *)ggtt->gsm;
@@ -2500,7 +2503,7 @@ static void gen8_ggtt_clear_range(struct i915_address_space *vm,
 	unsigned first_entry = start >> PAGE_SHIFT;
 	unsigned num_entries = length >> PAGE_SHIFT;
 	const gen8_pte_t scratch_pte =
-		gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC);
+		gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC, 0);
 	gen8_pte_t __iomem *gtt_base =
 		(gen8_pte_t __iomem *)ggtt->gsm + first_entry;
 	const int max_entries = ggtt_total_entries(ggtt) - first_entry;
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] drm/i915/gtt: Read-only pages for insert_entries on bdw+

[Chris Wilson]

From: Jon Bloomfield <jon.bloomfield@intel.com>

Hook up the flags to allow read-only ppGTT mappings for gen8+

Signed-off-by: Jon Bloomfield <jon.bloomfield@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Matthew Auld <matthew.william.auld@gmail.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c     | 45 ++++++++++++++++---------
 drivers/gpu/drm/i915/i915_gem_gtt.h     |  7 +++-
 drivers/gpu/drm/i915/intel_ringbuffer.c | 11 ++++--
 3 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 5936f0bfdd19..1ac626cacc8d 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -204,7 +204,7 @@ static int ppgtt_bind_vma(struct i915_vma *vma,
 			return ret;
 	}
 
-	/* Currently applicable only to VLV */
+	/* Applicable to VLV, and gen8+ */
 	pte_flags = 0;
 	if (vma->obj->gt_ro)
 		pte_flags |= PTE_READ_ONLY;
@@ -1008,10 +1008,11 @@ gen8_ppgtt_insert_pte_entries(struct i915_hw_ppgtt *ppgtt,
 			      struct i915_page_directory_pointer *pdp,
 			      struct sgt_dma *iter,
 			      struct gen8_insert_pte *idx,
-			      enum i915_cache_level cache_level)
+			      enum i915_cache_level cache_level,
+			      u32 flags)
 {
 	struct i915_page_directory *pd;
-	const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, 0);
+	const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, flags);
 	gen8_pte_t *vaddr;
 	bool ret;
 
@@ -1062,14 +1063,14 @@ gen8_ppgtt_insert_pte_entries(struct i915_hw_ppgtt *ppgtt,
 static void gen8_ppgtt_insert_3lvl(struct i915_address_space *vm,
 				   struct i915_vma *vma,
 				   enum i915_cache_level cache_level,
-				   u32 unused)
+				   u32 flags)
 {
 	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
 	struct sgt_dma iter = sgt_dma(vma);
 	struct gen8_insert_pte idx = gen8_insert_pte(vma->node.start);
 
 	gen8_ppgtt_insert_pte_entries(ppgtt, &ppgtt->pdp, &iter, &idx,
-				      cache_level);
+				      cache_level, flags);
 
 	vma->page_sizes.gtt = I915_GTT_PAGE_SIZE;
 }
@@ -1077,9 +1078,10 @@ static void gen8_ppgtt_insert_3lvl(struct i915_address_space *vm,
 static void gen8_ppgtt_insert_huge_entries(struct i915_vma *vma,
 					   struct i915_page_directory_pointer **pdps,
 					   struct sgt_dma *iter,
-					   enum i915_cache_level cache_level)
+					   enum i915_cache_level cache_level,
+					   u32 flags)
 {
-	const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, 0);
+	const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, flags);
 	u64 start = vma->node.start;
 	dma_addr_t rem = iter->sg->length;
 
@@ -1195,19 +1197,21 @@ static void gen8_ppgtt_insert_huge_entries(struct i915_vma *vma,
 static void gen8_ppgtt_insert_4lvl(struct i915_address_space *vm,
 				   struct i915_vma *vma,
 				   enum i915_cache_level cache_level,
-				   u32 unused)
+				   u32 flags)
 {
 	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
 	struct sgt_dma iter = sgt_dma(vma);
 	struct i915_page_directory_pointer **pdps = ppgtt->pml4.pdps;
 
 	if (vma->page_sizes.sg > I915_GTT_PAGE_SIZE) {
-		gen8_ppgtt_insert_huge_entries(vma, pdps, &iter, cache_level);
+		gen8_ppgtt_insert_huge_entries(vma, pdps, &iter, cache_level,
+					       flags);
 	} else {
 		struct gen8_insert_pte idx = gen8_insert_pte(vma->node.start);
 
 		while (gen8_ppgtt_insert_pte_entries(ppgtt, pdps[idx.pml4e++],
-						     &iter, &idx, cache_level))
+						     &iter, &idx, cache_level,
+						     flags))
 			GEM_BUG_ON(idx.pml4e >= GEN8_PML4ES_PER_PML4);
 
 		vma->page_sizes.gtt = I915_GTT_PAGE_SIZE;
@@ -1614,6 +1618,9 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 		1ULL << 48 :
 		1ULL << 32;
 
+	/* From bdw, there is support for read-only pages in the PPGTT */
+	ppgtt->base.has_read_only = true;
+
 	/* There are only few exceptions for gen >=6. chv and bxt.
 	 * And we are not sure about the latter so play safe for now.
 	 */
@@ -2430,7 +2437,7 @@ static void gen8_ggtt_insert_page(struct i915_address_space *vm,
 static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 				     struct i915_vma *vma,
 				     enum i915_cache_level level,
-				     u32 unused)
+				     u32 flags)
 {
 	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
 	struct sgt_iter sgt_iter;
@@ -2438,6 +2445,9 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 	const gen8_pte_t pte_encode = gen8_pte_encode(0, level, 0);
 	dma_addr_t addr;
 
+	/* The GTT does not support read-only mappings */
+	GEM_BUG_ON(flags & PTE_READ_ONLY);
+
 	gtt_entries = (gen8_pte_t __iomem *)ggtt->gsm;
 	gtt_entries += vma->node.start >> PAGE_SHIFT;
 	for_each_sgt_dma(addr, sgt_iter, vma->pages)
@@ -2564,13 +2574,14 @@ struct insert_entries {
 	struct i915_address_space *vm;
 	struct i915_vma *vma;
 	enum i915_cache_level level;
+	u32 flags;
 };
 
 static int bxt_vtd_ggtt_insert_entries__cb(void *_arg)
 {
 	struct insert_entries *arg = _arg;
 
-	gen8_ggtt_insert_entries(arg->vm, arg->vma, arg->level, 0);
+	gen8_ggtt_insert_entries(arg->vm, arg->vma, arg->level, arg->flags);
 	bxt_vtd_ggtt_wa(arg->vm);
 
 	return 0;
@@ -2579,9 +2590,9 @@ static int bxt_vtd_ggtt_insert_entries__cb(void *_arg)
 static void bxt_vtd_ggtt_insert_entries__BKL(struct i915_address_space *vm,
 					     struct i915_vma *vma,
 					     enum i915_cache_level level,
-					     u32 unused)
+					     u32 flags)
 {
-	struct insert_entries arg = { vm, vma, level };
+	struct insert_entries arg = { vm, vma, level, flags };
 
 	stop_machine(bxt_vtd_ggtt_insert_entries__cb, &arg, NULL);
 }
@@ -2672,7 +2683,7 @@ static int ggtt_bind_vma(struct i915_vma *vma,
 	struct drm_i915_gem_object *obj = vma->obj;
 	u32 pte_flags;
 
-	/* Currently applicable only to VLV */
+	/* Applicable to VLV (gen8+ do not support RO in the GGTT) */
 	pte_flags = 0;
 	if (obj->gt_ro)
 		pte_flags |= PTE_READ_ONLY;
@@ -3555,6 +3566,10 @@ int i915_ggtt_init_hw(struct drm_i915_private *dev_priv)
 	 */
 	mutex_lock(&dev_priv->drm.struct_mutex);
 	i915_address_space_init(&ggtt->base, dev_priv, "[global]");
+
+	/* Only VLV supports read-only GGTT mappings */
+	ggtt->base.has_read_only = IS_VALLEYVIEW(dev_priv);
+
 	if (!HAS_LLC(dev_priv) && !USES_PPGTT(dev_priv))
 		ggtt->base.mm.color_adjust = i915_gtt_color_adjust;
 	mutex_unlock(&dev_priv->drm.struct_mutex);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index aec4f73574f4..b0929b547846 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -309,7 +309,12 @@ struct i915_address_space {
 	struct list_head unbound_list;
 
 	struct pagevec free_pages;
-	bool pt_kmap_wc;
+
+	/* Some systems require uncached updates of the page directories */
+	bool pt_kmap_wc:1;
+
+	/* Some systems support read-only mappings for GGTT and/or PPGTT */
+	bool has_read_only:1;
 
 	/* FIXME: Need a more generic return type */
 	gen6_pte_t (*pte_encode)(dma_addr_t addr,
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 97b38bbb7ce2..6cac4ce59438 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1111,6 +1111,7 @@ void intel_ring_unpin(struct intel_ring *ring)
 static struct i915_vma *
 intel_ring_create_vma(struct drm_i915_private *dev_priv, int size)
 {
+	struct i915_address_space *vm = &dev_priv->ggtt.base;
 	struct drm_i915_gem_object *obj;
 	struct i915_vma *vma;
 
@@ -1120,10 +1121,14 @@ intel_ring_create_vma(struct drm_i915_private *dev_priv, int size)
 	if (IS_ERR(obj))
 		return ERR_CAST(obj);
 
-	/* mark ring buffers as read-only from GPU side by default */
-	obj->gt_ro = 1;
+	/*
+	 * Mark ring buffers as read-only from GPU side (so no stray overwrites)
+	 * if supported by the platform's GGTT.
+	 */
+	if (vm->has_read_only)
+		obj->gt_ro = 1;
 
-	vma = i915_vma_instance(obj, &dev_priv->ggtt.base, NULL);
+	vma = i915_vma_instance(obj, vm, NULL);
 	if (IS_ERR(vma))
 		goto err;
 
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5] drm/i915: Prevent writing into a read-only object via a GGTT mmap

[Chris Wilson]

If the user has created a read-only object, they should not be allowed
to circumvent the write protection by using a GGTT mmapping. Deny it.

Also most machines do not support read-only GGTT PTEs, so again we have
to reject attempted writes. Fortunately, this is known a priori, so we
can at least reject in the call to create the mmap with backup in the
fault handler. This is a little draconian as we could blatantly ignore
the write protection on the pages, but it is far simply to keep the
readonly object pure. (It is easier to lift a restriction than to impose
it later!)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Matthew Auld <matthew.william.auld@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 530d6d0109b4..e55278fadf9c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2005,6 +2005,10 @@ int i915_gem_fault(struct vm_fault *vmf)
 	unsigned int flags;
 	int ret;
 
+	/* Sanity check that we allow writing into this object */
+	if (obj->gt_ro && (write || !ggtt->base.has_read_only))
+		return VM_FAULT_SIGBUS;
+
 	/* We don't use vmf->pgoff since that has the fake offset */
 	page_offset = (vmf->address - area->vm_start) >> PAGE_SHIFT;
 
@@ -2291,10 +2295,17 @@ i915_gem_mmap_gtt(struct drm_file *file,
 	if (!obj)
 		return -ENOENT;
 
+	/* If we will not be able to create the GGTT vma, reject it early. */
+	if (obj->gt_ro && !to_i915(dev)->ggtt.base.has_read_only) {
+		ret = -ENODEV;
+		goto out;
+	}
+
 	ret = i915_gem_object_create_mmap_offset(obj);
 	if (ret == 0)
 		*offset = drm_vma_node_offset_addr(&obj->base.vma_node);
 
+out:
 	i915_gem_object_put(obj);
 	return ret;
 }
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm/i915: Reject attempted pwrites into a read-only object

[Chris Wilson]

If the user created a read-only object, they should not be allowed to
circumvent the write protection using the pwrite ioctl.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Matthew Auld <matthew.william.auld@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e55278fadf9c..f359ee507eb5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1619,6 +1619,12 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 		goto err;
 	}
 
+	/* Writes not allowed into this read-only object */
+	if (obj->gt_ro) {
+		ret = -EINVAL;
+		goto err;
+	}
+
 	trace_i915_gem_object_pwrite(obj, args->offset, args->size);
 
 	ret = -ENODEV;
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/5] drm/i915/userptr: Enable read-only support on gen8+

[Chris Wilson]

On gen8 and onwards, we can mark GPU accesses through the ppGTT as being
read-only, that is cause any GPU write onto that page to be discarded
(not triggering a fault). This is all that we need to finally support
the read-only flag for userptr!

Testcase: igt/gem_userptr_blits/readonly*
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 854bd51b9478..d4ee8fa4c379 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -789,10 +789,12 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
 		return -EFAULT;
 
 	if (args->flags & I915_USERPTR_READ_ONLY) {
-		/* On almost all of the current hw, we cannot tell the GPU that a
-		 * page is readonly, so this is just a placeholder in the uAPI.
+		/*
+		 * On almost all of the older hw, we cannot tell the GPU that
+		 * a page is readonly.
 		 */
-		return -ENODEV;
+		if (INTEL_GEN(dev_priv) < 8 || !USES_PPGTT(dev_priv))
+			return -ENODEV;
 	}
 
 	obj = i915_gem_object_alloc(dev_priv);
@@ -806,7 +808,10 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
 	i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
 
 	obj->userptr.ptr = args->user_ptr;
-	obj->userptr.read_only = !!(args->flags & I915_USERPTR_READ_ONLY);
+	if (args->flags & I915_USERPTR_READ_ONLY) {
+		obj->userptr.read_only = true;
+		obj->gt_ro = true;
+	}
 
 	/* And keep a pointer to the current->mm for resolving the user pages
 	 * at binding. This means that we need to hook into the mmu_notifier
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/5] drm/i915/gtt: Add read only pages to gen8_pte_encode

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915/gtt: Add read only pages to gen8_pte_encode
URL   : https://patchwork.freedesktop.org/series/44022/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
401a4686bb18 drm/i915/gtt: Add read only pages to gen8_pte_encode
7573e440233e drm/i915/gtt: Read-only pages for insert_entries on bdw+
-:184: WARNING:BOOL_BITFIELD: Avoid using bool as bitfield.  Prefer bool bitfields as unsigned int or u<8|16|32>
#184: FILE: drivers/gpu/drm/i915/i915_gem_gtt.h:314:
+	bool pt_kmap_wc:1;

-:187: WARNING:BOOL_BITFIELD: Avoid using bool as bitfield.  Prefer bool bitfields as unsigned int or u<8|16|32>
#187: FILE: drivers/gpu/drm/i915/i915_gem_gtt.h:317:
+	bool has_read_only:1;

total: 0 errors, 2 warnings, 0 checks, 180 lines checked
34af85d4bc45 drm/i915: Prevent writing into a read-only object via a GGTT mmap
541b6690dd24 drm/i915: Reject attempted pwrites into a read-only object
5d752b790438 drm/i915/userptr: Enable read-only support on gen8+

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/5] drm/i915/gtt: Add read only pages to gen8_pte_encode

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915/gtt: Add read only pages to gen8_pte_encode
URL   : https://patchwork.freedesktop.org/series/44022/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4264 -> Patchwork_9156 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_9156 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9156, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/44022/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9156:

  === IGT changes ===

    ==== Possible regressions ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-r:           PASS -> DMESG-WARN
      fi-skl-6600u:       PASS -> DMESG-WARN

    
    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        PASS -> SKIP

    
== Known issues ==

  Here are the changes found in Patchwork_9156 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_busy@basic-flip-c:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106097)

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-4200u:       PASS -> DMESG-FAIL (fdo#102614, fdo#106103)
      fi-hsw-peppy:       PASS -> DMESG-FAIL (fdo#102614, fdo#106103)

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         PASS -> FAIL (fdo#104008)

    
    ==== Possible fixes ====

    igt@drv_module_reload@basic-reload-inject:
      fi-glk-j4005:       DMESG-WARN (fdo#106248, fdo#106725) -> PASS

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7500u:       DMESG-WARN (fdo#105128) -> PASS

    igt@kms_pipe_crc_basic@read-crc-pipe-b-frame-sequence:
      fi-glk-j4005:       FAIL (fdo#103481) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS
      fi-cnl-psr:         DMESG-WARN (fdo#104951) -> PASS

    
  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#103481 https://bugs.freedesktop.org/show_bug.cgi?id=103481
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#104951 https://bugs.freedesktop.org/show_bug.cgi?id=104951
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
  fdo#106103 https://bugs.freedesktop.org/show_bug.cgi?id=106103
  fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725


== Participating hosts (43 -> 40) ==

  Missing    (3): fi-ctg-p8600 fi-ilk-m540 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4264 -> Patchwork_9156

  CI_DRM_4264: e267b381ac5a28004f13ed2dbbbab9e9230a264c @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4502: 63f0bf3d50b70f011b9d600a1a4bc1a1c7720654 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9156: 5d752b79043859f0b484c6d1629801351b33fb90 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

5d752b790438 drm/i915/userptr: Enable read-only support on gen8+
541b6690dd24 drm/i915: Reject attempted pwrites into a read-only object
34af85d4bc45 drm/i915: Prevent writing into a read-only object via a GGTT mmap
7573e440233e drm/i915/gtt: Read-only pages for insert_entries on bdw+
401a4686bb18 drm/i915/gtt: Add read only pages to gen8_pte_encode

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9156/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/gtt: Add read only pages to gen8_pte_encode

[Matthew Auld]

On 31 May 2018 at 12:35, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> From: Jon Bloomfield <jon.bloomfield@intel.com>
>
> We can set a bit inside the ppGTT PTE to indicate a page is read-only;
> writes from the GPU will be discarded. We can use this to protect pages
> and in particular support read-only userptr mappings (necessary for
> importing PROT_READ vma).
>
> Signed-off-by: Jon Bloomfield <jon.bloomfield@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Matthew Auld <matthew.william.auld@gmail.com>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

fwiw I didn't see any surprises with ro 2M/64K PTEs when testing locally.

For the series:
Reviewed-by: Matthew Auld <matthew.william.auld@gmail.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/gtt: Add read only pages to gen8_pte_encode

[Chris Wilson]

Quoting Matthew Auld (2018-05-31 15:43:14)
> On 31 May 2018 at 12:35, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > From: Jon Bloomfield <jon.bloomfield@intel.com>
> >
> > We can set a bit inside the ppGTT PTE to indicate a page is read-only;
> > writes from the GPU will be discarded. We can use this to protect pages
> > and in particular support read-only userptr mappings (necessary for
> > importing PROT_READ vma).
> >
> > Signed-off-by: Jon Bloomfield <jon.bloomfield@intel.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Matthew Auld <matthew.william.auld@gmail.com>
> > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> 
> fwiw I didn't see any surprises with ro 2M/64K PTEs when testing locally.
> 
> For the series:
> Reviewed-by: Matthew Auld <matthew.william.auld@gmail.com>

I think I'd like to see a selftest also exercising the ro nature before
pushing. Thanks for the review, stick around there'll be another patch
to come :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via a GGTT mmap

[Joonas Lahtinen]

On Thu, 2018-05-31 at 12:35 +0100, Chris Wilson wrote:
> If the user has created a read-only object, they should not be allowed
> to circumvent the write protection by using a GGTT mmapping. Deny it.
> 
> Also most machines do not support read-only GGTT PTEs, so again we have
> to reject attempted writes. Fortunately, this is known a priori, so we
> can at least reject in the call to create the mmap with backup in the
> fault handler. This is a little draconian as we could blatantly ignore
> the write protection on the pages, but it is far simply to keep the
> readonly object pure. (It is easier to lift a restriction than to impose
> it later!)
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Matthew Auld <matthew.william.auld@gmail.com>

As discussed in IRC, this is no change to previous uAPI as read-only
objects have not been available previously.

And even afterwards, only GTT_MMAP on RO userptr will be rejected for
platforms that don't have RO pages, so that's part of the newly
implemented RO userptr.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Reject attempted pwrites into a read-only object

[Joonas Lahtinen]

On Thu, 2018-05-31 at 12:35 +0100, Chris Wilson wrote:
> If the user created a read-only object, they should not be allowed to
> circumvent the write protection using the pwrite ioctl.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Matthew Auld <matthew.william.auld@gmail.com>

This asks for some igt's and selftests.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915/userptr: Enable read-only support on gen8+

[Joonas Lahtinen]

On Thu, 2018-05-31 at 12:35 +0100, Chris Wilson wrote:
> On gen8 and onwards, we can mark GPU accesses through the ppGTT as being
> read-only, that is cause any GPU write onto that page to be discarded
> (not triggering a fault). This is all that we need to finally support
> the read-only flag for userptr!
> 
> Testcase: igt/gem_userptr_blits/readonly*
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Please add clarification to the commit message that this expands uAPI
(by supporting RO userptrs), so Rodrigo will notice to highlight for
-next PR.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915/userptr: Enable read-only support on gen8+

[Chris Wilson]

Quoting Joonas Lahtinen (2018-06-01 11:21:43)
> On Thu, 2018-05-31 at 12:35 +0100, Chris Wilson wrote:
> > On gen8 and onwards, we can mark GPU accesses through the ppGTT as being
> > read-only, that is cause any GPU write onto that page to be discarded
> > (not triggering a fault). This is all that we need to finally support
> > the read-only flag for userptr!
> > 
> > Testcase: igt/gem_userptr_blits/readonly*
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> 
> Please add clarification to the commit message that this expands uAPI
> (by supporting RO userptrs), so Rodrigo will notice to highlight for
> -next PR.

Ha, I was playing the "this was already in the uAPI" card,
just never implemented so hadn't made it in the uABI. :)

Being able to import read-only shm segments (or PROT_READ file mmaps)
was always on my wishlist, just at the time the only prospect was
special case supporting it for byt (not very appealing).

PR material it is then.
-Chris

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Reject attempted pwrites into a read-only object

[Chris Wilson]

Quoting Joonas Lahtinen (2018-06-01 11:19:07)
> On Thu, 2018-05-31 at 12:35 +0100, Chris Wilson wrote:
> > If the user created a read-only object, they should not be allowed to
> > circumvent the write protection using the pwrite ioctl.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Matthew Auld <matthew.william.auld@gmail.com>
> 
> This asks for some igt's and selftests.

uABI, so hard for a selftest. (Or more to the point, I haven't figured
out how we are supposed to fake userspace memory from inside the kernel.
set_fs()? Maybe fork_usermode_helper? Hmm. So really I've been focusing
on uABI coverage in igt, and forcing the corner cases in selftests where
we can control faults much more easily.) The first round of igt was
already posted; the plan was to extend that test to cover poking at it
through the different holes in the uapi.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5] drm/i915: Prevent writing into a read-only object via a GGTT mmap

[Chris Wilson]

If the user has created a read-only object, they should not be allowed
to circumvent the write protection by using a GGTT mmapping. Deny it.

Also most machines do not support read-only GGTT PTEs, so again we have
to reject attempted writes. Fortunately, this is known a priori, so we
can at least reject in the call to create the mmap with backup in the
fault handler. This is a little draconian as we could blatantly ignore
the write protection on the pages, but it is far simply to keep the
readonly object pure. (It is easier to lift a restriction than to impose
it later!)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Matthew Auld <matthew.william.auld@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8dd4d35655af..d5eb73f7a90a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2009,6 +2009,10 @@ vm_fault_t i915_gem_fault(struct vm_fault *vmf)
 	unsigned int flags;
 	int ret;
 
+	/* Sanity check that we allow writing into this object */
+	if (obj->gt_ro && (write || !ggtt->vm.has_read_only))
+		return VM_FAULT_SIGBUS;
+
 	/* We don't use vmf->pgoff since that has the fake offset */
 	page_offset = (vmf->address - area->vm_start) >> PAGE_SHIFT;
 
@@ -2288,10 +2292,17 @@ i915_gem_mmap_gtt(struct drm_file *file,
 	if (!obj)
 		return -ENOENT;
 
+	/* If we will not be able to create the GGTT vma, reject it early. */
+	if (obj->gt_ro && !to_i915(dev)->ggtt.vm.has_read_only) {
+		ret = -ENODEV;
+		goto out;
+	}
+
 	ret = i915_gem_object_create_mmap_offset(obj);
 	if (ret == 0)
 		*offset = drm_vma_node_offset_addr(&obj->base.vma_node);
 
+out:
 	i915_gem_object_put(obj);
 	return ret;
 }
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via a GGTT mmap

[Bloomfield, Jon]

> -----Original Message-----
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Sent: Thursday, June 14, 2018 5:00 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Chris Wilson <chris@chris-wilson.co.uk>; Bloomfield, Jon
> <jon.bloomfield@intel.com>; Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com>; Matthew Auld
> <matthew.william.auld@gmail.com>
> Subject: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via a
> GGTT mmap
> 
> If the user has created a read-only object, they should not be allowed
> to circumvent the write protection by using a GGTT mmapping. Deny it.
> 
> Also most machines do not support read-only GGTT PTEs, so again we have
> to reject attempted writes. Fortunately, this is known a priori, so we
> can at least reject in the call to create the mmap with backup in the
> fault handler. This is a little draconian as we could blatantly ignore
> the write protection on the pages, but it is far simply to keep the
> readonly object pure. (It is easier to lift a restriction than to impose
> it later!)
Are you sure this is necessary? I assumed you would just create a ro IA
mapping to the page, irrespective of the ability of ggtt. It feels wrong to
disallow mapping a read-only object to the CPU as read-only. With ppgtt
the presence of an unprotected mapping in the ggtt should be immune
from tampering in the GT, so only the cpu mapping should really matter.

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Matthew Auld <matthew.william.auld@gmail.com>
> ---

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via a GGTT mmap

[Chris Wilson]

Quoting Bloomfield, Jon (2018-06-14 15:53:13)
> > -----Original Message-----
> > From: Chris Wilson <chris@chris-wilson.co.uk>
> > Sent: Thursday, June 14, 2018 5:00 AM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>; Bloomfield, Jon
> > <jon.bloomfield@intel.com>; Joonas Lahtinen
> > <joonas.lahtinen@linux.intel.com>; Matthew Auld
> > <matthew.william.auld@gmail.com>
> > Subject: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via a
> > GGTT mmap
> > 
> > If the user has created a read-only object, they should not be allowed
> > to circumvent the write protection by using a GGTT mmapping. Deny it.
> > 
> > Also most machines do not support read-only GGTT PTEs, so again we have
> > to reject attempted writes. Fortunately, this is known a priori, so we
> > can at least reject in the call to create the mmap with backup in the
> > fault handler. This is a little draconian as we could blatantly ignore
> > the write protection on the pages, but it is far simply to keep the
> > readonly object pure. (It is easier to lift a restriction than to impose
> > it later!)
> Are you sure this is necessary? I assumed you would just create a ro IA
> mapping to the page, irrespective of the ability of ggtt.

You are thinking of the CPU mmap? The GTT mmap offers a linear view of
the tiled object. It would be very wrong for us to bypass the PROT_READ
protection of a user page by accessing it via the GTT.

> It feels wrong to
> disallow mapping a read-only object to the CPU as read-only. With ppgtt
> the presence of an unprotected mapping in the ggtt should be immune
> from tampering in the GT, so only the cpu mapping should really matter.

And the CPU mapping has its protection bits on the IA PTE.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via a GGTT mmap

[Bloomfield, Jon]

> -----Original Message-----
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Sent: Thursday, June 14, 2018 8:00 AM
> To: Bloomfield, Jon <jon.bloomfield@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Matthew Auld
> <matthew.william.auld@gmail.com>
> Subject: RE: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via
> a GGTT mmap
> 
> Quoting Bloomfield, Jon (2018-06-14 15:53:13)
> > > -----Original Message-----
> > > From: Chris Wilson <chris@chris-wilson.co.uk>
> > > Sent: Thursday, June 14, 2018 5:00 AM
> > > To: intel-gfx@lists.freedesktop.org
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>; Bloomfield, Jon
> > > <jon.bloomfield@intel.com>; Joonas Lahtinen
> > > <joonas.lahtinen@linux.intel.com>; Matthew Auld
> > > <matthew.william.auld@gmail.com>
> > > Subject: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via
> a
> > > GGTT mmap
> > >
> > > If the user has created a read-only object, they should not be allowed
> > > to circumvent the write protection by using a GGTT mmapping. Deny it.
> > >
> > > Also most machines do not support read-only GGTT PTEs, so again we
> have
> > > to reject attempted writes. Fortunately, this is known a priori, so we
> > > can at least reject in the call to create the mmap with backup in the
> > > fault handler. This is a little draconian as we could blatantly ignore
> > > the write protection on the pages, but it is far simply to keep the
> > > readonly object pure. (It is easier to lift a restriction than to impose
> > > it later!)
> > Are you sure this is necessary? I assumed you would just create a ro IA
> > mapping to the page, irrespective of the ability of ggtt.
> 
> You are thinking of the CPU mmap? The GTT mmap offers a linear view of
> the tiled object. It would be very wrong for us to bypass the PROT_READ
> protection of a user page by accessing it via the GTT.
No, I was thinking of gtt mmap. That requires both GTT and IA PTE mappings
right? One to map the object into the GTT, and then a second to point the
IA at the aperture. Why wouldn't marking the IA mapping RO protect the
object if the GT cannot reach the GTT mapping (from user batches).

> 
> > It feels wrong to
> > disallow mapping a read-only object to the CPU as read-only. With ppgtt
> > the presence of an unprotected mapping in the ggtt should be immune
> > from tampering in the GT, so only the cpu mapping should really matter.
> 
> And the CPU mapping has its protection bits on the IA PTE.
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via a GGTT mmap

[Chris Wilson]

Quoting Bloomfield, Jon (2018-06-14 16:06:40)
> > -----Original Message-----
> > From: Chris Wilson <chris@chris-wilson.co.uk>
> > Sent: Thursday, June 14, 2018 8:00 AM
> > To: Bloomfield, Jon <jon.bloomfield@intel.com>; intel-
> > gfx@lists.freedesktop.org
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Matthew Auld
> > <matthew.william.auld@gmail.com>
> > Subject: RE: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via
> > a GGTT mmap
> > 
> > Quoting Bloomfield, Jon (2018-06-14 15:53:13)
> > > > -----Original Message-----
> > > > From: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Sent: Thursday, June 14, 2018 5:00 AM
> > > > To: intel-gfx@lists.freedesktop.org
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>; Bloomfield, Jon
> > > > <jon.bloomfield@intel.com>; Joonas Lahtinen
> > > > <joonas.lahtinen@linux.intel.com>; Matthew Auld
> > > > <matthew.william.auld@gmail.com>
> > > > Subject: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via
> > a
> > > > GGTT mmap
> > > >
> > > > If the user has created a read-only object, they should not be allowed
> > > > to circumvent the write protection by using a GGTT mmapping. Deny it.
> > > >
> > > > Also most machines do not support read-only GGTT PTEs, so again we
> > have
> > > > to reject attempted writes. Fortunately, this is known a priori, so we
> > > > can at least reject in the call to create the mmap with backup in the
> > > > fault handler. This is a little draconian as we could blatantly ignore
> > > > the write protection on the pages, but it is far simply to keep the
> > > > readonly object pure. (It is easier to lift a restriction than to impose
> > > > it later!)
> > > Are you sure this is necessary? I assumed you would just create a ro IA
> > > mapping to the page, irrespective of the ability of ggtt.
> > 
> > You are thinking of the CPU mmap? The GTT mmap offers a linear view of
> > the tiled object. It would be very wrong for us to bypass the PROT_READ
> > protection of a user page by accessing it via the GTT.
> No, I was thinking of gtt mmap. That requires both GTT and IA PTE mappings
> right? One to map the object into the GTT, and then a second to point the
> IA at the aperture. Why wouldn't marking the IA mapping RO protect the
> object if the GT cannot reach the GTT mapping (from user batches).

Hmm. I keep forgetting that we can get at the vma from mmap(), because
that's hidden away elsewhere and only see i915_gem_fault() on a daily
basis. Hmm, is legal to read a PROT_READ-only vma is PROT_WRITE is
requested, or are meant to return -EINVAL?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via a GGTT mmap

[Bloomfield, Jon]

> -----Original Message-----
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Sent: Thursday, June 14, 2018 8:22 AM
> To: Bloomfield, Jon <jon.bloomfield@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Matthew Auld
> <matthew.william.auld@gmail.com>
> Subject: RE: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via
> a GGTT mmap
> 
> Quoting Bloomfield, Jon (2018-06-14 16:06:40)
> > > -----Original Message-----
> > > From: Chris Wilson <chris@chris-wilson.co.uk>
> > > Sent: Thursday, June 14, 2018 8:00 AM
> > > To: Bloomfield, Jon <jon.bloomfield@intel.com>; intel-
> > > gfx@lists.freedesktop.org
> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Matthew Auld
> > > <matthew.william.auld@gmail.com>
> > > Subject: RE: [PATCH 3/5] drm/i915: Prevent writing into a read-only
> object via
> > > a GGTT mmap
> > >
> > > Quoting Bloomfield, Jon (2018-06-14 15:53:13)
> > > > > -----Original Message-----
> > > > > From: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Sent: Thursday, June 14, 2018 5:00 AM
> > > > > To: intel-gfx@lists.freedesktop.org
> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>; Bloomfield, Jon
> > > > > <jon.bloomfield@intel.com>; Joonas Lahtinen
> > > > > <joonas.lahtinen@linux.intel.com>; Matthew Auld
> > > > > <matthew.william.auld@gmail.com>
> > > > > Subject: [PATCH 3/5] drm/i915: Prevent writing into a read-only
> object via
> > > a
> > > > > GGTT mmap
> > > > >
> > > > > If the user has created a read-only object, they should not be allowed
> > > > > to circumvent the write protection by using a GGTT mmapping. Deny
> it.
> > > > >
> > > > > Also most machines do not support read-only GGTT PTEs, so again we
> > > have
> > > > > to reject attempted writes. Fortunately, this is known a priori, so we
> > > > > can at least reject in the call to create the mmap with backup in the
> > > > > fault handler. This is a little draconian as we could blatantly ignore
> > > > > the write protection on the pages, but it is far simply to keep the
> > > > > readonly object pure. (It is easier to lift a restriction than to impose
> > > > > it later!)
> > > > Are you sure this is necessary? I assumed you would just create a ro IA
> > > > mapping to the page, irrespective of the ability of ggtt.
> > >
> > > You are thinking of the CPU mmap? The GTT mmap offers a linear view of
> > > the tiled object. It would be very wrong for us to bypass the PROT_READ
> > > protection of a user page by accessing it via the GTT.
> > No, I was thinking of gtt mmap. That requires both GTT and IA PTE
> mappings
> > right? One to map the object into the GTT, and then a second to point the
> > IA at the aperture. Why wouldn't marking the IA mapping RO protect the
> > object if the GT cannot reach the GTT mapping (from user batches).
> 
> Hmm. I keep forgetting that we can get at the vma from mmap(), because
> that's hidden away elsewhere and only see i915_gem_fault() on a daily
> basis. Hmm, is legal to read a PROT_READ-only vma is PROT_WRITE is
> requested, or are meant to return -EINVAL?
> -Chris
That's a trickier question :-) My instinct in -EINVAL if the user specifies
PROT_WRITE, but allowed if they only PROT_READ, and ppgtt is enabled
(including aliased) so that userspace can't see the gtt mapping from the GT.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5] drm/i915: Prevent writing into a read-only object via a GGTT mmap

[Chris Wilson]

If the user has created a read-only object, they should not be allowed
to circumvent the write protection by using a GGTT mmapping. Deny it.

Also most machines do not support read-only GGTT PTEs, so again we have
to reject attempted writes. Fortunately, this is known a priori, so we
can at least reject in the call to create the mmap (with a sanity check
in the fault handler).

v2: Check the vma->vm_flags during mmap() to allow readonly access.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Matthew Auld <matthew.william.auld@gmail.com>
Cc: David Herrmann <dh.herrmann@gmail.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> #v1
Reviewed-by: Matthew Auld <matthew.william.auld@gmail.com> #v1
Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
---
 drivers/gpu/drm/drm_gem.c                         |  5 +++++
 drivers/gpu/drm/i915/i915_gem.c                   |  4 ++++
 drivers/gpu/drm/i915/i915_gem_gtt.c               | 12 +++++++-----
 drivers/gpu/drm/i915/i915_gem_object.h            | 13 ++++++++++++-
 drivers/gpu/drm/i915/intel_ringbuffer.c           |  2 +-
 drivers/gpu/drm/i915/selftests/i915_gem_context.c |  5 +++--
 include/drm/drm_vma_manager.h                     |  1 +
 7 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 4a16d7b26c89..230863813905 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1036,6 +1036,11 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
 		return -EACCES;
 	}
 
+	if (vma->vm_flags & VM_WRITE && node->readonly) {
+		drm_gem_object_put_unlocked(obj);
+		return -EINVAL;
+	}
+
 	ret = drm_gem_mmap_obj(obj, drm_vma_node_size(node) << PAGE_SHIFT,
 			       vma);
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8dd4d35655af..2bfb16e83af2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2009,6 +2009,10 @@ vm_fault_t i915_gem_fault(struct vm_fault *vmf)
 	unsigned int flags;
 	int ret;
 
+	/* Sanity check that we allow writing into this object */
+	if (i915_gem_object_is_readonly(obj) && write)
+		return VM_FAULT_SIGBUS;
+
 	/* We don't use vmf->pgoff since that has the fake offset */
 	page_offset = (vmf->address - area->vm_start) >> PAGE_SHIFT;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index bfe23e10a127..8591e7051f0d 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -206,7 +206,7 @@ static int ppgtt_bind_vma(struct i915_vma *vma,
 
 	/* Applicable to VLV, and gen8+ */
 	pte_flags = 0;
-	if (vma->obj->gt_ro)
+	if (i915_gem_object_is_readonly(vma->obj))
 		pte_flags |= PTE_READ_ONLY;
 
 	vma->vm->insert_entries(vma->vm, vma, cache_level, pte_flags);
@@ -2423,8 +2423,10 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 	const gen8_pte_t pte_encode = gen8_pte_encode(0, level, 0);
 	dma_addr_t addr;
 
-	/* The GTT does not support read-only mappings */
-	GEM_BUG_ON(flags & PTE_READ_ONLY);
+	/*
+	 * Note that we ignore PTE_READ_ONLY here. The caller must be careful
+	 * not to allow the user to override access to a read only page.
+	 */
 
 	gtt_entries = (gen8_pte_t __iomem *)ggtt->gsm;
 	gtt_entries += vma->node.start >> PAGE_SHIFT;
@@ -2663,7 +2665,7 @@ static int ggtt_bind_vma(struct i915_vma *vma,
 
 	/* Applicable to VLV (gen8+ do not support RO in the GGTT) */
 	pte_flags = 0;
-	if (obj->gt_ro)
+	if (i915_gem_object_is_readonly(obj))
 		pte_flags |= PTE_READ_ONLY;
 
 	intel_runtime_pm_get(i915);
@@ -2701,7 +2703,7 @@ static int aliasing_gtt_bind_vma(struct i915_vma *vma,
 
 	/* Currently applicable only to VLV */
 	pte_flags = 0;
-	if (vma->obj->gt_ro)
+	if (i915_gem_object_is_readonly(vma->obj))
 		pte_flags |= PTE_READ_ONLY;
 
 	if (flags & I915_VMA_LOCAL_BIND) {
diff --git a/drivers/gpu/drm/i915/i915_gem_object.h b/drivers/gpu/drm/i915/i915_gem_object.h
index 54f00b350779..fd703d768b70 100644
--- a/drivers/gpu/drm/i915/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/i915_gem_object.h
@@ -141,7 +141,6 @@ struct drm_i915_gem_object {
 	 * Is the object to be mapped as read-only to the GPU
 	 * Only honoured if hardware has relevant pte bit
 	 */
-	unsigned long gt_ro:1;
 	unsigned int cache_level:3;
 	unsigned int cache_coherent:2;
 #define I915_BO_CACHE_COHERENT_FOR_READ BIT(0)
@@ -367,6 +366,18 @@ static inline void i915_gem_object_unlock(struct drm_i915_gem_object *obj)
 	reservation_object_unlock(obj->resv);
 }
 
+static inline void
+i915_gem_object_set_readonly(struct drm_i915_gem_object *obj)
+{
+	obj->base.vma_node.readonly = true;
+}
+
+static inline bool
+i915_gem_object_is_readonly(const struct drm_i915_gem_object *obj)
+{
+	return obj->base.vma_node.readonly;
+}
+
 static inline bool
 i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj)
 {
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 8853a5e6d421..9d54c3c24b10 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1112,7 +1112,7 @@ intel_ring_create_vma(struct drm_i915_private *dev_priv, int size)
 	 * if supported by the platform's GGTT.
 	 */
 	if (vm->has_read_only)
-		obj->gt_ro = 1;
+		i915_gem_object_set_readonly(obj);
 
 	vma = i915_vma_instance(obj, vm, NULL);
 	if (IS_ERR(vma))
diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
index 5bae52068926..fea447b1a74f 100644
--- a/drivers/gpu/drm/i915/selftests/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
@@ -510,7 +510,8 @@ static int igt_ctx_readonly(void *arg)
 					goto out_unlock;
 				}
 
-				obj->gt_ro = prandom_u32_state(&prng);
+				if (prandom_u32_state(&prng) & 1)
+					i915_gem_object_set_readonly(obj);
 			}
 
 			intel_runtime_pm_get(i915);
@@ -539,7 +540,7 @@ static int igt_ctx_readonly(void *arg)
 		unsigned int rem =
 			min_t(unsigned int, ndwords - dw, max_dwords(obj));
 
-		if (obj->gt_ro)
+		if (i915_gem_object_is_readonly(obj))
 			err = ro_check(obj, rem);
 		else
 			err = cpu_check(obj, rem);
diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
index 8758df94e9a0..c7987daeaed0 100644
--- a/include/drm/drm_vma_manager.h
+++ b/include/drm/drm_vma_manager.h
@@ -41,6 +41,7 @@ struct drm_vma_offset_node {
 	rwlock_t vm_lock;
 	struct drm_mm_node vm_node;
 	struct rb_root vm_files;
+	bool readonly:1;
 };
 
 struct drm_vma_offset_manager {
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via a GGTT mmap

[Joonas Lahtinen]

Quoting Chris Wilson (2018-06-14 22:24:02)
> If the user has created a read-only object, they should not be allowed
> to circumvent the write protection by using a GGTT mmapping. Deny it.
> 
> Also most machines do not support read-only GGTT PTEs, so again we have
> to reject attempted writes. Fortunately, this is known a priori, so we
> can at least reject in the call to create the mmap (with a sanity check
> in the fault handler).
> 
> v2: Check the vma->vm_flags during mmap() to allow readonly access.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Matthew Auld <matthew.william.auld@gmail.com>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> #v1
> Reviewed-by: Matthew Auld <matthew.william.auld@gmail.com> #v1
> Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>

<SNIP>

> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1036,6 +1036,11 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>                 return -EACCES;
>         }
>  
> +       if (vma->vm_flags & VM_WRITE && node->readonly) {
> +               drm_gem_object_put_unlocked(obj);
> +               return -EINVAL;
> +       }
> +

Pretty sure you want to split this patch and Cc: dri-devel. With that,
both are:

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via a GGTT mmap

[Chris Wilson]

Quoting Joonas Lahtinen (2018-06-15 09:08:54)
> Quoting Chris Wilson (2018-06-14 22:24:02)
> > If the user has created a read-only object, they should not be allowed
> > to circumvent the write protection by using a GGTT mmapping. Deny it.
> > 
> > Also most machines do not support read-only GGTT PTEs, so again we have
> > to reject attempted writes. Fortunately, this is known a priori, so we
> > can at least reject in the call to create the mmap (with a sanity check
> > in the fault handler).
> > 
> > v2: Check the vma->vm_flags during mmap() to allow readonly access.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Matthew Auld <matthew.william.auld@gmail.com>
> > Cc: David Herrmann <dh.herrmann@gmail.com>
> > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> #v1
> > Reviewed-by: Matthew Auld <matthew.william.auld@gmail.com> #v1
> > Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
> 
> <SNIP>
> 
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -1036,6 +1036,11 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> >                 return -EACCES;
> >         }
> >  
> > +       if (vma->vm_flags & VM_WRITE && node->readonly) {
> > +               drm_gem_object_put_unlocked(obj);
> > +               return -EINVAL;
> > +       }
> > +
> 
> Pretty sure you want to split this patch and Cc: dri-devel. With that,
> both are:

I thought I cc'ed dri-devel on the previous sending. (At least I thought
about it.) Besides until ttm differentiates between read/write access,
it's a moot point.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx