[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