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

[Christian König <christian.koenig@xxxxxxx>]

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@xxxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxx>
>
>
> > ---
> > 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);