[Intel-gfx] [PATCH v2] drm/syncobj: Stop reusing the same struct file for all syncobj -> fd

[Dave Airlie]

On 21 December 2017 at 19:28, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> The vk cts test:
> dEQP-VK.api.external.semaphore.opaque_fd.export_multiple_times_temporary
>
> triggers a lot of
> VFS: Close: file count is 0
>
> Dave pointed out that clearing the syncobj->file from
> drm_syncobj_file_release() was sufficient to silence the test, but that
> opens a can of worm since we assumed that the syncobj->file was never
> unset. So stop trying to reuse the same struct file for every fd pointing
> to the drm_syncobj, and allocate one file for each fd instead.
>
> v2: Fixup return handling of drm_syncobj_fd_to_handle
>
> Reported-by: Dave Airlie <airlied at redhat.com>
> Testcase: igt/syncobj_base/test-create-close-twice
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Dave Airlie <airlied at redhat.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
>  drivers/gpu/drm/drm_syncobj.c | 78 ++++++++++++++++---------------------------
>  include/drm/drm_syncobj.h     |  4 ---
>  2 files changed, 29 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 131695915acd..c235046c9ac2 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -399,23 +399,6 @@ static const struct file_operations drm_syncobj_file_fops = {
>         .release = drm_syncobj_file_release,
>  };
>
> -static int drm_syncobj_alloc_file(struct drm_syncobj *syncobj)
> -{
> -       struct file *file = anon_inode_getfile("syncobj_file",
> -                                              &drm_syncobj_file_fops,
> -                                              syncobj, 0);
> -       if (IS_ERR(file))
> -               return PTR_ERR(file);
> -
> -       drm_syncobj_get(syncobj);
> -       if (cmpxchg(&syncobj->file, NULL, file)) {
> -               /* lost the race */
> -               fput(file);
> -       }
> -
> -       return 0;
> -}
> -
>  /**
>   * drm_syncobj_get_fd - get a file descriptor from a syncobj
>   * @syncobj: Sync object to export
> @@ -427,21 +410,24 @@ static int drm_syncobj_alloc_file(struct drm_syncobj *syncobj)
>   */
>  int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd)
>  {
> -       int ret;
> +       struct file *file;
>         int fd;
>
>         fd = get_unused_fd_flags(O_CLOEXEC);
>         if (fd < 0)
>                 return fd;
>
> -       if (!syncobj->file) {
> -               ret = drm_syncobj_alloc_file(syncobj);
> -               if (ret) {
> -                       put_unused_fd(fd);
> -                       return ret;
> -               }
> +       file = anon_inode_getfile("syncobj_file",
> +                                 &drm_syncobj_file_fops,
> +                                 syncobj, 0);
> +       if (IS_ERR(file)) {
> +               put_unused_fd(fd);
> +               return PTR_ERR(file);
>         }
> -       fd_install(fd, syncobj->file);
> +
> +       drm_syncobj_get(syncobj);
> +       fd_install(fd, file);
> +
>         *p_fd = fd;
>         return 0;
>  }
> @@ -461,32 +447,22 @@ static int drm_syncobj_handle_to_fd(struct drm_file *file_private,
>         return ret;
>  }
>
> -static struct drm_syncobj *drm_syncobj_fdget(int fd)
> -{
> -       struct file *file = fget(fd);
> -
> -       if (!file)
> -               return NULL;
> -       if (file->f_op != &drm_syncobj_file_fops)
> -               goto err;
> -
> -       return file->private_data;
> -err:
> -       fput(file);
> -       return NULL;
> -};
> -
>  static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
>                                     int fd, u32 *handle)
>  {
> -       struct drm_syncobj *syncobj = drm_syncobj_fdget(fd);
> +       struct drm_syncobj *syncobj;
> +       struct file *file;
>         int ret;
>
> -       if (!syncobj)
> +       file = fget(fd);
> +       if (!file)
>                 return -EINVAL;
>
> -       /* take a reference to put in the idr */
> -       drm_syncobj_get(syncobj);
> +       if (file->f_op != &drm_syncobj_file_fops) {
> +               ret = -EINVAL;
> +               goto out_file;
> +       }
> +       syncobj = file->private_data;
>
>         idr_preload(GFP_KERNEL);
>         spin_lock(&file_private->syncobj_table_lock);
> @@ -494,12 +470,16 @@ static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
>         spin_unlock(&file_private->syncobj_table_lock);
>         idr_preload_end();
>
> -       if (ret < 0) {
> -               fput(syncobj->file);
> -               return ret;
> +       if (ret > 0) {
> +               /* take a reference for the idr */
> +               drm_syncobj_get(syncobj);
> +               *handle = ret;
> +               ret = 0;

I can't see getting the refcount after adding it to the idr is safe here.

Entering this function we could have just the fd reference to the syncobj,
We add it to the dir, and someone could call syncobj_destroy on the
handle randomly (i.e. without us having it), then we'd free syncobj.

I'm going to push this patch with the get in there, but with a put
in the error path I think.

Dave.