Re: [PATCH 2/2] drm/imx: make sure to cleanup DRM before unbinding components

[Stefan Agner]

[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@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxx>
>     Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> 
> 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