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

Felix Kuehling felix.kuehling at amd.com
Tue Jun 4 18:08:30 UTC 2024


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 at 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 at 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);


More information about the amd-gfx mailing list