| Message ID | 20230126102814.8722-1-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/client: fix circular reference counting issue | expand |
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@amd.com> > Fixes: c76f0f7cb546 ("drm: Begin an API for in-kernel clients") > Cc: <stable@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. Anyway, take my r-b, t-b tags. Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> Tested-by: Thomas Zimmermann <tzimmermann@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 > */
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@amd.com> > > Fixes: c76f0f7cb546 ("drm: Begin an API for in-kernel clients") > > Cc: <stable@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 ... -Daniel > > Anyway, take my r-b, t-b tags. > > Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > Tested-by: Thomas Zimmermann <tzimmermann@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
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@amd.com> >>> Fixes: c76f0f7cb546 ("drm: Begin an API for in-kernel clients") >>> Cc: <stable@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@suse.de> >> Tested-by: Thomas Zimmermann <tzimmermann@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 > > >
On Fri, 17 Feb 2023 at 13:06, Christian König <ckoenig.leichtzumerken@gmail.com> 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@amd.com> > >>> Fixes: c76f0f7cb546 ("drm: Begin an API for in-kernel clients") > >>> Cc: <stable@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. 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@suse.de> > >> Tested-by: Thomas Zimmermann <tzimmermann@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 > > > > > > >
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 */
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@amd.com> Fixes: c76f0f7cb546 ("drm: Begin an API for in-kernel clients") Cc: <stable@vger.kernel.org> --- drivers/gpu/drm/drm_client.c | 33 ++++++++++++++++++++------------- include/drm/drm_client.h | 5 ----- 2 files changed, 20 insertions(+), 18 deletions(-)