| Message ID | 20180711155632.37437-1-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
在 2018-07-11三的 17:56 +0200，Noralf Trønnes写道： > Commit 894a677f4b3e ("drm/cma-helper: Use the generic fbdev > emulation") > broke almost all drivers that use the CMA helper. > > The reason is that drm_client_new() requires that the DRM device has > been registered, but the drivers register fbdev before registering > DRM. > > Remove the requirement that DRM should be registered when creating a > new client. > > This creates a theoretical race condition where drm_client_new() > races > with drm_dev_unregister() resulting in the .unregister hook not being > called. > > Case: > User loads module which calls drm_client_new(). > Device is unplugged and drm_dev_unregister() is called. > > If drm_dev_unregister() gets the clientlist_mutex first, the new > client > will never get it's unregister hook called. > > Fixes: c76f0f7cb546 ("drm: Begin an API for in-kernel clients") > Cc: Maxime Ripard <maxime.ripard@bootlin.com> > Cc: Icenowy Zheng <icenowy@aosc.io> > Cc: Chen-Yu Tsai <wens@csie.org> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> Tested on sun4i-drm: ``` [ 1.361768] sun4i-drm display-engine: bound 1100000.mixer (ops 0xc0ccf608) [ 1.369008] sun4i-drm display-engine: bound 1c0c000.lcd-controller (ops 0xc0ccc8d8) [ 1.377644] sun8i-dw-hdmi 1ee0000.hdmi: Detected HDMI TX controller v1.32a with HDCP (sun8i_dw_hdmi_phy) [ 1.387637] sun8i-dw-hdmi 1ee0000.hdmi: registered DesignWare HDMI I2C bus driver [ 1.395527] sun4i-drm display-engine: bound 1ee0000.hdmi (ops 0xc0ccf08c) [ 1.402361] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). [ 1.408987] [drm] No driver support for vblank timestamp query. [ 1.714324] Console: switching to colour frame buffer device 128x48 [ 1.736736] sun4i-drm display-engine: fb0: DRM emulated frame buffer device [ 1.744252] [drm] Initialized sun4i-drm 1.0.0 20150629 for display- engine on minor 0 ``` Tested-by: Icenowy Zheng <icenowy@aosc.io> Thanks! Icenowy > --- > drivers/gpu/drm/drm_client.c | 11 +---------- > 1 file changed, 1 insertion(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/drm_client.c > b/drivers/gpu/drm/drm_client.c > index 4039a4d103a8..9b142f58d489 100644 > --- a/drivers/gpu/drm/drm_client.c > +++ b/drivers/gpu/drm/drm_client.c > @@ -78,7 +78,6 @@ EXPORT_SYMBOL(drm_client_close); > int drm_client_new(struct drm_device *dev, struct drm_client_dev > *client, > const char *name, const struct drm_client_funcs > *funcs) > { > - bool registered; > int ret; > > if (!drm_core_check_feature(dev, DRIVER_MODESET) || > @@ -97,21 +96,13 @@ int drm_client_new(struct drm_device *dev, struct > drm_client_dev *client, > goto err_put_module; > > mutex_lock(&dev->clientlist_mutex); > - registered = dev->registered; > - if (registered) > - list_add(&client->list, &dev->clientlist); > + list_add(&client->list, &dev->clientlist); > mutex_unlock(&dev->clientlist_mutex); > - if (!registered) { > - ret = -ENODEV; > - goto err_close; > - } > > drm_dev_get(dev); > > return 0; > > -err_close: > - drm_client_close(client); > err_put_module: > if (funcs) > module_put(funcs->owner);
On Wed, Jul 11, 2018 at 05:56:32PM +0200, Noralf Trønnes wrote: > Commit 894a677f4b3e ("drm/cma-helper: Use the generic fbdev emulation") > broke almost all drivers that use the CMA helper. > > The reason is that drm_client_new() requires that the DRM device has > been registered, but the drivers register fbdev before registering DRM. > > Remove the requirement that DRM should be registered when creating a > new client. > > This creates a theoretical race condition where drm_client_new() races > with drm_dev_unregister() resulting in the .unregister hook not being > called. > > Case: > User loads module which calls drm_client_new(). > Device is unplugged and drm_dev_unregister() is called. > > If drm_dev_unregister() gets the clientlist_mutex first, the new client > will never get it's unregister hook called. As long as we assume that drm_client_new() is serialized against drm_dev_unregister (which it has to, or all the current fbdev code would be busted) I'm not seeing a race here. We can't allow drm_client_new concurrently with drm_dev_register, but that feature got canned. So looks all good to me, and no races anywhere to be seen. Assuming you agree and update the commit message: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Fixes: c76f0f7cb546 ("drm: Begin an API for in-kernel clients") > Cc: Maxime Ripard <maxime.ripard@bootlin.com> > Cc: Icenowy Zheng <icenowy@aosc.io> > Cc: Chen-Yu Tsai <wens@csie.org> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > --- > drivers/gpu/drm/drm_client.c | 11 +---------- > 1 file changed, 1 insertion(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c > index 4039a4d103a8..9b142f58d489 100644 > --- a/drivers/gpu/drm/drm_client.c > +++ b/drivers/gpu/drm/drm_client.c > @@ -78,7 +78,6 @@ EXPORT_SYMBOL(drm_client_close); > int drm_client_new(struct drm_device *dev, struct drm_client_dev *client, > const char *name, const struct drm_client_funcs *funcs) > { > - bool registered; > int ret; > > if (!drm_core_check_feature(dev, DRIVER_MODESET) || > @@ -97,21 +96,13 @@ int drm_client_new(struct drm_device *dev, struct drm_client_dev *client, > goto err_put_module; > > mutex_lock(&dev->clientlist_mutex); > - registered = dev->registered; > - if (registered) > - list_add(&client->list, &dev->clientlist); > + list_add(&client->list, &dev->clientlist); > mutex_unlock(&dev->clientlist_mutex); > - if (!registered) { > - ret = -ENODEV; > - goto err_close; > - } > > drm_dev_get(dev); > > return 0; > > -err_close: > - drm_client_close(client); > err_put_module: > if (funcs) > module_put(funcs->owner); > -- > 2.15.1 >
Den 11.07.2018 20.00, skrev Daniel Vetter: > On Wed, Jul 11, 2018 at 05:56:32PM +0200, Noralf Trønnes wrote: >> Commit 894a677f4b3e ("drm/cma-helper: Use the generic fbdev emulation") >> broke almost all drivers that use the CMA helper. >> >> The reason is that drm_client_new() requires that the DRM device has >> been registered, but the drivers register fbdev before registering DRM. >> >> Remove the requirement that DRM should be registered when creating a >> new client. >> >> This creates a theoretical race condition where drm_client_new() races >> with drm_dev_unregister() resulting in the .unregister hook not being >> called. >> >> Case: >> User loads module which calls drm_client_new(). >> Device is unplugged and drm_dev_unregister() is called. >> >> If drm_dev_unregister() gets the clientlist_mutex first, the new client >> will never get it's unregister hook called. > As long as we assume that drm_client_new() is serialized against > drm_dev_unregister (which it has to, or all the current fbdev code would > be busted) I'm not seeing a race here. > > We can't allow drm_client_new concurrently with drm_dev_register, but that > feature got canned. So looks all good to me, and no races anywhere to be > seen. Assuming you agree and update the commit message: There's no problem with an fbdev or bootsplash client, but the problem is with say a kmscon client that is loaded as a module. kmscon can be loaded any time when there is a DRM device present, but DRM can be unregistered while registering this new client. At least this is how I understand this. Noralf. > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> Fixes: c76f0f7cb546 ("drm: Begin an API for in-kernel clients") >> Cc: Maxime Ripard <maxime.ripard@bootlin.com> >> Cc: Icenowy Zheng <icenowy@aosc.io> >> Cc: Chen-Yu Tsai <wens@csie.org> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >> --- >> drivers/gpu/drm/drm_client.c | 11 +---------- >> 1 file changed, 1 insertion(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c >> index 4039a4d103a8..9b142f58d489 100644 >> --- a/drivers/gpu/drm/drm_client.c >> +++ b/drivers/gpu/drm/drm_client.c >> @@ -78,7 +78,6 @@ EXPORT_SYMBOL(drm_client_close); >> int drm_client_new(struct drm_device *dev, struct drm_client_dev *client, >> const char *name, const struct drm_client_funcs *funcs) >> { >> - bool registered; >> int ret; >> >> if (!drm_core_check_feature(dev, DRIVER_MODESET) || >> @@ -97,21 +96,13 @@ int drm_client_new(struct drm_device *dev, struct drm_client_dev *client, >> goto err_put_module; >> >> mutex_lock(&dev->clientlist_mutex); >> - registered = dev->registered; >> - if (registered) >> - list_add(&client->list, &dev->clientlist); >> + list_add(&client->list, &dev->clientlist); >> mutex_unlock(&dev->clientlist_mutex); >> - if (!registered) { >> - ret = -ENODEV; >> - goto err_close; >> - } >> >> drm_dev_get(dev); >> >> return 0; >> >> -err_close: >> - drm_client_close(client); >> err_put_module: >> if (funcs) >> module_put(funcs->owner); >> -- >> 2.15.1 >>
On Wed, Jul 11, 2018 at 8:21 PM, Noralf Trønnes <noralf@tronnes.org> wrote: > > Den 11.07.2018 20.00, skrev Daniel Vetter: >> >> On Wed, Jul 11, 2018 at 05:56:32PM +0200, Noralf Trønnes wrote: >>> >>> Commit 894a677f4b3e ("drm/cma-helper: Use the generic fbdev emulation") >>> broke almost all drivers that use the CMA helper. >>> >>> The reason is that drm_client_new() requires that the DRM device has >>> been registered, but the drivers register fbdev before registering DRM. >>> >>> Remove the requirement that DRM should be registered when creating a >>> new client. >>> >>> This creates a theoretical race condition where drm_client_new() races >>> with drm_dev_unregister() resulting in the .unregister hook not being >>> called. >>> >>> Case: >>> User loads module which calls drm_client_new(). >>> Device is unplugged and drm_dev_unregister() is called. >>> >>> If drm_dev_unregister() gets the clientlist_mutex first, the new client >>> will never get it's unregister hook called. >> >> As long as we assume that drm_client_new() is serialized against >> drm_dev_unregister (which it has to, or all the current fbdev code would >> be busted) I'm not seeing a race here. >> >> We can't allow drm_client_new concurrently with drm_dev_register, but that >> feature got canned. So looks all good to me, and no races anywhere to be >> seen. Assuming you agree and update the commit message: > > > There's no problem with an fbdev or bootsplash client, but the problem is > with say a kmscon client that is loaded as a module. kmscon can be loaded > any time when there is a DRM device present, but DRM can be unregistered > while registering this new client. > > At least this is how I understand this. I think we can figure out what to do with kmscon once it's being (re)submitted for upstream. No point having headaches over an issue that doesn't yet exist. My first reaction would be to handle it exactly as the fbdev stuff, and not allow kmscon to be loaded later on. -Daniel > > Noralf. > > > >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> >>> Fixes: c76f0f7cb546 ("drm: Begin an API for in-kernel clients") >>> Cc: Maxime Ripard <maxime.ripard@bootlin.com> >>> Cc: Icenowy Zheng <icenowy@aosc.io> >>> Cc: Chen-Yu Tsai <wens@csie.org> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >>> --- >>> drivers/gpu/drm/drm_client.c | 11 +---------- >>> 1 file changed, 1 insertion(+), 10 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c >>> index 4039a4d103a8..9b142f58d489 100644 >>> --- a/drivers/gpu/drm/drm_client.c >>> +++ b/drivers/gpu/drm/drm_client.c >>> @@ -78,7 +78,6 @@ EXPORT_SYMBOL(drm_client_close); >>> int drm_client_new(struct drm_device *dev, struct drm_client_dev >>> *client, >>> const char *name, const struct drm_client_funcs >>> *funcs) >>> { >>> - bool registered; >>> int ret; >>> if (!drm_core_check_feature(dev, DRIVER_MODESET) || >>> @@ -97,21 +96,13 @@ int drm_client_new(struct drm_device *dev, struct >>> drm_client_dev *client, >>> goto err_put_module; >>> mutex_lock(&dev->clientlist_mutex); >>> - registered = dev->registered; >>> - if (registered) >>> - list_add(&client->list, &dev->clientlist); >>> + list_add(&client->list, &dev->clientlist); >>> mutex_unlock(&dev->clientlist_mutex); >>> - if (!registered) { >>> - ret = -ENODEV; >>> - goto err_close; >>> - } >>> drm_dev_get(dev); >>> return 0; >>> -err_close: >>> - drm_client_close(client); >>> err_put_module: >>> if (funcs) >>> module_put(funcs->owner); >>> -- >>> 2.15.1 >>> >
Den 11.07.2018 20.53, skrev Daniel Vetter: > On Wed, Jul 11, 2018 at 8:21 PM, Noralf Trønnes <noralf@tronnes.org> wrote: >> Den 11.07.2018 20.00, skrev Daniel Vetter: >>> On Wed, Jul 11, 2018 at 05:56:32PM +0200, Noralf Trønnes wrote: >>>> Commit 894a677f4b3e ("drm/cma-helper: Use the generic fbdev emulation") >>>> broke almost all drivers that use the CMA helper. >>>> >>>> The reason is that drm_client_new() requires that the DRM device has >>>> been registered, but the drivers register fbdev before registering DRM. >>>> >>>> Remove the requirement that DRM should be registered when creating a >>>> new client. >>>> >>>> This creates a theoretical race condition where drm_client_new() races >>>> with drm_dev_unregister() resulting in the .unregister hook not being >>>> called. >>>> >>>> Case: >>>> User loads module which calls drm_client_new(). >>>> Device is unplugged and drm_dev_unregister() is called. >>>> >>>> If drm_dev_unregister() gets the clientlist_mutex first, the new client >>>> will never get it's unregister hook called. >>> As long as we assume that drm_client_new() is serialized against >>> drm_dev_unregister (which it has to, or all the current fbdev code would >>> be busted) I'm not seeing a race here. >>> >>> We can't allow drm_client_new concurrently with drm_dev_register, but that >>> feature got canned. So looks all good to me, and no races anywhere to be >>> seen. Assuming you agree and update the commit message: >> >> There's no problem with an fbdev or bootsplash client, but the problem is >> with say a kmscon client that is loaded as a module. kmscon can be loaded >> any time when there is a DRM device present, but DRM can be unregistered >> while registering this new client. >> >> At least this is how I understand this. > I think we can figure out what to do with kmscon once it's being > (re)submitted for upstream. No point having headaches over an issue > that doesn't yet exist. > > My first reaction would be to handle it exactly as the fbdev stuff, > and not allow kmscon to be loaded later on. > -Daniel Ok, I've fixed the commit message and pushed. Thanks for reviewing. Icenowy, thanks for testing. Noralf. >> Noralf. >> >> >> >>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>> >>>> Fixes: c76f0f7cb546 ("drm: Begin an API for in-kernel clients") >>>> Cc: Maxime Ripard <maxime.ripard@bootlin.com> >>>> Cc: Icenowy Zheng <icenowy@aosc.io> >>>> Cc: Chen-Yu Tsai <wens@csie.org> >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >>>> --- >>>> drivers/gpu/drm/drm_client.c | 11 +---------- >>>> 1 file changed, 1 insertion(+), 10 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c >>>> index 4039a4d103a8..9b142f58d489 100644 >>>> --- a/drivers/gpu/drm/drm_client.c >>>> +++ b/drivers/gpu/drm/drm_client.c >>>> @@ -78,7 +78,6 @@ EXPORT_SYMBOL(drm_client_close); >>>> int drm_client_new(struct drm_device *dev, struct drm_client_dev >>>> *client, >>>> const char *name, const struct drm_client_funcs >>>> *funcs) >>>> { >>>> - bool registered; >>>> int ret; >>>> if (!drm_core_check_feature(dev, DRIVER_MODESET) || >>>> @@ -97,21 +96,13 @@ int drm_client_new(struct drm_device *dev, struct >>>> drm_client_dev *client, >>>> goto err_put_module; >>>> mutex_lock(&dev->clientlist_mutex); >>>> - registered = dev->registered; >>>> - if (registered) >>>> - list_add(&client->list, &dev->clientlist); >>>> + list_add(&client->list, &dev->clientlist); >>>> mutex_unlock(&dev->clientlist_mutex); >>>> - if (!registered) { >>>> - ret = -ENODEV; >>>> - goto err_close; >>>> - } >>>> drm_dev_get(dev); >>>> return 0; >>>> -err_close: >>>> - drm_client_close(client); >>>> err_put_module: >>>> if (funcs) >>>> module_put(funcs->owner); >>>> -- >>>> 2.15.1 >>>> > >
diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c index 4039a4d103a8..9b142f58d489 100644 --- a/drivers/gpu/drm/drm_client.c +++ b/drivers/gpu/drm/drm_client.c @@ -78,7 +78,6 @@ EXPORT_SYMBOL(drm_client_close); int drm_client_new(struct drm_device *dev, struct drm_client_dev *client, const char *name, const struct drm_client_funcs *funcs) { - bool registered; int ret; if (!drm_core_check_feature(dev, DRIVER_MODESET) || @@ -97,21 +96,13 @@ int drm_client_new(struct drm_device *dev, struct drm_client_dev *client, goto err_put_module; mutex_lock(&dev->clientlist_mutex); - registered = dev->registered; - if (registered) - list_add(&client->list, &dev->clientlist); + list_add(&client->list, &dev->clientlist); mutex_unlock(&dev->clientlist_mutex); - if (!registered) { - ret = -ENODEV; - goto err_close; - } drm_dev_get(dev); return 0; -err_close: - drm_client_close(client); err_put_module: if (funcs) module_put(funcs->owner);
Commit 894a677f4b3e ("drm/cma-helper: Use the generic fbdev emulation") broke almost all drivers that use the CMA helper. The reason is that drm_client_new() requires that the DRM device has been registered, but the drivers register fbdev before registering DRM. Remove the requirement that DRM should be registered when creating a new client. This creates a theoretical race condition where drm_client_new() races with drm_dev_unregister() resulting in the .unregister hook not being called. Case: User loads module which calls drm_client_new(). Device is unplugged and drm_dev_unregister() is called. If drm_dev_unregister() gets the clientlist_mutex first, the new client will never get it's unregister hook called. Fixes: c76f0f7cb546 ("drm: Begin an API for in-kernel clients") Cc: Maxime Ripard <maxime.ripard@bootlin.com> Cc: Icenowy Zheng <icenowy@aosc.io> Cc: Chen-Yu Tsai <wens@csie.org> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- drivers/gpu/drm/drm_client.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)