Re: [PATCH] drm/syncobj: reset file ptr to NULL when released.

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Dec 19, 2017 at 09:30:12PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> 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
> 
> This patch fixes it, but I'm guessing it's racy and someone will
> smell rcu, but I just wanted to send out the proof of fixing it
> so I remember.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_syncobj.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index f776fc1cc543..ffa5bbd75852 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -361,6 +361,7 @@ static int drm_syncobj_file_release(struct inode *inode, struct file *file)
>  {
>  	struct drm_syncobj *syncobj = file->private_data;
>  
> +	syncobj->file = NULL;

Yeah that won't work :-)

The problem is that you have a pointer to the file without holding a
reference. But if you'd hold a full reference then there's a loop and it
never frees.

We need the same trick as for dma_buf and gem objects where we track the
number of idr entries in ->handle_count and drop the ->file cache when
that reaches 0.

Or you just allocate a new file every time userspace asks for one. The
only reason we cache them for dma-buf is to allow userspace to figure out
uniqueness of reimported stuff (since some drivers reject execbuf if you
give them the underlying object twice). This option is definitely less
typing if you can get away with it :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel