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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 17 Feb 2023 at 13:06, Christian König
<ckoenig.leichtzumerken@xxxxxxxxx> wrote:
>
> 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@xxxxxxx>
> >>> Fixes: c76f0f7cb546 ("drm: Begin an API for in-kernel clients")
> >>> Cc: <stable@xxxxxxxxxxxxxxx>
> >> 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.

Well the device/component still needs to eventually hold a reference
on the module, or bad things can happen. But yeah dma-buf also holding
one but not a device/component reference is definitely bad.
-Daniel

>
> Christian.
>
> > -Daniel
> >
> >> Anyway, take my r-b, t-b tags.
> >>
> >> Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> >> Tested-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> >>
> >> 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
> >
> >
> >
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux