[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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
