[PATCH] drm/drm_bridge: adjust bridge module's refcount

[Laurent Pinchart]

Hi Daniel,

On Wednesday 30 Nov 2016 10:12:01 Daniel Vetter wrote:
> On Wed, Nov 30, 2016 at 10:25:57AM +0200, Laurent Pinchart wrote:
> > On Wednesday 30 Nov 2016 09:13:00 Daniel Vetter wrote:
> >> On Wed, Nov 30, 2016 at 12:06:25AM +0200, Laurent Pinchart wrote:
> >>> On Tuesday 29 Nov 2016 23:12:56 Jyri Sarha wrote:
> >>>> Store the module that provides the bridge and adjust its refcount
> >>>> accordingly. The bridge module unload should not be allowed while the
> >>>> bridge is attached.
> >>>> 
> >>>> Signed-off-by: Jyri Sarha <jsarha at ti.com>
> >>>> ---
> >>>> 
> >>>>  drivers/gpu/drm/drm_bridge.c | 15 ++++++++++++---
> >>>>  include/drm/drm_bridge.h     | 11 ++++++++++-
> >>>>  2 files changed, 22 insertions(+), 4 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/gpu/drm/drm_bridge.c
> >>>> b/drivers/gpu/drm/drm_bridge.c
> >>>> index 0ee052b..36d427b 100644
> >>>> --- a/drivers/gpu/drm/drm_bridge.c
> >>>> +++ b/drivers/gpu/drm/drm_bridge.c
> > 
> > [snip]
> > 
> >>>> @@ -114,6 +118,9 @@ int drm_bridge_attach(struct drm_device *dev,
> >>>> struct drm_bridge *bridge)
> >>>>  	if (bridge->dev)
> >>>>  		return -EBUSY;
> >>>> 
> >>>> +	if (!try_module_get(bridge->module))
> >>>> +		return -ENOENT;
> >>> 
> >>> Isn't this still racy ? What happens if the module is removed right
> >>> before this call ? Won't the bridge object be freed, and this code then
> >>> try to call try_module_get() on freed memory ?
> >>> 
> >>> To fix this properly I think we need to make the bridge object
> >>> refcounted, with a release callback to signal to the bridge driver that
> >>> memory can be freed. The refcount should be increased in
> >>> of_drm_find_bridge(), and decreased in a new drm_bridge_put() function
> >>> (the "fun" part will be to update drivers to call that :-S).
> >>> 
> >>> The module refcount still needs to be increased in drm_bridge_attach()
> >>> like you do here, but you'll need to protect it with bridge_lock to
> >>> avoid a race between try_module_get() and drm_bridge_remove().
> >> 
> >> Hm right, I thought _attach is the function called directly, but we do
> >> lookup+attach. Might be good to have an of helper which does the
> >> lookup+attach and then drops the reference. We can do that as long as
> >> attach/detach holds onto a reference like the one below.
> >> 
> >> And I don't think we need a new bridge refcount, we can just us
> >> try_module_get/put for that (but wrapped into helpers ofc).
> > 
> > The bridge refcount and the module refcount serve two different purposes.
> > The first one addresses the unbind race by preventing the object from
> > being freed while still referenced, to avoid faulty data memory accesses
> > (note that increasing a module refcount doesn't prevent unbinding the
> > module from the device). The second one addresses the module unload race
> > by preventing code from being unloaded while still being reachable
> > through function pointers, avoiding faulty text memory accesses (as well
> > as data memory accesses to any .data or .rodata section in the module, if
> > those are referenced through pointers in the bridge object).
> > 
> > We thus need both types of refcounting, with the former tied to the lookup
> > operation and the latter to the attach operation (even though we could
> > handle module refcounting at lookup time as well if preferred, but in a
> > well-behaved system the bridge callbacks should not be invoked before
> > attach time).
>
> So you're saying struct drm_bridge should embed a struct device? Or at
> least kobj? Handling that race is one of the reasons we have them ...

I'm thinking about kref + a release callback. The really fun part to solve 
this in the long term will be to teach the DRM core about encoder hot-unplug 
:-) In the meantime the minimum we need is to allow the system to fail safely 
when a bridge is unplugged, which will involve safe recovery from 
.atomic_commit() failures.

-- 
Regards,

Laurent Pinchart