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

[Felix Kuehling <felix.kuehling@xxxxxxx>]

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@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?

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