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

[Russell King - ARM Linux <linux@armlinux.org.uk>]

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().

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.

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up