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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Daniel,

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@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).

> >>  	bridge->dev = dev;
> >>  	
> >>  	if (bridge->funcs->attach)

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel