[PATCH 1/2][RFC] amdgpu: fix a race in kfd_mem_export_dmabuf()
Felix Kuehling
felix.kuehling at amd.com
Thu Jun 6 21:57:29 UTC 2024
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 at 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 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