[PATCHES] [drm] file descriptor fixes

[PATCHES] [drm] file descriptor fixes

[Al Viro]

	Resurrecting the stuff from last cycle.
Context: several places in drm have racy uses of close_fd().
Not hard to fix, thankfully.

	Changed since the last posting: as requested, KFD
fix had been split in two commits - introduction of helper
(drm_gem_prime_handle_to_dmabuf()) and switching kfd_mem_export_dmabuf()
to that.

	Branch in git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #for-drm,
individual patches in followups.

	Please, review; IMO that ought to go through drm and amd-gfx
trees.

Shortlog:

Al Viro (4):
      new helper: drm_gem_prime_handle_to_dmabuf()
      amdgpu: fix a race in kfd_mem_export_dmabuf()
      amdkfd CRIU fixes
      amdgpu: get rid of bogus includes of fdtable.h

Diffstat:
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c    |  1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c   | 12 +---
 drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c          |  1 -
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c           | 64 ++++++++++++-----
 drivers/gpu/drm/drm_prime.c                        | 84 +++++++++++-----------
 include/drm/drm_prime.h                            |  3 +
 6 files changed, 95 insertions(+), 70 deletions(-)

[PATCH 1/4] new helper: drm_gem_prime_handle_to_dmabuf()

[Al Viro]

Once something had been put into descriptor table, the only thing you
can do with it is returning descriptor to userland - you can't withdraw
it on subsequent failure exit, etc.  You certainly can't count upon
it staying in the same slot of descriptor table - another thread
could've played with close(2)/dup2(2)/whatnot.

Add drm_gem_prime_handle_to_dmabuf() - the "set dmabuf up" parts of
drm_gem_prime_handle_to_fd() without the descriptor-related ones.
Instead of inserting into descriptor table and returning the file
descriptor it just returns the struct file.

drm_gem_prime_handle_to_fd() becomes a wrapper for it.  Other users
will be introduced in the next commit.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/gpu/drm/drm_prime.c | 84 +++++++++++++++++++------------------
 include/drm/drm_prime.h     |  3 ++
 2 files changed, 46 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 03bd3c7bd0dc..467c7a278ad3 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -409,23 +409,9 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
 	return dmabuf;
 }
 
-/**
- * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
- * @dev: dev to export the buffer from
- * @file_priv: drm file-private structure
- * @handle: buffer handle to export
- * @flags: flags like DRM_CLOEXEC
- * @prime_fd: pointer to storage for the fd id of the create dma-buf
- *
- * This is the PRIME export function which must be used mandatorily by GEM
- * drivers to ensure correct lifetime management of the underlying GEM object.
- * The actual exporting from GEM object to a dma-buf is done through the
- * &drm_gem_object_funcs.export callback.
- */
-int drm_gem_prime_handle_to_fd(struct drm_device *dev,
+struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
 			       struct drm_file *file_priv, uint32_t handle,
-			       uint32_t flags,
-			       int *prime_fd)
+			       uint32_t flags)
 {
 	struct drm_gem_object *obj;
 	int ret = 0;
@@ -434,14 +420,14 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 	mutex_lock(&file_priv->prime.lock);
 	obj = drm_gem_object_lookup(file_priv, handle);
 	if (!obj)  {
-		ret = -ENOENT;
+		dmabuf = ERR_PTR(-ENOENT);
 		goto out_unlock;
 	}
 
 	dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle);
 	if (dmabuf) {
 		get_dma_buf(dmabuf);
-		goto out_have_handle;
+		goto out;
 	}
 
 	mutex_lock(&dev->object_name_lock);
@@ -463,7 +449,6 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 		/* normally the created dma-buf takes ownership of the ref,
 		 * but if that fails then drop the ref
 		 */
-		ret = PTR_ERR(dmabuf);
 		mutex_unlock(&dev->object_name_lock);
 		goto out;
 	}
@@ -478,34 +463,51 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 	ret = drm_prime_add_buf_handle(&file_priv->prime,
 				       dmabuf, handle);
 	mutex_unlock(&dev->object_name_lock);
-	if (ret)
-		goto fail_put_dmabuf;
-
-out_have_handle:
-	ret = dma_buf_fd(dmabuf, flags);
-	/*
-	 * We must _not_ remove the buffer from the handle cache since the newly
-	 * created dma buf is already linked in the global obj->dma_buf pointer,
-	 * and that is invariant as long as a userspace gem handle exists.
-	 * Closing the handle will clean out the cache anyway, so we don't leak.
-	 */
-	if (ret < 0) {
-		goto fail_put_dmabuf;
-	} else {
-		*prime_fd = ret;
-		ret = 0;
+	if (ret) {
+		dma_buf_put(dmabuf);
+		dmabuf = ERR_PTR(ret);
 	}
-
-	goto out;
-
-fail_put_dmabuf:
-	dma_buf_put(dmabuf);
 out:
 	drm_gem_object_put(obj);
 out_unlock:
 	mutex_unlock(&file_priv->prime.lock);
+	return dmabuf;
+}
+EXPORT_SYMBOL(drm_gem_prime_handle_to_dmabuf);
 
-	return ret;
+/**
+ * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
+ * @dev: dev to export the buffer from
+ * @file_priv: drm file-private structure
+ * @handle: buffer handle to export
+ * @flags: flags like DRM_CLOEXEC
+ * @prime_fd: pointer to storage for the fd id of the create dma-buf
+ *
+ * This is the PRIME export function which must be used mandatorily by GEM
+ * drivers to ensure correct lifetime management of the underlying GEM object.
+ * The actual exporting from GEM object to a dma-buf is done through the
+ * &drm_gem_object_funcs.export callback.
+ */
+int drm_gem_prime_handle_to_fd(struct drm_device *dev,
+			       struct drm_file *file_priv, uint32_t handle,
+			       uint32_t flags,
+			       int *prime_fd)
+{
+	struct dma_buf *dmabuf;
+	int fd = get_unused_fd_flags(flags);
+
+	if (fd < 0)
+		return fd;
+
+	dmabuf = drm_gem_prime_handle_to_dmabuf(dev, file_priv, handle, flags);
+	if (IS_ERR(dmabuf)) {
+		put_unused_fd(fd);
+		return PTR_ERR(dmabuf);
+	}
+
+	fd_install(fd, dmabuf->file);
+	*prime_fd = fd;
+	return 0;
 }
 EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
 
diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
index 2a1d01e5b56b..fa085c44d4ca 100644
--- a/include/drm/drm_prime.h
+++ b/include/drm/drm_prime.h
@@ -69,6 +69,9 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf);
 
 int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 			       struct drm_file *file_priv, int prime_fd, uint32_t *handle);
+struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
+			       struct drm_file *file_priv, uint32_t handle,
+			       uint32_t flags);
 int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 			       struct drm_file *file_priv, uint32_t handle, uint32_t flags,
 			       int *prime_fd);
-- 
2.39.2

[PATCH 2/4] amdgpu: fix a race in kfd_mem_export_dmabuf()

[Al Viro]

Using drm_gem_prime_handle_to_fd() to set dmabuf up and insert it into
descriptor table, only to have it looked up by file descriptor and
remove it from descriptor table is not just too convoluted - it's
racy; another thread might have modified the descriptor table while
we'd been going through that song and dance.

Switch kfd_mem_export_dmabuf() to using drm_gem_prime_handle_to_dmabuf()
and leave the descriptor table alone...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 11672bfe4fad..bc5401de2948 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -25,7 +25,6 @@
 #include <linux/pagemap.h>
 #include <linux/sched/mm.h>
 #include <linux/sched/task.h>
-#include <linux/fdtable.h>
 #include <drm/ttm/ttm_tt.h>
 
 #include <drm/drm_exec.h>
@@ -818,18 +817,13 @@ static int kfd_mem_export_dmabuf(struct kgd_mem *mem)
 	if (!mem->dmabuf) {
 		struct amdgpu_device *bo_adev;
 		struct dma_buf *dmabuf;
-		int r, fd;
 
 		bo_adev = amdgpu_ttm_adev(mem->bo->tbo.bdev);
-		r = drm_gem_prime_handle_to_fd(&bo_adev->ddev, bo_adev->kfd.client.file,
+		dmabuf = drm_gem_prime_handle_to_dmabuf(&bo_adev->ddev, bo_adev->kfd.client.file,
 					       mem->gem_handle,
 			mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ?
-					       DRM_RDWR : 0, &fd);
-		if (r)
-			return r;
-		dmabuf = dma_buf_get(fd);
-		close_fd(fd);
-		if (WARN_ON_ONCE(IS_ERR(dmabuf)))
+					       DRM_RDWR : 0);
+		if (IS_ERR(dmabuf))
 			return PTR_ERR(dmabuf);
 		mem->dmabuf = dmabuf;
 	}
-- 
2.39.2

Re: [PATCH 2/4] amdgpu: fix a race in kfd_mem_export_dmabuf()

[Felix Kuehling]

On 2024-08-12 02:59, Al Viro wrote:
> Using drm_gem_prime_handle_to_fd() to set dmabuf up and insert it into
> descriptor table, only to have it looked up by file descriptor and
> remove it from descriptor table is not just too convoluted - it's
> racy; another thread might have modified the descriptor table while
> we'd been going through that song and dance.
>
> Switch kfd_mem_export_dmabuf() to using drm_gem_prime_handle_to_dmabuf()
> and leave the descriptor table alone...
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

This patch is

Reviewed-by: Felix Kuehling <felix.kuehling@amd.com>


> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 11672bfe4fad..bc5401de2948 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -25,7 +25,6 @@
>   #include <linux/pagemap.h>
>   #include <linux/sched/mm.h>
>   #include <linux/sched/task.h>
> -#include <linux/fdtable.h>
>   #include <drm/ttm/ttm_tt.h>
>   
>   #include <drm/drm_exec.h>
> @@ -818,18 +817,13 @@ static int kfd_mem_export_dmabuf(struct kgd_mem *mem)
>   	if (!mem->dmabuf) {
>   		struct amdgpu_device *bo_adev;
>   		struct dma_buf *dmabuf;
> -		int r, fd;
>   
>   		bo_adev = amdgpu_ttm_adev(mem->bo->tbo.bdev);
> -		r = drm_gem_prime_handle_to_fd(&bo_adev->ddev, bo_adev->kfd.client.file,
> +		dmabuf = drm_gem_prime_handle_to_dmabuf(&bo_adev->ddev, bo_adev->kfd.client.file,
>   					       mem->gem_handle,
>   			mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ?
> -					       DRM_RDWR : 0, &fd);
> -		if (r)
> -			return r;
> -		dmabuf = dma_buf_get(fd);
> -		close_fd(fd);
> -		if (WARN_ON_ONCE(IS_ERR(dmabuf)))
> +					       DRM_RDWR : 0);
> +		if (IS_ERR(dmabuf))
>   			return PTR_ERR(dmabuf);
>   		mem->dmabuf = dmabuf;
>   	}

Re: [PATCH 2/4] amdgpu: fix a race in kfd_mem_export_dmabuf()

[Al Viro]

On Wed, Aug 14, 2024 at 06:15:46PM -0400, Felix Kuehling wrote:
> 
> On 2024-08-12 02:59, Al Viro wrote:
> > Using drm_gem_prime_handle_to_fd() to set dmabuf up and insert it into
> > descriptor table, only to have it looked up by file descriptor and
> > remove it from descriptor table is not just too convoluted - it's
> > racy; another thread might have modified the descriptor table while
> > we'd been going through that song and dance.
> > 
> > Switch kfd_mem_export_dmabuf() to using drm_gem_prime_handle_to_dmabuf()
> > and leave the descriptor table alone...
> > 
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> 
> This patch is
> 
> Reviewed-by: Felix Kuehling <felix.kuehling@amd.com>

Umm...  So which tree should that series go through?

I can put it through vfs.git, or send a pull request to drm folks, or...

Preferences?

Re: [PATCH 2/4] amdgpu: fix a race in kfd_mem_export_dmabuf()

[Alex Deucher]

On Wed, Aug 21, 2024 at 8:29 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Wed, Aug 14, 2024 at 06:15:46PM -0400, Felix Kuehling wrote:
> >
> > On 2024-08-12 02:59, Al Viro wrote:
> > > Using drm_gem_prime_handle_to_fd() to set dmabuf up and insert it into
> > > descriptor table, only to have it looked up by file descriptor and
> > > remove it from descriptor table is not just too convoluted - it's
> > > racy; another thread might have modified the descriptor table while
> > > we'd been going through that song and dance.
> > >
> > > Switch kfd_mem_export_dmabuf() to using drm_gem_prime_handle_to_dmabuf()
> > > and leave the descriptor table alone...
> > >
> > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> >
> > This patch is
> >
> > Reviewed-by: Felix Kuehling <felix.kuehling@amd.com>
>
> Umm...  So which tree should that series go through?
>
> I can put it through vfs.git, or send a pull request to drm folks, or...
>
> Preferences?

I'm happy to take these via the amdgpu tree once the other patches get reviewed.

Alex

[PATCH 3/4] amdkfd CRIU fixes

[Al Viro]

Instead of trying to use close_fd() on failure exits, just have
criu_get_prime_handle() store the file reference without inserting
it into descriptor table.

Then, once the callers are past the last failure exit, they can go
and either insert all those file references into the corresponding
slots of descriptor table, or drop all those file references and
free the unused descriptors.

Reviewed-by: Felix Kuehling <felix.kuehling@amd.com>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 64 +++++++++++++++++-------
 1 file changed, 46 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 32e5db509560..23e5e3083367 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -36,7 +36,6 @@
 #include <linux/mman.h>
 #include <linux/ptrace.h>
 #include <linux/dma-buf.h>
-#include <linux/fdtable.h>
 #include <linux/processor.h>
 #include "kfd_priv.h"
 #include "kfd_device_queue_manager.h"
@@ -1857,7 +1856,8 @@ static uint32_t get_process_num_bos(struct kfd_process *p)
 }
 
 static int criu_get_prime_handle(struct kgd_mem *mem,
-				 int flags, u32 *shared_fd)
+				 int flags, u32 *shared_fd,
+				 struct file **file)
 {
 	struct dma_buf *dmabuf;
 	int ret;
@@ -1868,13 +1868,14 @@ static int criu_get_prime_handle(struct kgd_mem *mem,
 		return ret;
 	}
 
-	ret = dma_buf_fd(dmabuf, flags);
+	ret = get_unused_fd_flags(flags);
 	if (ret < 0) {
 		pr_err("dmabuf create fd failed, ret:%d\n", ret);
 		goto out_free_dmabuf;
 	}
 
 	*shared_fd = ret;
+	*file = dmabuf->file;
 	return 0;
 
 out_free_dmabuf:
@@ -1882,6 +1883,25 @@ static int criu_get_prime_handle(struct kgd_mem *mem,
 	return ret;
 }
 
+static void commit_files(struct file **files,
+			 struct kfd_criu_bo_bucket *bo_buckets,
+			 unsigned int count,
+			 int err)
+{
+	while (count--) {
+		struct file *file = files[count];
+
+		if (!file)
+			continue;
+		if (err) {
+			fput(file);
+			put_unused_fd(bo_buckets[count].dmabuf_fd);
+		} else {
+			fd_install(bo_buckets[count].dmabuf_fd, file);
+		}
+	}
+}
+
 static int criu_checkpoint_bos(struct kfd_process *p,
 			       uint32_t num_bos,
 			       uint8_t __user *user_bos,
@@ -1890,6 +1910,7 @@ static int criu_checkpoint_bos(struct kfd_process *p,
 {
 	struct kfd_criu_bo_bucket *bo_buckets;
 	struct kfd_criu_bo_priv_data *bo_privs;
+	struct file **files = NULL;
 	int ret = 0, pdd_index, bo_index = 0, id;
 	void *mem;
 
@@ -1903,6 +1924,12 @@ static int criu_checkpoint_bos(struct kfd_process *p,
 		goto exit;
 	}
 
+	files = kvzalloc(num_bos * sizeof(struct file *), GFP_KERNEL);
+	if (!files) {
+		ret = -ENOMEM;
+		goto exit;
+	}
+
 	for (pdd_index = 0; pdd_index < p->n_pdds; pdd_index++) {
 		struct kfd_process_device *pdd = p->pdds[pdd_index];
 		struct amdgpu_bo *dumper_bo;
@@ -1945,7 +1972,7 @@ static int criu_checkpoint_bos(struct kfd_process *p,
 				ret = criu_get_prime_handle(kgd_mem,
 						bo_bucket->alloc_flags &
 						KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ? DRM_RDWR : 0,
-						&bo_bucket->dmabuf_fd);
+						&bo_bucket->dmabuf_fd, &files[bo_index]);
 				if (ret)
 					goto exit;
 			} else {
@@ -1996,12 +2023,8 @@ static int criu_checkpoint_bos(struct kfd_process *p,
 	*priv_offset += num_bos * sizeof(*bo_privs);
 
 exit:
-	while (ret && bo_index--) {
-		if (bo_buckets[bo_index].alloc_flags
-		    & (KFD_IOC_ALLOC_MEM_FLAGS_VRAM | KFD_IOC_ALLOC_MEM_FLAGS_GTT))
-			close_fd(bo_buckets[bo_index].dmabuf_fd);
-	}
-
+	commit_files(files, bo_buckets, bo_index, ret);
+	kvfree(files);
 	kvfree(bo_buckets);
 	kvfree(bo_privs);
 	return ret;
@@ -2353,7 +2376,8 @@ static int criu_restore_memory_of_gpu(struct kfd_process_device *pdd,
 
 static int criu_restore_bo(struct kfd_process *p,
 			   struct kfd_criu_bo_bucket *bo_bucket,
-			   struct kfd_criu_bo_priv_data *bo_priv)
+			   struct kfd_criu_bo_priv_data *bo_priv,
+			   struct file **file)
 {
 	struct kfd_process_device *pdd;
 	struct kgd_mem *kgd_mem;
@@ -2405,7 +2429,7 @@ static int criu_restore_bo(struct kfd_process *p,
 	if (bo_bucket->alloc_flags
 	    & (KFD_IOC_ALLOC_MEM_FLAGS_VRAM | KFD_IOC_ALLOC_MEM_FLAGS_GTT)) {
 		ret = criu_get_prime_handle(kgd_mem, DRM_RDWR,
-					    &bo_bucket->dmabuf_fd);
+					    &bo_bucket->dmabuf_fd, file);
 		if (ret)
 			return ret;
 	} else {
@@ -2422,6 +2446,7 @@ static int criu_restore_bos(struct kfd_process *p,
 {
 	struct kfd_criu_bo_bucket *bo_buckets = NULL;
 	struct kfd_criu_bo_priv_data *bo_privs = NULL;
+	struct file **files = NULL;
 	int ret = 0;
 	uint32_t i = 0;
 
@@ -2435,6 +2460,12 @@ static int criu_restore_bos(struct kfd_process *p,
 	if (!bo_buckets)
 		return -ENOMEM;
 
+	files = kvzalloc(args->num_bos * sizeof(struct file *), GFP_KERNEL);
+	if (!files) {
+		ret = -ENOMEM;
+		goto exit;
+	}
+
 	ret = copy_from_user(bo_buckets, (void __user *)args->bos,
 			     args->num_bos * sizeof(*bo_buckets));
 	if (ret) {
@@ -2460,7 +2491,7 @@ static int criu_restore_bos(struct kfd_process *p,
 
 	/* Create and map new BOs */
 	for (; i < args->num_bos; i++) {
-		ret = criu_restore_bo(p, &bo_buckets[i], &bo_privs[i]);
+		ret = criu_restore_bo(p, &bo_buckets[i], &bo_privs[i], &files[i]);
 		if (ret) {
 			pr_debug("Failed to restore BO[%d] ret%d\n", i, ret);
 			goto exit;
@@ -2475,11 +2506,8 @@ static int criu_restore_bos(struct kfd_process *p,
 		ret = -EFAULT;
 
 exit:
-	while (ret && i--) {
-		if (bo_buckets[i].alloc_flags
-		   & (KFD_IOC_ALLOC_MEM_FLAGS_VRAM | KFD_IOC_ALLOC_MEM_FLAGS_GTT))
-			close_fd(bo_buckets[i].dmabuf_fd);
-	}
+	commit_files(files, bo_buckets, i, ret);
+	kvfree(files);
 	kvfree(bo_buckets);
 	kvfree(bo_privs);
 	return ret;
-- 
2.39.2

[PATCH 4/4] amdgpu: get rid of bogus includes of fdtable.h

[Al Viro]

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c | 1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c           | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
index 3a3f3ce09f00..2e2cfc5b5ed9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
@@ -20,7 +20,6 @@
  * OTHER DEALINGS IN THE SOFTWARE.
  */
 #include <linux/module.h>
-#include <linux/fdtable.h>
 #include <linux/uaccess.h>
 #include <linux/firmware.h>
 #include "amdgpu.h"
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
index 863b2a34b2d6..f9ff493c100e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
@@ -22,7 +22,6 @@
  * Authors: Andres Rodriguez <andresx7@gmail.com>
  */
 
-#include <linux/fdtable.h>
 #include <linux/file.h>
 #include <linux/pid.h>
 
-- 
2.39.2

Re: [PATCH 1/4] new helper: drm_gem_prime_handle_to_dmabuf()

[Thomas Zimmermann]

Hi

Am 12.08.24 um 08:59 schrieb Al Viro:
> Once something had been put into descriptor table, the only thing you
> can do with it is returning descriptor to userland - you can't withdraw
> it on subsequent failure exit, etc.  You certainly can't count upon
> it staying in the same slot of descriptor table - another thread
> could've played with close(2)/dup2(2)/whatnot.

This paragraph appears to refer to the newly added call to fd_install(). 
Maybe spell that out.

>
> Add drm_gem_prime_handle_to_dmabuf() - the "set dmabuf up" parts of
> drm_gem_prime_handle_to_fd() without the descriptor-related ones.
> Instead of inserting into descriptor table and returning the file
> descriptor it just returns the struct file.
>
> drm_gem_prime_handle_to_fd() becomes a wrapper for it.  Other users
> will be introduced in the next commit.
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>   drivers/gpu/drm/drm_prime.c | 84 +++++++++++++++++++------------------
>   include/drm/drm_prime.h     |  3 ++
>   2 files changed, 46 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 03bd3c7bd0dc..467c7a278ad3 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -409,23 +409,9 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
>   	return dmabuf;
>   }
>   
> -/**
> - * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
> - * @dev: dev to export the buffer from
> - * @file_priv: drm file-private structure
> - * @handle: buffer handle to export
> - * @flags: flags like DRM_CLOEXEC
> - * @prime_fd: pointer to storage for the fd id of the create dma-buf
> - *
> - * This is the PRIME export function which must be used mandatorily by GEM
> - * drivers to ensure correct lifetime management of the underlying GEM object.
> - * The actual exporting from GEM object to a dma-buf is done through the
> - * &drm_gem_object_funcs.export callback.
> - */
> -int drm_gem_prime_handle_to_fd(struct drm_device *dev,
> +struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,

If it's exported it should have kernel docs. At least copy-paste the 
docs from drm_gem_prime_handle_to_fd()
and reword a few bits.

Best regards
Thomas

>   			       struct drm_file *file_priv, uint32_t handle,
> -			       uint32_t flags,
> -			       int *prime_fd)
> +			       uint32_t flags)
>   {
>   	struct drm_gem_object *obj;
>   	int ret = 0;
> @@ -434,14 +420,14 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>   	mutex_lock(&file_priv->prime.lock);
>   	obj = drm_gem_object_lookup(file_priv, handle);
>   	if (!obj)  {
> -		ret = -ENOENT;
> +		dmabuf = ERR_PTR(-ENOENT);
>   		goto out_unlock;
>   	}
>   
>   	dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle);
>   	if (dmabuf) {
>   		get_dma_buf(dmabuf);
> -		goto out_have_handle;
> +		goto out;
>   	}
>   
>   	mutex_lock(&dev->object_name_lock);
> @@ -463,7 +449,6 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>   		/* normally the created dma-buf takes ownership of the ref,
>   		 * but if that fails then drop the ref
>   		 */
> -		ret = PTR_ERR(dmabuf);
>   		mutex_unlock(&dev->object_name_lock);
>   		goto out;
>   	}
> @@ -478,34 +463,51 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>   	ret = drm_prime_add_buf_handle(&file_priv->prime,
>   				       dmabuf, handle);
>   	mutex_unlock(&dev->object_name_lock);
> -	if (ret)
> -		goto fail_put_dmabuf;
> -
> -out_have_handle:
> -	ret = dma_buf_fd(dmabuf, flags);
> -	/*
> -	 * We must _not_ remove the buffer from the handle cache since the newly
> -	 * created dma buf is already linked in the global obj->dma_buf pointer,
> -	 * and that is invariant as long as a userspace gem handle exists.
> -	 * Closing the handle will clean out the cache anyway, so we don't leak.
> -	 */
> -	if (ret < 0) {
> -		goto fail_put_dmabuf;
> -	} else {
> -		*prime_fd = ret;
> -		ret = 0;
> +	if (ret) {
> +		dma_buf_put(dmabuf);
> +		dmabuf = ERR_PTR(ret);
>   	}
> -
> -	goto out;
> -
> -fail_put_dmabuf:
> -	dma_buf_put(dmabuf);
>   out:
>   	drm_gem_object_put(obj);
>   out_unlock:
>   	mutex_unlock(&file_priv->prime.lock);
> +	return dmabuf;
> +}
> +EXPORT_SYMBOL(drm_gem_prime_handle_to_dmabuf);
>   
> -	return ret;
> +/**
> + * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
> + * @dev: dev to export the buffer from
> + * @file_priv: drm file-private structure
> + * @handle: buffer handle to export
> + * @flags: flags like DRM_CLOEXEC
> + * @prime_fd: pointer to storage for the fd id of the create dma-buf
> + *
> + * This is the PRIME export function which must be used mandatorily by GEM
> + * drivers to ensure correct lifetime management of the underlying GEM object.
> + * The actual exporting from GEM object to a dma-buf is done through the
> + * &drm_gem_object_funcs.export callback.
> + */
> +int drm_gem_prime_handle_to_fd(struct drm_device *dev,
> +			       struct drm_file *file_priv, uint32_t handle,
> +			       uint32_t flags,
> +			       int *prime_fd)
> +{
> +	struct dma_buf *dmabuf;
> +	int fd = get_unused_fd_flags(flags);
> +
> +	if (fd < 0)
> +		return fd;
> +
> +	dmabuf = drm_gem_prime_handle_to_dmabuf(dev, file_priv, handle, flags);
> +	if (IS_ERR(dmabuf)) {
> +		put_unused_fd(fd);
> +		return PTR_ERR(dmabuf);
> +	}
> +
> +	fd_install(fd, dmabuf->file);
> +	*prime_fd = fd;
> +	return 0;
>   }
>   EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
>   
> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
> index 2a1d01e5b56b..fa085c44d4ca 100644
> --- a/include/drm/drm_prime.h
> +++ b/include/drm/drm_prime.h
> @@ -69,6 +69,9 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf);
>   
>   int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>   			       struct drm_file *file_priv, int prime_fd, uint32_t *handle);
> +struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
> +			       struct drm_file *file_priv, uint32_t handle,
> +			       uint32_t flags);
>   int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>   			       struct drm_file *file_priv, uint32_t handle, uint32_t flags,
>   			       int *prime_fd);

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH 1/4] new helper: drm_gem_prime_handle_to_dmabuf()

[Al Viro]

On Thu, Aug 22, 2024 at 04:41:59PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 12.08.24 um 08:59 schrieb Al Viro:
> > Once something had been put into descriptor table, the only thing you
> > can do with it is returning descriptor to userland - you can't withdraw
> > it on subsequent failure exit, etc.  You certainly can't count upon
> > it staying in the same slot of descriptor table - another thread
> > could've played with close(2)/dup2(2)/whatnot.
> 
> This paragraph appears to refer to the newly added call to fd_install().

It refers to dma_buf_fd() call that had been there all along, actually.
dma_buf_fd() is get_unused_fd_flags() + fd_install().  The reason
for splitting it in new variant and calling get_unused_fd_flags() and
fd_install() separately is that it makes for simpler cleanup; we could
use dma_buf_fd() instead - it would be a bit more clumsy, but that's
it.

The real issue is that drm_gem_prime_handle_to_fd() forces us to make
the thing reachable via descriptor table; it's just what we need when
all we are going to do is returning descriptor to userland, but it's
inherently racy for internal uses - anything put into descriptor table,
be it by fd_install() or by dma_buf_fd(), is fair game for all syscalls
by other threads.
 
> > Add drm_gem_prime_handle_to_dmabuf() - the "set dmabuf up" parts of
> > drm_gem_prime_handle_to_fd() without the descriptor-related ones.
> > Instead of inserting into descriptor table and returning the file
> > descriptor it just returns the struct file.
> > 
> > drm_gem_prime_handle_to_fd() becomes a wrapper for it.  Other users
> > will be introduced in the next commit.

> > -int drm_gem_prime_handle_to_fd(struct drm_device *dev,
> > +struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
> 
> If it's exported it should have kernel docs. At least copy-paste the docs
> from drm_gem_prime_handle_to_fd()
> and reword a few bits.

Point...

Re: [PATCH 1/4] new helper: drm_gem_prime_handle_to_dmabuf()

[Al Viro]

Do you have any problems with the variant below?  The only changes are
in commit message and added comment for new helper...

commit 8c291056e3e88153ef4b6316d5247547da200757
Author: Al Viro <viro@zeniv.linux.org.uk>
Date:   Fri Aug 2 09:56:28 2024 -0400

    new helper: drm_gem_prime_handle_to_dmabuf()
    
    Once something had been put into descriptor table, the only thing you
    can do with it is returning descriptor to userland - you can't withdraw
    it on subsequent failure exit, etc.  You certainly can't count upon
    it staying in the same slot of descriptor table - another thread
    could've played with close(2)/dup2(2)/whatnot.
    
    drm_gem_prime_handle_to_fd() creates a dmabuf, allocates a descriptor
    and attaches dmabuf's file to it (the last two steps are done
    in dma_buf_fd()).  That's nice when all you are going to do is
    passing a descriptor to userland.  If you just need to work with the
    resulting object or have something else to be done that might fail,
    drm_gem_prime_handle_to_fd() is racy.
    
    The problem is analogous to one with anon_inode_getfd(), and solution
    is similar to what anon_inode_getfile() provides.
    
    Add drm_gem_prime_handle_to_dmabuf() - the "set dmabuf up" parts of
    drm_gem_prime_handle_to_fd() without the descriptor-related ones.
    Instead of inserting into descriptor table and returning the file
    descriptor it just returns the struct file.
    
    drm_gem_prime_handle_to_fd() becomes a wrapper for it.  Other users
    will be introduced in the next commit.
    
    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 03bd3c7bd0dc..0e3f8adf162f 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -410,22 +410,30 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
 }
 
 /**
- * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
+ * drm_gem_prime_handle_to_dmabuf - PRIME export function for GEM drivers
  * @dev: dev to export the buffer from
  * @file_priv: drm file-private structure
  * @handle: buffer handle to export
  * @flags: flags like DRM_CLOEXEC
- * @prime_fd: pointer to storage for the fd id of the create dma-buf
  *
  * This is the PRIME export function which must be used mandatorily by GEM
  * drivers to ensure correct lifetime management of the underlying GEM object.
  * The actual exporting from GEM object to a dma-buf is done through the
  * &drm_gem_object_funcs.export callback.
+ *
+ * Unlike drm_gem_prime_handle_to_fd(), it returns the struct dma_buf it
+ * has created, without attaching it to any file descriptors.  The difference
+ * between those two is similar to that between anon_inode_getfile() and
+ * anon_inode_getfd(); insertion into descriptor table is something you
+ * can not revert if any cleanup is needed, so the descriptor-returning
+ * variants should only be used when you are past the last failure exit
+ * and the only thing left is passing the new file descriptor to userland.
+ * When all you need is the object itself or when you need to do something
+ * else that might fail, use that one instead.
  */
-int drm_gem_prime_handle_to_fd(struct drm_device *dev,
+struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
 			       struct drm_file *file_priv, uint32_t handle,
-			       uint32_t flags,
-			       int *prime_fd)
+			       uint32_t flags)
 {
 	struct drm_gem_object *obj;
 	int ret = 0;
@@ -434,14 +442,14 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 	mutex_lock(&file_priv->prime.lock);
 	obj = drm_gem_object_lookup(file_priv, handle);
 	if (!obj)  {
-		ret = -ENOENT;
+		dmabuf = ERR_PTR(-ENOENT);
 		goto out_unlock;
 	}
 
 	dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle);
 	if (dmabuf) {
 		get_dma_buf(dmabuf);
-		goto out_have_handle;
+		goto out;
 	}
 
 	mutex_lock(&dev->object_name_lock);
@@ -463,7 +471,6 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 		/* normally the created dma-buf takes ownership of the ref,
 		 * but if that fails then drop the ref
 		 */
-		ret = PTR_ERR(dmabuf);
 		mutex_unlock(&dev->object_name_lock);
 		goto out;
 	}
@@ -478,34 +485,51 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 	ret = drm_prime_add_buf_handle(&file_priv->prime,
 				       dmabuf, handle);
 	mutex_unlock(&dev->object_name_lock);
-	if (ret)
-		goto fail_put_dmabuf;
-
-out_have_handle:
-	ret = dma_buf_fd(dmabuf, flags);
-	/*
-	 * We must _not_ remove the buffer from the handle cache since the newly
-	 * created dma buf is already linked in the global obj->dma_buf pointer,
-	 * and that is invariant as long as a userspace gem handle exists.
-	 * Closing the handle will clean out the cache anyway, so we don't leak.
-	 */
-	if (ret < 0) {
-		goto fail_put_dmabuf;
-	} else {
-		*prime_fd = ret;
-		ret = 0;
+	if (ret) {
+		dma_buf_put(dmabuf);
+		dmabuf = ERR_PTR(ret);
 	}
-
-	goto out;
-
-fail_put_dmabuf:
-	dma_buf_put(dmabuf);
 out:
 	drm_gem_object_put(obj);
 out_unlock:
 	mutex_unlock(&file_priv->prime.lock);
+	return dmabuf;
+}
+EXPORT_SYMBOL(drm_gem_prime_handle_to_dmabuf);
 
-	return ret;
+/**
+ * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
+ * @dev: dev to export the buffer from
+ * @file_priv: drm file-private structure
+ * @handle: buffer handle to export
+ * @flags: flags like DRM_CLOEXEC
+ * @prime_fd: pointer to storage for the fd id of the create dma-buf
+ *
+ * This is the PRIME export function which must be used mandatorily by GEM
+ * drivers to ensure correct lifetime management of the underlying GEM object.
+ * The actual exporting from GEM object to a dma-buf is done through the
+ * &drm_gem_object_funcs.export callback.
+ */
+int drm_gem_prime_handle_to_fd(struct drm_device *dev,
+			       struct drm_file *file_priv, uint32_t handle,
+			       uint32_t flags,
+			       int *prime_fd)
+{
+	struct dma_buf *dmabuf;
+	int fd = get_unused_fd_flags(flags);
+
+	if (fd < 0)
+		return fd;
+
+	dmabuf = drm_gem_prime_handle_to_dmabuf(dev, file_priv, handle, flags);
+	if (IS_ERR(dmabuf)) {
+		put_unused_fd(fd);
+		return PTR_ERR(dmabuf);
+	}
+
+	fd_install(fd, dmabuf->file);
+	*prime_fd = fd;
+	return 0;
 }
 EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
 
diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
index 2a1d01e5b56b..fa085c44d4ca 100644
--- a/include/drm/drm_prime.h
+++ b/include/drm/drm_prime.h
@@ -69,6 +69,9 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf);
 
 int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 			       struct drm_file *file_priv, int prime_fd, uint32_t *handle);
+struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
+			       struct drm_file *file_priv, uint32_t handle,
+			       uint32_t flags);
 int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 			       struct drm_file *file_priv, uint32_t handle, uint32_t flags,
 			       int *prime_fd);

Re: [PATCH 1/4] new helper: drm_gem_prime_handle_to_dmabuf()

[Thomas Zimmermann]

Hi

Am 23.08.24 um 03:57 schrieb Al Viro:
> Do you have any problems with the variant below?  The only changes are
> in commit message and added comment for new helper...
>
> commit 8c291056e3e88153ef4b6316d5247547da200757
> Author: Al Viro <viro@zeniv.linux.org.uk>
> Date:   Fri Aug 2 09:56:28 2024 -0400
>
>      new helper: drm_gem_prime_handle_to_dmabuf()
>      
>      Once something had been put into descriptor table, the only thing you
>      can do with it is returning descriptor to userland - you can't withdraw
>      it on subsequent failure exit, etc.  You certainly can't count upon
>      it staying in the same slot of descriptor table - another thread
>      could've played with close(2)/dup2(2)/whatnot.
>      
>      drm_gem_prime_handle_to_fd() creates a dmabuf, allocates a descriptor
>      and attaches dmabuf's file to it (the last two steps are done
>      in dma_buf_fd()).  That's nice when all you are going to do is
>      passing a descriptor to userland.  If you just need to work with the
>      resulting object or have something else to be done that might fail,
>      drm_gem_prime_handle_to_fd() is racy.
>      
>      The problem is analogous to one with anon_inode_getfd(), and solution
>      is similar to what anon_inode_getfile() provides.
>      
>      Add drm_gem_prime_handle_to_dmabuf() - the "set dmabuf up" parts of
>      drm_gem_prime_handle_to_fd() without the descriptor-related ones.
>      Instead of inserting into descriptor table and returning the file
>      descriptor it just returns the struct file.
>      
>      drm_gem_prime_handle_to_fd() becomes a wrapper for it.  Other users
>      will be introduced in the next commit.
>      
>      Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

Thank you so much.

Best regards
Thomas

>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 03bd3c7bd0dc..0e3f8adf162f 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -410,22 +410,30 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
>   }
>   
>   /**
> - * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
> + * drm_gem_prime_handle_to_dmabuf - PRIME export function for GEM drivers
>    * @dev: dev to export the buffer from
>    * @file_priv: drm file-private structure
>    * @handle: buffer handle to export
>    * @flags: flags like DRM_CLOEXEC
> - * @prime_fd: pointer to storage for the fd id of the create dma-buf
>    *
>    * This is the PRIME export function which must be used mandatorily by GEM
>    * drivers to ensure correct lifetime management of the underlying GEM object.
>    * The actual exporting from GEM object to a dma-buf is done through the
>    * &drm_gem_object_funcs.export callback.
> + *
> + * Unlike drm_gem_prime_handle_to_fd(), it returns the struct dma_buf it
> + * has created, without attaching it to any file descriptors.  The difference
> + * between those two is similar to that between anon_inode_getfile() and
> + * anon_inode_getfd(); insertion into descriptor table is something you
> + * can not revert if any cleanup is needed, so the descriptor-returning
> + * variants should only be used when you are past the last failure exit
> + * and the only thing left is passing the new file descriptor to userland.
> + * When all you need is the object itself or when you need to do something
> + * else that might fail, use that one instead.
>    */
> -int drm_gem_prime_handle_to_fd(struct drm_device *dev,
> +struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
>   			       struct drm_file *file_priv, uint32_t handle,
> -			       uint32_t flags,
> -			       int *prime_fd)
> +			       uint32_t flags)
>   {
>   	struct drm_gem_object *obj;
>   	int ret = 0;
> @@ -434,14 +442,14 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>   	mutex_lock(&file_priv->prime.lock);
>   	obj = drm_gem_object_lookup(file_priv, handle);
>   	if (!obj)  {
> -		ret = -ENOENT;
> +		dmabuf = ERR_PTR(-ENOENT);
>   		goto out_unlock;
>   	}
>   
>   	dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle);
>   	if (dmabuf) {
>   		get_dma_buf(dmabuf);
> -		goto out_have_handle;
> +		goto out;
>   	}
>   
>   	mutex_lock(&dev->object_name_lock);
> @@ -463,7 +471,6 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>   		/* normally the created dma-buf takes ownership of the ref,
>   		 * but if that fails then drop the ref
>   		 */
> -		ret = PTR_ERR(dmabuf);
>   		mutex_unlock(&dev->object_name_lock);
>   		goto out;
>   	}
> @@ -478,34 +485,51 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>   	ret = drm_prime_add_buf_handle(&file_priv->prime,
>   				       dmabuf, handle);
>   	mutex_unlock(&dev->object_name_lock);
> -	if (ret)
> -		goto fail_put_dmabuf;
> -
> -out_have_handle:
> -	ret = dma_buf_fd(dmabuf, flags);
> -	/*
> -	 * We must _not_ remove the buffer from the handle cache since the newly
> -	 * created dma buf is already linked in the global obj->dma_buf pointer,
> -	 * and that is invariant as long as a userspace gem handle exists.
> -	 * Closing the handle will clean out the cache anyway, so we don't leak.
> -	 */
> -	if (ret < 0) {
> -		goto fail_put_dmabuf;
> -	} else {
> -		*prime_fd = ret;
> -		ret = 0;
> +	if (ret) {
> +		dma_buf_put(dmabuf);
> +		dmabuf = ERR_PTR(ret);
>   	}
> -
> -	goto out;
> -
> -fail_put_dmabuf:
> -	dma_buf_put(dmabuf);
>   out:
>   	drm_gem_object_put(obj);
>   out_unlock:
>   	mutex_unlock(&file_priv->prime.lock);
> +	return dmabuf;
> +}
> +EXPORT_SYMBOL(drm_gem_prime_handle_to_dmabuf);
>   
> -	return ret;
> +/**
> + * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
> + * @dev: dev to export the buffer from
> + * @file_priv: drm file-private structure
> + * @handle: buffer handle to export
> + * @flags: flags like DRM_CLOEXEC
> + * @prime_fd: pointer to storage for the fd id of the create dma-buf
> + *
> + * This is the PRIME export function which must be used mandatorily by GEM
> + * drivers to ensure correct lifetime management of the underlying GEM object.
> + * The actual exporting from GEM object to a dma-buf is done through the
> + * &drm_gem_object_funcs.export callback.
> + */
> +int drm_gem_prime_handle_to_fd(struct drm_device *dev,
> +			       struct drm_file *file_priv, uint32_t handle,
> +			       uint32_t flags,
> +			       int *prime_fd)
> +{
> +	struct dma_buf *dmabuf;
> +	int fd = get_unused_fd_flags(flags);
> +
> +	if (fd < 0)
> +		return fd;
> +
> +	dmabuf = drm_gem_prime_handle_to_dmabuf(dev, file_priv, handle, flags);
> +	if (IS_ERR(dmabuf)) {
> +		put_unused_fd(fd);
> +		return PTR_ERR(dmabuf);
> +	}
> +
> +	fd_install(fd, dmabuf->file);
> +	*prime_fd = fd;
> +	return 0;
>   }
>   EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
>   
> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
> index 2a1d01e5b56b..fa085c44d4ca 100644
> --- a/include/drm/drm_prime.h
> +++ b/include/drm/drm_prime.h
> @@ -69,6 +69,9 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf);
>   
>   int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>   			       struct drm_file *file_priv, int prime_fd, uint32_t *handle);
> +struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
> +			       struct drm_file *file_priv, uint32_t handle,
> +			       uint32_t flags);
>   int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>   			       struct drm_file *file_priv, uint32_t handle, uint32_t flags,
>   			       int *prime_fd);

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH 1/4] new helper: drm_gem_prime_handle_to_dmabuf()

[Al Viro]

On Fri, Aug 23, 2024 at 09:21:14AM +0200, Thomas Zimmermann wrote:

> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> Thank you so much.

OK, Acked-by added, branch force-pushed to
git://git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git #for-drm

In case if anybody wants a signed pull request, see below; or just
cherry-pick, etc. - entirely up to drm and amd folks...

The following changes since commit 8400291e289ee6b2bf9779ff1c83a291501f017b:

  Linux 6.11-rc1 (2024-07-28 14:19:55 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git tags/pull-for-drm

for you to fetch changes up to 30581926c42d1886cce2a04dcf615f551d829814:

  amdgpu: get rid of bogus includes of fdtable.h (2024-08-23 03:46:46 -0400)

----------------------------------------------------------------
get rid of racy manipulations of descriptor table in amdgpu

----------------------------------------------------------------
Al Viro (4):
      new helper: drm_gem_prime_handle_to_dmabuf()
      amdgpu: fix a race in kfd_mem_export_dmabuf()
      amdkfd CRIU fixes
      amdgpu: get rid of bogus includes of fdtable.h

 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c    |  1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c   | 12 +---
 drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c          |  1 -
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c           | 64 ++++++++++++-----
 drivers/gpu/drm/drm_prime.c                        | 84 ++++++++++++++--------
 include/drm/drm_prime.h                            |  3 +
 6 files changed, 106 insertions(+), 59 deletions(-)

Re: [PATCH 1/4] new helper: drm_gem_prime_handle_to_dmabuf()

[Alex Deucher]

Thanks.  I cherry-picked these to my tree.  Sorry for the delay.

Alex

On Fri, Aug 23, 2024 at 3:53 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Fri, Aug 23, 2024 at 09:21:14AM +0200, Thomas Zimmermann wrote:
>
> > Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> >
> > Thank you so much.
>
> OK, Acked-by added, branch force-pushed to
> git://git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git #for-drm
>
> In case if anybody wants a signed pull request, see below; or just
> cherry-pick, etc. - entirely up to drm and amd folks...
>
> The following changes since commit 8400291e289ee6b2bf9779ff1c83a291501f017b:
>
>   Linux 6.11-rc1 (2024-07-28 14:19:55 -0700)
>
> are available in the Git repository at:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git tags/pull-for-drm
>
> for you to fetch changes up to 30581926c42d1886cce2a04dcf615f551d829814:
>
>   amdgpu: get rid of bogus includes of fdtable.h (2024-08-23 03:46:46 -0400)
>
> ----------------------------------------------------------------
> get rid of racy manipulations of descriptor table in amdgpu
>
> ----------------------------------------------------------------
> Al Viro (4):
>       new helper: drm_gem_prime_handle_to_dmabuf()
>       amdgpu: fix a race in kfd_mem_export_dmabuf()
>       amdkfd CRIU fixes
>       amdgpu: get rid of bogus includes of fdtable.h
>
>  .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c    |  1 -
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c   | 12 +---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c          |  1 -
>  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c           | 64 ++++++++++++-----
>  drivers/gpu/drm/drm_prime.c                        | 84 ++++++++++++++--------
>  include/drm/drm_prime.h                            |  3 +
>  6 files changed, 106 insertions(+), 59 deletions(-)