All of lore.kernel.org
 help / color / mirror / Atom feed
From: Chris Wilson <chris@chris-wilson.co.uk>
To: dri-devel@lists.freedesktop.org
Cc: intel-gfx@lists.freedesktop.org,
	"Chris Wilson" <chris@chris-wilson.co.uk>,
	"Lepton Wu" <ytht.net@gmail.com>,
	"Daniel Vetter" <daniel@ffwll.ch>,
	"Christian König" <christian.koenig@amd.com>,
	"Thomas Hellström" <thomas_os@shipmail.org>,
	stable@vger.kernel.org
Subject: [PATCH 1/2] drm/vgem: Do not allocate backing shmemfs file for an import dmabuf object
Date: Tue,  7 Jul 2020 17:00:11 +0100	[thread overview]
Message-ID: <20200707160012.1299338-1-chris@chris-wilson.co.uk> (raw)

If we assign obj->filp, we believe that the create vgem bo is native and
allow direct operations like mmap() assuming it behaves as backed by a
shmemfs inode. When imported from a dmabuf, the obj->pages are
not always meaningful and the shmemfs backing store misleading.

Note, that regular mmap access to a vgem bo is via the dumb buffer API,
and that rejects attempts to mmap an imported dmabuf,

drm_gem_dumb_map_offset():
        if (obj->import_attach) return -EINVAL;

So the only route by which we might accidentally allow mmapping of an
imported buffer is via vgem_prime_mmap(), which checked for
obj->filp assuming that it would be NULL.

Well it would had it been updated to use the common
drm_gem_dum_map_offset() helper, instead it has

vgem_gem_dumb_map():
	if (!obj->filp) return -EINVAL;

falling foul of the same trap as above.

Reported-by: Lepton Wu <ytht.net@gmail.com>
Fixes: af33a9190d02 ("drm/vgem: Enable dmabuf import interfaces")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Lepton Wu <ytht.net@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Christian König <christian.koenig@amd.com>
Cc: Thomas Hellström (Intel) <thomas_os@shipmail.org>
Cc: <stable@vger.kernel.org> # v4.13+
---
 drivers/gpu/drm/vgem/vgem_drv.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index 909eba43664a..eb3b7cdac941 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -91,7 +91,7 @@ static vm_fault_t vgem_gem_fault(struct vm_fault *vmf)
 		ret = 0;
 	}
 	mutex_unlock(&obj->pages_lock);
-	if (ret) {
+	if (ret && obj->base.filp) {
 		struct page *page;
 
 		page = shmem_read_mapping_page(
@@ -157,7 +157,8 @@ static void vgem_postclose(struct drm_device *dev, struct drm_file *file)
 }
 
 static struct drm_vgem_gem_object *__vgem_gem_create(struct drm_device *dev,
-						unsigned long size)
+						     struct file *shmem,
+						     unsigned long size)
 {
 	struct drm_vgem_gem_object *obj;
 	int ret;
@@ -166,11 +167,8 @@ static struct drm_vgem_gem_object *__vgem_gem_create(struct drm_device *dev,
 	if (!obj)
 		return ERR_PTR(-ENOMEM);
 
-	ret = drm_gem_object_init(dev, &obj->base, roundup(size, PAGE_SIZE));
-	if (ret) {
-		kfree(obj);
-		return ERR_PTR(ret);
-	}
+	drm_gem_private_object_init(dev, &obj->base, size);
+	obj->base.filp = shmem;
 
 	mutex_init(&obj->pages_lock);
 
@@ -189,11 +187,20 @@ static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
 					      unsigned long size)
 {
 	struct drm_vgem_gem_object *obj;
+	struct file *shmem;
 	int ret;
 
-	obj = __vgem_gem_create(dev, size);
-	if (IS_ERR(obj))
+	size = roundup(size, PAGE_SIZE);
+
+	shmem = shmem_file_setup(DRIVER_NAME, size, VM_NORESERVE);
+	if (IS_ERR(shmem))
+		return ERR_CAST(shmem);
+
+	obj = __vgem_gem_create(dev, shmem, size);
+	if (IS_ERR(obj)) {
+		fput(shmem);
 		return ERR_CAST(obj);
+	}
 
 	ret = drm_gem_handle_create(file, &obj->base, handle);
 	if (ret) {
@@ -363,7 +370,7 @@ static struct drm_gem_object *vgem_prime_import_sg_table(struct drm_device *dev,
 	struct drm_vgem_gem_object *obj;
 	int npages;
 
-	obj = __vgem_gem_create(dev, attach->dmabuf->size);
+	obj = __vgem_gem_create(dev, NULL, attach->dmabuf->size);
 	if (IS_ERR(obj))
 		return ERR_CAST(obj);
 
-- 
2.27.0


WARNING: multiple messages have this Message-ID (diff)
From: Chris Wilson <chris@chris-wilson.co.uk>
To: dri-devel@lists.freedesktop.org
Cc: "Chris Wilson" <chris@chris-wilson.co.uk>,
	"Thomas Hellström" <thomas_os@shipmail.org>,
	stable@vger.kernel.org,
	"Christian König" <christian.koenig@amd.com>,
	"Lepton Wu" <ytht.net@gmail.com>,
	intel-gfx@lists.freedesktop.org
Subject: [PATCH 1/2] drm/vgem: Do not allocate backing shmemfs file for an import dmabuf object
Date: Tue,  7 Jul 2020 17:00:11 +0100	[thread overview]
Message-ID: <20200707160012.1299338-1-chris@chris-wilson.co.uk> (raw)

If we assign obj->filp, we believe that the create vgem bo is native and
allow direct operations like mmap() assuming it behaves as backed by a
shmemfs inode. When imported from a dmabuf, the obj->pages are
not always meaningful and the shmemfs backing store misleading.

Note, that regular mmap access to a vgem bo is via the dumb buffer API,
and that rejects attempts to mmap an imported dmabuf,

drm_gem_dumb_map_offset():
        if (obj->import_attach) return -EINVAL;

So the only route by which we might accidentally allow mmapping of an
imported buffer is via vgem_prime_mmap(), which checked for
obj->filp assuming that it would be NULL.

Well it would had it been updated to use the common
drm_gem_dum_map_offset() helper, instead it has

vgem_gem_dumb_map():
	if (!obj->filp) return -EINVAL;

falling foul of the same trap as above.

Reported-by: Lepton Wu <ytht.net@gmail.com>
Fixes: af33a9190d02 ("drm/vgem: Enable dmabuf import interfaces")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Lepton Wu <ytht.net@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Christian König <christian.koenig@amd.com>
Cc: Thomas Hellström (Intel) <thomas_os@shipmail.org>
Cc: <stable@vger.kernel.org> # v4.13+
---
 drivers/gpu/drm/vgem/vgem_drv.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index 909eba43664a..eb3b7cdac941 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -91,7 +91,7 @@ static vm_fault_t vgem_gem_fault(struct vm_fault *vmf)
 		ret = 0;
 	}
 	mutex_unlock(&obj->pages_lock);
-	if (ret) {
+	if (ret && obj->base.filp) {
 		struct page *page;
 
 		page = shmem_read_mapping_page(
@@ -157,7 +157,8 @@ static void vgem_postclose(struct drm_device *dev, struct drm_file *file)
 }
 
 static struct drm_vgem_gem_object *__vgem_gem_create(struct drm_device *dev,
-						unsigned long size)
+						     struct file *shmem,
+						     unsigned long size)
 {
 	struct drm_vgem_gem_object *obj;
 	int ret;
@@ -166,11 +167,8 @@ static struct drm_vgem_gem_object *__vgem_gem_create(struct drm_device *dev,
 	if (!obj)
 		return ERR_PTR(-ENOMEM);
 
-	ret = drm_gem_object_init(dev, &obj->base, roundup(size, PAGE_SIZE));
-	if (ret) {
-		kfree(obj);
-		return ERR_PTR(ret);
-	}
+	drm_gem_private_object_init(dev, &obj->base, size);
+	obj->base.filp = shmem;
 
 	mutex_init(&obj->pages_lock);
 
@@ -189,11 +187,20 @@ static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
 					      unsigned long size)
 {
 	struct drm_vgem_gem_object *obj;
+	struct file *shmem;
 	int ret;
 
-	obj = __vgem_gem_create(dev, size);
-	if (IS_ERR(obj))
+	size = roundup(size, PAGE_SIZE);
+
+	shmem = shmem_file_setup(DRIVER_NAME, size, VM_NORESERVE);
+	if (IS_ERR(shmem))
+		return ERR_CAST(shmem);
+
+	obj = __vgem_gem_create(dev, shmem, size);
+	if (IS_ERR(obj)) {
+		fput(shmem);
 		return ERR_CAST(obj);
+	}
 
 	ret = drm_gem_handle_create(file, &obj->base, handle);
 	if (ret) {
@@ -363,7 +370,7 @@ static struct drm_gem_object *vgem_prime_import_sg_table(struct drm_device *dev,
 	struct drm_vgem_gem_object *obj;
 	int npages;
 
-	obj = __vgem_gem_create(dev, attach->dmabuf->size);
+	obj = __vgem_gem_create(dev, NULL, attach->dmabuf->size);
 	if (IS_ERR(obj))
 		return ERR_CAST(obj);
 
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

WARNING: multiple messages have this Message-ID (diff)
From: Chris Wilson <chris@chris-wilson.co.uk>
To: dri-devel@lists.freedesktop.org
Cc: "Chris Wilson" <chris@chris-wilson.co.uk>,
	stable@vger.kernel.org,
	"Christian König" <christian.koenig@amd.com>,
	"Lepton Wu" <ytht.net@gmail.com>,
	intel-gfx@lists.freedesktop.org
Subject: [Intel-gfx] [PATCH 1/2] drm/vgem: Do not allocate backing shmemfs file for an import dmabuf object
Date: Tue,  7 Jul 2020 17:00:11 +0100	[thread overview]
Message-ID: <20200707160012.1299338-1-chris@chris-wilson.co.uk> (raw)

If we assign obj->filp, we believe that the create vgem bo is native and
allow direct operations like mmap() assuming it behaves as backed by a
shmemfs inode. When imported from a dmabuf, the obj->pages are
not always meaningful and the shmemfs backing store misleading.

Note, that regular mmap access to a vgem bo is via the dumb buffer API,
and that rejects attempts to mmap an imported dmabuf,

drm_gem_dumb_map_offset():
        if (obj->import_attach) return -EINVAL;

So the only route by which we might accidentally allow mmapping of an
imported buffer is via vgem_prime_mmap(), which checked for
obj->filp assuming that it would be NULL.

Well it would had it been updated to use the common
drm_gem_dum_map_offset() helper, instead it has

vgem_gem_dumb_map():
	if (!obj->filp) return -EINVAL;

falling foul of the same trap as above.

Reported-by: Lepton Wu <ytht.net@gmail.com>
Fixes: af33a9190d02 ("drm/vgem: Enable dmabuf import interfaces")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Lepton Wu <ytht.net@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Christian König <christian.koenig@amd.com>
Cc: Thomas Hellström (Intel) <thomas_os@shipmail.org>
Cc: <stable@vger.kernel.org> # v4.13+
---
 drivers/gpu/drm/vgem/vgem_drv.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index 909eba43664a..eb3b7cdac941 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -91,7 +91,7 @@ static vm_fault_t vgem_gem_fault(struct vm_fault *vmf)
 		ret = 0;
 	}
 	mutex_unlock(&obj->pages_lock);
-	if (ret) {
+	if (ret && obj->base.filp) {
 		struct page *page;
 
 		page = shmem_read_mapping_page(
@@ -157,7 +157,8 @@ static void vgem_postclose(struct drm_device *dev, struct drm_file *file)
 }
 
 static struct drm_vgem_gem_object *__vgem_gem_create(struct drm_device *dev,
-						unsigned long size)
+						     struct file *shmem,
+						     unsigned long size)
 {
 	struct drm_vgem_gem_object *obj;
 	int ret;
@@ -166,11 +167,8 @@ static struct drm_vgem_gem_object *__vgem_gem_create(struct drm_device *dev,
 	if (!obj)
 		return ERR_PTR(-ENOMEM);
 
-	ret = drm_gem_object_init(dev, &obj->base, roundup(size, PAGE_SIZE));
-	if (ret) {
-		kfree(obj);
-		return ERR_PTR(ret);
-	}
+	drm_gem_private_object_init(dev, &obj->base, size);
+	obj->base.filp = shmem;
 
 	mutex_init(&obj->pages_lock);
 
@@ -189,11 +187,20 @@ static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
 					      unsigned long size)
 {
 	struct drm_vgem_gem_object *obj;
+	struct file *shmem;
 	int ret;
 
-	obj = __vgem_gem_create(dev, size);
-	if (IS_ERR(obj))
+	size = roundup(size, PAGE_SIZE);
+
+	shmem = shmem_file_setup(DRIVER_NAME, size, VM_NORESERVE);
+	if (IS_ERR(shmem))
+		return ERR_CAST(shmem);
+
+	obj = __vgem_gem_create(dev, shmem, size);
+	if (IS_ERR(obj)) {
+		fput(shmem);
 		return ERR_CAST(obj);
+	}
 
 	ret = drm_gem_handle_create(file, &obj->base, handle);
 	if (ret) {
@@ -363,7 +370,7 @@ static struct drm_gem_object *vgem_prime_import_sg_table(struct drm_device *dev,
 	struct drm_vgem_gem_object *obj;
 	int npages;
 
-	obj = __vgem_gem_create(dev, attach->dmabuf->size);
+	obj = __vgem_gem_create(dev, NULL, attach->dmabuf->size);
 	if (IS_ERR(obj))
 		return ERR_CAST(obj);
 
-- 
2.27.0

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

             reply	other threads:[~2020-07-07 16:16 UTC|newest]

Thread overview: 56+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-07 16:00 Chris Wilson [this message]
2020-07-07 16:00 ` [Intel-gfx] [PATCH 1/2] drm/vgem: Do not allocate backing shmemfs file for an import dmabuf object Chris Wilson
2020-07-07 16:00 ` Chris Wilson
2020-07-07 16:00 ` [PATCH 2/2] drm/vgem: Replace opencoded version of drm_gem_dumb_map_offset() Chris Wilson
2020-07-07 16:00   ` [Intel-gfx] " Chris Wilson
2020-07-08  9:56   ` Daniel Vetter
2020-07-08  9:56     ` [Intel-gfx] " Daniel Vetter
2020-07-08 14:53     ` Chris Wilson
2020-07-08 14:53       ` [Intel-gfx] " Chris Wilson
2020-07-07 16:33 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [1/2] drm/vgem: Do not allocate backing shmemfs file for an import dmabuf object Patchwork
2020-07-07 17:05 ` [PATCH 1/2] " lepton
2020-07-07 17:05   ` [Intel-gfx] " lepton
2020-07-07 17:05   ` lepton
2020-07-07 17:20   ` Chris Wilson
2020-07-07 17:20     ` [Intel-gfx] " Chris Wilson
2020-07-07 18:17     ` lepton
2020-07-07 18:17       ` [Intel-gfx] " lepton
2020-07-07 18:17       ` lepton
2020-07-07 18:35       ` Chris Wilson
2020-07-07 18:35         ` [Intel-gfx] " Chris Wilson
2020-07-08  9:22         ` Christian König
2020-07-08  9:22           ` [Intel-gfx] " Christian König
2020-07-08  9:22           ` Christian König
2020-07-08  9:54           ` Daniel Vetter
2020-07-08  9:54             ` [Intel-gfx] " Daniel Vetter
2020-07-08  9:54             ` Daniel Vetter
2020-07-08 14:37             ` Christian König
2020-07-08 14:37               ` [Intel-gfx] " Christian König
2020-07-08 14:37               ` Christian König
2020-07-08 15:01               ` Daniel Vetter
2020-07-08 15:01                 ` [Intel-gfx] " Daniel Vetter
2020-07-08 15:01                 ` Daniel Vetter
2020-07-08 15:05                 ` Christian König
2020-07-08 15:05                   ` [Intel-gfx] " Christian König
2020-07-08 15:05                   ` Christian König
2020-07-08 16:11                   ` Daniel Vetter
2020-07-08 16:11                     ` [Intel-gfx] " Daniel Vetter
2020-07-08 16:11                     ` Daniel Vetter
2020-07-08 16:19                     ` Daniel Vetter
2020-07-08 16:19                       ` [Intel-gfx] " Daniel Vetter
2020-07-08 16:19                       ` Daniel Vetter
2020-07-09  8:48                       ` Christian König
2020-07-09  8:48                         ` [Intel-gfx] " Christian König
2020-07-09  8:48                         ` Christian König
2020-07-09 13:54                         ` Steven Price
2020-07-09 13:54                           ` [Intel-gfx] " Steven Price
2020-07-09 13:54                           ` Steven Price
2020-07-09 14:15                           ` Christian König
2020-07-09 14:15                             ` [Intel-gfx] " Christian König
2020-07-09 14:15                             ` Christian König
2020-07-09  8:49                     ` Christian König
2020-07-09  8:49                       ` [Intel-gfx] " Christian König
2020-07-09  8:49                       ` Christian König
2020-07-08  5:44 ` lepton
2020-07-08  5:44   ` [Intel-gfx] " lepton
2020-07-08  5:44   ` lepton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200707160012.1299338-1-chris@chris-wilson.co.uk \
    --to=chris@chris-wilson.co.uk \
    --cc=christian.koenig@amd.com \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=stable@vger.kernel.org \
    --cc=thomas_os@shipmail.org \
    --cc=ytht.net@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.