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
