lkml.org 
[lkml]   [2018]   [Oct]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/2] drm/imx: make sure to cleanup DRM before unbinding components
[adding Lucas]

On 10.10.2018 12:38, Russell King - ARM Linux wrote:
> On Tue, Oct 09, 2018 at 11:30:49PM +0200, Stefan Agner wrote:
>> In situations where a component fails to bind, a previously
>> successfully bound component might already registered itself
>> with the DRM framework (e.g. an encoder). When the master
>> component then calls drm_mode_config_cleanup, we end up in a
>> use after free sitution.
>>
>> Use the cleanup callback to make sure all framework level
>> cleanup is done by the time we unbind components.
>
> I'm not sure about this approach - the idea about the component bind
> and unbind callbacks is that unbind undoes _everything_ that bind has
> done, so everything is correctly ordered. If bind registers something,
> unbind should unregister it.
>
> What seems to be going on is that imx is registering stuff in bind()
> but not unregistering it in unbind().

Yes indeed, if that can be fixed this seems to be the better approach to
me.

>
> Since imx was one of the drivers that the component helper was
> created for, if it's now crashing, that's a regression in the imx
> driver. Looking at the commit log, I'd say:
>
> commit 8e3b16e2117409625b89807de3912ff773aea354
> Author: Lucas Stach <l.stach@pengutronix.de>
> Date: Thu Aug 11 11:18:49 2016 +0200
>
> drm/imx: don't destroy mode objects manually on driver unbind
>
> Instead let drm_mode_config_cleanup() do the work when taking down
> the master device. This requires all cleanup functions to be
> properly hooked up to the mode object .destroy callback.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>
> is probably responsible for introducing this problem, since the
> explicit calls were added by me when imx was stuck in staging due to
> the problems that the component helper solved.

The commit above does not revert cleanly today, but a quick fixing
seemed to resolve the problem I am seeing...

>
> I think what we have here are different opinions on how cleanup
> should be handled.

In the regular case using the framework cleanup function before calling
component_unbind_all() works fine.

Its really only the case where a subcomponent fails to bind where unbind
happens before calling drm_mode_config_cleanup(drm). I guess Lucas was
not aware of that special case...?

I can send a patch which properly reverts the above commit.

--
Stefan

\
 
 \ /
  Last update: 2018-10-10 13:02    [W:0.516 / U:0.320 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site