[PATCHES][RFC][CFT] close_fd() misuses in amdgpu

[PATCHES][RFC][CFT] close_fd() misuses in amdgpu

[Al Viro]

	Use of close_fd() in cleanup on failure exits is
wrong; descriptor table is a shared data structure, and
as soon as you've inserted a file reference there it is
entirely possible for another thread to have move it
around, replace it or remove it.

	Fortunately, not many places are using it that
way; two of such are in amdgpu and AFAICS they are not
hard to get rid of.

	Patches are in followups; review and testing would
be very welcome - as it is, I've verified that they build,
but that's about it.

[PATCH 1/2][RFC] 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.

It's not hard to fix - turn drm_gem_prime_handle_to_fd()
into a wrapper for a new helper that would simply return the
dmabuf, without messing with descriptor table.

Then kfd_mem_export_dmabuf() would simply use that new helper
and leave the descriptor table alone.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 8975cf41a91a..793780bb819c 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>
@@ -812,18 +811,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;
 	}
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 03bd3c7bd0dc..622c51d3fe18 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,49 @@ 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))
+		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/2][RFC] amdgpu: fix a race in kfd_mem_export_dmabuf()

[Felix Kuehling]

On 2024-06-03 22:13, 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.
>
> It's not hard to fix - turn drm_gem_prime_handle_to_fd()
> into a wrapper for a new helper that would simply return the
> dmabuf, without messing with descriptor table.
>
> Then kfd_mem_export_dmabuf() would simply use that new helper
> and leave the descriptor table alone.
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

This patch looks good to me on the amdgpu side. For the DRM side I'm 
adding dri-devel.

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


> ---
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 8975cf41a91a..793780bb819c 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>
> @@ -812,18 +811,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;
>   	}
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 03bd3c7bd0dc..622c51d3fe18 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,49 @@ 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))
> +		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/2][RFC] amdgpu: fix a race in kfd_mem_export_dmabuf()

[Al Viro]

On Tue, Jun 04, 2024 at 02:08:30PM -0400, Felix Kuehling wrote:
> > +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))
> > +		return PTR_ERR(dmabuf);

That ought to be
	if (IS_ERR(dmabuf)) {
		put_unused_fd(fd);
		return PTR_ERR(dmabuf);
	}
or we'll leak an allocated descriptor.  Will fix and repost...

[PATCH v2 1/2][RFC] amdgpu: fix a race in kfd_mem_export_dmabuf()

[Al Viro]

[now without a descriptor leak; it really needs testing, though]

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.

It's not hard to fix - turn drm_gem_prime_handle_to_fd()
into a wrapper for a new helper that would simply return the
dmabuf, without messing with descriptor table.

Then kfd_mem_export_dmabuf() would simply use that new helper
and leave the descriptor table alone.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 8975cf41a91a..793780bb819c 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>
@@ -812,18 +811,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;
 	}
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);

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

[Christian König]

Am 04.06.24 um 20:08 schrieb Felix Kuehling:
>
> On 2024-06-03 22:13, 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.
>>
>> It's not hard to fix - turn drm_gem_prime_handle_to_fd()
>> into a wrapper for a new helper that would simply return the
>> dmabuf, without messing with descriptor table.
>>
>> Then kfd_mem_export_dmabuf() would simply use that new helper
>> and leave the descriptor table alone.
>>
>> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>
> This patch looks good to me on the amdgpu side. For the DRM side I'm 
> adding dri-devel.

Yeah that patch should probably be split up and the DRM changes 
discussed separately.

On the other hand skimming over it it seems reasonable to me.

Felix are you going to look into this or should I take a look and try to 
push it through drm-misc-next?

Thanks,
Christian.

>
> Acked-by: Felix Kuehling <felix.kuehling@amd.com>
>
>
>> ---
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> index 8975cf41a91a..793780bb819c 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>
>> @@ -812,18 +811,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;
>>       }
>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> index 03bd3c7bd0dc..622c51d3fe18 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,49 @@ 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))
>> +        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/2][RFC] amdgpu: fix a race in kfd_mem_export_dmabuf()

[Felix Kuehling]

On 2024-06-05 05:14, Christian König wrote:
> Am 04.06.24 um 20:08 schrieb Felix Kuehling:
>>
>> On 2024-06-03 22:13, 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.
>>>
>>> It's not hard to fix - turn drm_gem_prime_handle_to_fd()
>>> into a wrapper for a new helper that would simply return the
>>> dmabuf, without messing with descriptor table.
>>>
>>> Then kfd_mem_export_dmabuf() would simply use that new helper
>>> and leave the descriptor table alone.
>>>
>>> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>>
>> This patch looks good to me on the amdgpu side. For the DRM side I'm 
>> adding dri-devel.
>
> Yeah that patch should probably be split up and the DRM changes 
> discussed separately.
>
> On the other hand skimming over it it seems reasonable to me.
>
> Felix are you going to look into this or should I take a look and try 
> to push it through drm-misc-next?

It doesn't matter much to me, as long as we submit both changes together.

Thanks,
   Felix


>
> Thanks,
> Christian.
>
>>
>> Acked-by: Felix Kuehling <felix.kuehling@amd.com>
>>
>>
>>> ---
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> index 8975cf41a91a..793780bb819c 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>
>>> @@ -812,18 +811,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;
>>>       }
>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>>> index 03bd3c7bd0dc..622c51d3fe18 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,49 @@ 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))
>>> +        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);
>

[PATCH 2/2][RFC] 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.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index fdf171ad4a3c..3f129e1c0daa 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,24 @@ 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 +1909,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 +1923,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;
@@ -1950,7 +1976,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 {
@@ -2001,12 +2027,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;
@@ -2358,7 +2380,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;
@@ -2410,7 +2433,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 {
@@ -2427,6 +2450,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;
 
@@ -2440,6 +2464,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) {
@@ -2465,7 +2495,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;
@@ -2480,11 +2510,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;

Re: [PATCH 2/2][RFC] amdkfd CRIU fixes

[Felix Kuehling]

On 2024-06-03 22:14, Al Viro wrote:
> 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.
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Thank you for the patches and the explanation. One minor nit-pick 
inline. With that fixed, this patch is

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

I can apply this patch to amd-staging-drm-next, if you want. See one 
comment inline ...


> ---
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index fdf171ad4a3c..3f129e1c0daa 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,24 @@ 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)

checkpatch.pl would complain here without an empty line after the 
variable definition.

Regards,
   Felix


> +			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 +1909,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 +1923,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;
> @@ -1950,7 +1976,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 {
> @@ -2001,12 +2027,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;
> @@ -2358,7 +2380,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;
> @@ -2410,7 +2433,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 {
> @@ -2427,6 +2450,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;
>   
> @@ -2440,6 +2464,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) {
> @@ -2465,7 +2495,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;
> @@ -2480,11 +2510,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;

Re: [PATCH 2/2][RFC] amdkfd CRIU fixes

[Al Viro]

On Tue, Jun 04, 2024 at 02:16:00PM -0400, Felix Kuehling wrote:
> 
> On 2024-06-03 22:14, Al Viro wrote:
> > 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.
> > 
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> 
> Thank you for the patches and the explanation. One minor nit-pick inline.
> With that fixed, this patch is
> 
> Reviewed-by: Felix Kuehling <felix.kuehling@amd.com>
> 
> I can apply this patch to amd-staging-drm-next, if you want. See one comment
> inline ...

Fine by me; I think this stuff would be better off in the relevant trees -
it's not as if we realistically could unexport close_fd() this cycle anyway,
more's the pity...  So nothing I've got in my queue has that as a prereq and
it would definitely have better odds of getting tested in your tree.