[PATCH 1/2] drm/client: fix circular reference counting issue

[Christian König]

Am 16.02.23 um 15:34 schrieb Daniel Vetter:
> On Thu, Jan 26, 2023 at 03:30:31PM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 26.01.23 um 11:28 schrieb Christian König:
>>> We reference dump buffers both by their handle as well as their
>>> object. The problem is now that when anybody iterates over the DRM
>>> framebuffers and exports the underlying GEM objects through DMA-buf
>>> we run into a circular reference count situation.
>>>
>>> The result is that the fbdev handling holds the GEM handle preventing
>>> the DMA-buf in the GEM object to be released. This DMA-buf in turn
>>> holds a reference to the driver module which on unload would release
>>> the fbdev.
>>>
>>> Break that loop by releasing the handle as soon as the DRM
>>> framebuffer object is created. The DRM framebuffer and the DRM client
>>> buffer structure still hold a reference to the underlying GEM object
>>> preventing its destruction.
>>>
>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>> Fixes: c76f0f7cb546 ("drm: Begin an API for in-kernel clients")
>>> Cc: <stable at vger.kernel.org>
>> I tested with Weston and Gnome in X11 and Wayland mode under simpledrm,
>> which I started stopped from the console. No obvious problems.
>>
>> I heard that sway/wlroots has issues with drivers that don't support
>> dma-buf. Maybe(!) that could be affected by this patch.
> dma-buf export should still work. Also the loop is imo a red herring, I
> think if you force unbind the driver then this should all get resolved
> automatically.
>
> What is true is that once we start refcounting everything correctly then
> there will be elevated module refcounts, which means you cannot use module
> unloading to provoke a driver unbind, which would kick out all the
> leftover references. You instead need to manually unbind the driver first,
> which should drop all remaining references to zero (might need to kill
> also any userspace), and only then can you unload the driver.
>
> But this confusion is extremely common, a lot of people think that just
> holding a module reference is enough, we really should also hold a
> drm_device reference for dma-buf ...

Yeah, hot plug removal of amdgpu revealed a couple of those as well.

Essentially what DMA-buf does with grabbing a module reference on the 
owner of a DMA-buf is a bad idea.

Instead we should reference the device or component which is exporting 
the buffer, but since we don't have a common structure here it's more 
work to generalize that approach.

Christian.

> -Daniel
>
>> Anyway, take my r-b, t-b tags.
>>
>> Reviewed-by: Thomas Zimmermann <tzimmermann at suse.de>
>> Tested-by: Thomas Zimmermann <tzimmermann at suse.de>
>>
>> Thank you for fixing this bug.
>>
>> Best regards
>> Thomas
>>
>>> ---
>>>    drivers/gpu/drm/drm_client.c | 33 ++++++++++++++++++++-------------
>>>    include/drm/drm_client.h     |  5 -----
>>>    2 files changed, 20 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
>>> index 009e7b10455c..f6292ba0e6fc 100644
>>> --- a/drivers/gpu/drm/drm_client.c
>>> +++ b/drivers/gpu/drm/drm_client.c
>>> @@ -243,21 +243,17 @@ void drm_client_dev_restore(struct drm_device *dev)
>>>    static void drm_client_buffer_delete(struct drm_client_buffer *buffer)
>>>    {
>>> -	struct drm_device *dev = buffer->client->dev;
>>> -
>>>    	if (buffer->gem) {
>>>    		drm_gem_vunmap_unlocked(buffer->gem, &buffer->map);
>>>    		drm_gem_object_put(buffer->gem);
>>>    	}
>>> -	if (buffer->handle)
>>> -		drm_mode_destroy_dumb(dev, buffer->handle, buffer->client->file);
>>> -
>>>    	kfree(buffer);
>>>    }
>>>    static struct drm_client_buffer *
>>> -drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u32 format)
>>> +drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height,
>>> +			 u32 format, u32 *handle)
>>>    {
>>>    	const struct drm_format_info *info = drm_format_info(format);
>>>    	struct drm_mode_create_dumb dumb_args = { };
>>> @@ -279,16 +275,15 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u
>>>    	if (ret)
>>>    		goto err_delete;
>>> -	buffer->handle = dumb_args.handle;
>>> -	buffer->pitch = dumb_args.pitch;
>>> -
>>>    	obj = drm_gem_object_lookup(client->file, dumb_args.handle);
>>>    	if (!obj)  {
>>>    		ret = -ENOENT;
>>>    		goto err_delete;
>>>    	}
>>> +	buffer->pitch = dumb_args.pitch;
>>>    	buffer->gem = obj;
>>> +	*handle = dumb_args.handle;
>>>    	return buffer;
>>> @@ -375,7 +370,8 @@ static void drm_client_buffer_rmfb(struct drm_client_buffer *buffer)
>>>    }
>>>    static int drm_client_buffer_addfb(struct drm_client_buffer *buffer,
>>> -				   u32 width, u32 height, u32 format)
>>> +				   u32 width, u32 height, u32 format,
>>> +				   u32 handle)
>>>    {
>>>    	struct drm_client_dev *client = buffer->client;
>>>    	struct drm_mode_fb_cmd fb_req = { };
>>> @@ -387,7 +383,7 @@ static int drm_client_buffer_addfb(struct drm_client_buffer *buffer,
>>>    	fb_req.depth = info->depth;
>>>    	fb_req.width = width;
>>>    	fb_req.height = height;
>>> -	fb_req.handle = buffer->handle;
>>> +	fb_req.handle = handle;
>>>    	fb_req.pitch = buffer->pitch;
>>>    	ret = drm_mode_addfb(client->dev, &fb_req, client->file);
>>> @@ -424,13 +420,24 @@ struct drm_client_buffer *
>>>    drm_client_framebuffer_create(struct drm_client_dev *client, u32 width, u32 height, u32 format)
>>>    {
>>>    	struct drm_client_buffer *buffer;
>>> +	u32 handle;
>>>    	int ret;
>>> -	buffer = drm_client_buffer_create(client, width, height, format);
>>> +	buffer = drm_client_buffer_create(client, width, height, format,
>>> +					  &handle);
>>>    	if (IS_ERR(buffer))
>>>    		return buffer;
>>> -	ret = drm_client_buffer_addfb(buffer, width, height, format);
>>> +	ret = drm_client_buffer_addfb(buffer, width, height, format, handle);
>>> +
>>> +	/*
>>> +	 * The handle is only needed for creating the framebuffer, destroy it
>>> +	 * again to solve a circular dependency should anybody export the GEM
>>> +	 * object as DMA-buf. The framebuffer and our buffer structure are still
>>> +	 * holding references to the GEM object to prevent its destruction.
>>> +	 */
>>> +	drm_mode_destroy_dumb(client->dev, handle, client->file);
>>> +
>>>    	if (ret) {
>>>    		drm_client_buffer_delete(buffer);
>>>    		return ERR_PTR(ret);
>>> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
>>> index 39482527a775..b5acdab73766 100644
>>> --- a/include/drm/drm_client.h
>>> +++ b/include/drm/drm_client.h
>>> @@ -134,11 +134,6 @@ struct drm_client_buffer {
>>>    	 */
>>>    	struct drm_client_dev *client;
>>> -	/**
>>> -	 * @handle: Buffer handle
>>> -	 */
>>> -	u32 handle;
>>> -
>>>    	/**
>>>    	 * @pitch: Buffer pitch
>>>    	 */
>> -- 
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Ivo Totev
>
>
>