* [PATCH] Revert "drm/imx: don't destroy mode objects manually on driver unbind"
@ 2018-10-16 16:09 Stefan Agner
2018-10-16 16:45 ` Daniel Vetter
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Stefan Agner @ 2018-10-16 16:09 UTC (permalink / raw)
To: p.zabel
Cc: airlied, rmk+kernel, l.stach, dri-devel, linux-kernel, Stefan Agner
This reverts commit 8e3b16e2117409625b89807de3912ff773aea354.
Using the component framework requires all components to undo in
->unbind what ->bind does. Unfortunately that particular commit
broke this rule. In particular, this is an issue if a single
component during probe fails. In that case, component_bind_all()
calls unbind on already succussfully bound components, and then
frees memory allocated using devm. If one of those components
registered e.g. an encoder with the framework then this leads to
use after free situations.
Revert the commit to ensure that all components properly undo
what ->bind does.
Link: https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg233327.html
Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
drivers/gpu/drm/imx/imx-drm-core.c | 4 ++--
drivers/gpu/drm/imx/imx-ldb.c | 6 ++++++
drivers/gpu/drm/imx/imx-tve.c | 3 +++
drivers/gpu/drm/imx/parallel-display.c | 3 +++
4 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 5ea0c82f9957..caa6061a98ba 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -305,11 +305,11 @@ static void imx_drm_unbind(struct device *dev)
drm_fb_cma_fbdev_fini(drm);
- drm_mode_config_cleanup(drm);
-
component_unbind_all(drm->dev, drm);
dev_set_drvdata(dev, NULL);
+ drm_mode_config_cleanup(drm);
+
drm_dev_put(drm);
}
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 3bd0f8a18e74..592aabc4a262 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -723,6 +723,12 @@ static void imx_ldb_unbind(struct device *dev, struct device *master,
if (channel->panel)
drm_panel_detach(channel->panel);
+ if (!channel->connector.funcs)
+ continue;
+
+ channel->connector.funcs->destroy(&channel->connector);
+ channel->encoder.funcs->destroy(&channel->encoder);
+
kfree(channel->edid);
i2c_put_adapter(channel->ddc);
}
diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index cffd3310240e..8d6e89ce1edb 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -673,6 +673,9 @@ static void imx_tve_unbind(struct device *dev, struct device *master,
{
struct imx_tve *tve = dev_get_drvdata(dev);
+ tve->connector.funcs->destroy(&tve->connector);
+ tve->encoder.funcs->destroy(&tve->encoder);
+
if (!IS_ERR(tve->dac_reg))
regulator_disable(tve->dac_reg);
}
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index aefd04e18f93..6f11bffcde37 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -258,6 +258,9 @@ static void imx_pd_unbind(struct device *dev, struct device *master,
if (imxpd->panel)
drm_panel_detach(imxpd->panel);
+ imxpd->encoder.funcs->destroy(&imxpd->encoder);
+ imxpd->connector.funcs->destroy(&imxpd->connector);
+
kfree(imxpd->edid);
}
--
2.19.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "drm/imx: don't destroy mode objects manually on driver unbind"
2018-10-16 16:09 [PATCH] Revert "drm/imx: don't destroy mode objects manually on driver unbind" Stefan Agner
@ 2018-10-16 16:45 ` Daniel Vetter
2018-10-16 16:51 ` Lucas Stach
2018-10-17 11:25 ` Stefan Agner
2 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2018-10-16 16:45 UTC (permalink / raw)
To: Stefan Agner
Cc: p.zabel, airlied, linux-kernel, rmk+kernel, dri-devel, Benjamin Gaignard
On Tue, Oct 16, 2018 at 06:09:23PM +0200, Stefan Agner wrote:
> This reverts commit 8e3b16e2117409625b89807de3912ff773aea354.
>
> Using the component framework requires all components to undo in
> ->unbind what ->bind does. Unfortunately that particular commit
> broke this rule. In particular, this is an issue if a single
> component during probe fails. In that case, component_bind_all()
> calls unbind on already succussfully bound components, and then
> frees memory allocated using devm. If one of those components
> registered e.g. an encoder with the framework then this leads to
> use after free situations.
>
> Revert the commit to ensure that all components properly undo
> what ->bind does.
>
> Link: https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg233327.html
> Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
Adding Benjamin, who just made the same mistake I think (and I reviewed it
... oh well).
-Daniel
> ---
> drivers/gpu/drm/imx/imx-drm-core.c | 4 ++--
> drivers/gpu/drm/imx/imx-ldb.c | 6 ++++++
> drivers/gpu/drm/imx/imx-tve.c | 3 +++
> drivers/gpu/drm/imx/parallel-display.c | 3 +++
> 4 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index 5ea0c82f9957..caa6061a98ba 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -305,11 +305,11 @@ static void imx_drm_unbind(struct device *dev)
>
> drm_fb_cma_fbdev_fini(drm);
>
> - drm_mode_config_cleanup(drm);
> -
> component_unbind_all(drm->dev, drm);
> dev_set_drvdata(dev, NULL);
>
> + drm_mode_config_cleanup(drm);
> +
> drm_dev_put(drm);
> }
>
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> index 3bd0f8a18e74..592aabc4a262 100644
> --- a/drivers/gpu/drm/imx/imx-ldb.c
> +++ b/drivers/gpu/drm/imx/imx-ldb.c
> @@ -723,6 +723,12 @@ static void imx_ldb_unbind(struct device *dev, struct device *master,
> if (channel->panel)
> drm_panel_detach(channel->panel);
>
> + if (!channel->connector.funcs)
> + continue;
> +
> + channel->connector.funcs->destroy(&channel->connector);
> + channel->encoder.funcs->destroy(&channel->encoder);
> +
> kfree(channel->edid);
> i2c_put_adapter(channel->ddc);
> }
> diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
> index cffd3310240e..8d6e89ce1edb 100644
> --- a/drivers/gpu/drm/imx/imx-tve.c
> +++ b/drivers/gpu/drm/imx/imx-tve.c
> @@ -673,6 +673,9 @@ static void imx_tve_unbind(struct device *dev, struct device *master,
> {
> struct imx_tve *tve = dev_get_drvdata(dev);
>
> + tve->connector.funcs->destroy(&tve->connector);
> + tve->encoder.funcs->destroy(&tve->encoder);
> +
> if (!IS_ERR(tve->dac_reg))
> regulator_disable(tve->dac_reg);
> }
> diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> index aefd04e18f93..6f11bffcde37 100644
> --- a/drivers/gpu/drm/imx/parallel-display.c
> +++ b/drivers/gpu/drm/imx/parallel-display.c
> @@ -258,6 +258,9 @@ static void imx_pd_unbind(struct device *dev, struct device *master,
> if (imxpd->panel)
> drm_panel_detach(imxpd->panel);
>
> + imxpd->encoder.funcs->destroy(&imxpd->encoder);
> + imxpd->connector.funcs->destroy(&imxpd->connector);
> +
> kfree(imxpd->edid);
> }
>
> --
> 2.19.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "drm/imx: don't destroy mode objects manually on driver unbind"
2018-10-16 16:09 [PATCH] Revert "drm/imx: don't destroy mode objects manually on driver unbind" Stefan Agner
2018-10-16 16:45 ` Daniel Vetter
@ 2018-10-16 16:51 ` Lucas Stach
2018-10-17 10:52 ` Stefan Agner
2018-10-17 11:25 ` Stefan Agner
2 siblings, 1 reply; 6+ messages in thread
From: Lucas Stach @ 2018-10-16 16:51 UTC (permalink / raw)
To: Stefan Agner, p.zabel; +Cc: airlied, rmk+kernel, dri-devel, linux-kernel
Am Dienstag, den 16.10.2018, 18:09 +0200 schrieb Stefan Agner:
> This reverts commit 8e3b16e2117409625b89807de3912ff773aea354.
>
> Using the component framework requires all components to undo in
> ->unbind what ->bind does. Unfortunately that particular commit
> broke this rule. In particular, this is an issue if a single
> component during probe fails. In that case, component_bind_all()
> calls unbind on already succussfully bound components, and then
> frees memory allocated using devm. If one of those components
> registered e.g. an encoder with the framework then this leads to
> use after free situations.
>
> Revert the commit to ensure that all components properly undo
> what ->bind does.
I agree with this patch in general, but I think I remember why I
changed this in the first place: dw_hdmi_imx_unbind does not destroy
the encoder that has been registered in dw_hdmi_imx_bind.
8e3b16e21174 was an (apparently wrong) attempt to fix such issues once
and for all. By reverting this commit we go back to leaking the HDMI
encoder, so I think this patch should include a fix to destroy the
encoder on unbind.
Regards,
Lucas
> Link: https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg233327.html
> > Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
> > Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
> drivers/gpu/drm/imx/imx-drm-core.c | 4 ++--
> drivers/gpu/drm/imx/imx-ldb.c | 6 ++++++
> drivers/gpu/drm/imx/imx-tve.c | 3 +++
> drivers/gpu/drm/imx/parallel-display.c | 3 +++
> 4 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index 5ea0c82f9957..caa6061a98ba 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -305,11 +305,11 @@ static void imx_drm_unbind(struct device *dev)
>
> > drm_fb_cma_fbdev_fini(drm);
>
> > - drm_mode_config_cleanup(drm);
> -
> > component_unbind_all(drm->dev, drm);
> > dev_set_drvdata(dev, NULL);
>
> > + drm_mode_config_cleanup(drm);
> +
> > drm_dev_put(drm);
> }
>
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> index 3bd0f8a18e74..592aabc4a262 100644
> --- a/drivers/gpu/drm/imx/imx-ldb.c
> +++ b/drivers/gpu/drm/imx/imx-ldb.c
> @@ -723,6 +723,12 @@ static void imx_ldb_unbind(struct device *dev, struct device *master,
> > if (channel->panel)
> > drm_panel_detach(channel->panel);
>
> > + if (!channel->connector.funcs)
> > + continue;
> +
> > + channel->connector.funcs->destroy(&channel->connector);
> > + channel->encoder.funcs->destroy(&channel->encoder);
> +
> > kfree(channel->edid);
> > i2c_put_adapter(channel->ddc);
> > }
> diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
> index cffd3310240e..8d6e89ce1edb 100644
> --- a/drivers/gpu/drm/imx/imx-tve.c
> +++ b/drivers/gpu/drm/imx/imx-tve.c
> @@ -673,6 +673,9 @@ static void imx_tve_unbind(struct device *dev, struct device *master,
> {
> > struct imx_tve *tve = dev_get_drvdata(dev);
>
> > + tve->connector.funcs->destroy(&tve->connector);
> > + tve->encoder.funcs->destroy(&tve->encoder);
> +
> > if (!IS_ERR(tve->dac_reg))
> > regulator_disable(tve->dac_reg);
> }
> diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> index aefd04e18f93..6f11bffcde37 100644
> --- a/drivers/gpu/drm/imx/parallel-display.c
> +++ b/drivers/gpu/drm/imx/parallel-display.c
> @@ -258,6 +258,9 @@ static void imx_pd_unbind(struct device *dev, struct device *master,
> > if (imxpd->panel)
> > drm_panel_detach(imxpd->panel);
>
> > + imxpd->encoder.funcs->destroy(&imxpd->encoder);
> > + imxpd->connector.funcs->destroy(&imxpd->connector);
> +
> > kfree(imxpd->edid);
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "drm/imx: don't destroy mode objects manually on driver unbind"
2018-10-16 16:51 ` Lucas Stach
@ 2018-10-17 10:52 ` Stefan Agner
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Agner @ 2018-10-17 10:52 UTC (permalink / raw)
To: Lucas Stach; +Cc: p.zabel, airlied, rmk+kernel, dri-devel, linux-kernel
On 16.10.2018 18:51, Lucas Stach wrote:
> Am Dienstag, den 16.10.2018, 18:09 +0200 schrieb Stefan Agner:
>> This reverts commit 8e3b16e2117409625b89807de3912ff773aea354.
>>
>> Using the component framework requires all components to undo in
>> ->unbind what ->bind does. Unfortunately that particular commit
>> broke this rule. In particular, this is an issue if a single
>> component during probe fails. In that case, component_bind_all()
>> calls unbind on already succussfully bound components, and then
>> frees memory allocated using devm. If one of those components
>> registered e.g. an encoder with the framework then this leads to
>> use after free situations.
>>
>> Revert the commit to ensure that all components properly undo
>> what ->bind does.
>
> I agree with this patch in general, but I think I remember why I
> changed this in the first place: dw_hdmi_imx_unbind does not destroy
> the encoder that has been registered in dw_hdmi_imx_bind.
Good point, yeah that is missing.
>
> 8e3b16e21174 was an (apparently wrong) attempt to fix such issues once
> and for all. By reverting this commit we go back to leaking the HDMI
> encoder, so I think this patch should include a fix to destroy the
> encoder on unbind.
Makes sense, will add that to the same commit and send a v2.
--
Stefan
>
> Regards,
> Lucas
>
>> Link: https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg233327.html
>> > Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
>> > Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>> drivers/gpu/drm/imx/imx-drm-core.c | 4 ++--
>> drivers/gpu/drm/imx/imx-ldb.c | 6 ++++++
>> drivers/gpu/drm/imx/imx-tve.c | 3 +++
>> drivers/gpu/drm/imx/parallel-display.c | 3 +++
>> 4 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
>> index 5ea0c82f9957..caa6061a98ba 100644
>> --- a/drivers/gpu/drm/imx/imx-drm-core.c
>> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
>> @@ -305,11 +305,11 @@ static void imx_drm_unbind(struct device *dev)
>>
>> > drm_fb_cma_fbdev_fini(drm);
>>
>> > - drm_mode_config_cleanup(drm);
>> -
>> > component_unbind_all(drm->dev, drm);
>> > dev_set_drvdata(dev, NULL);
>>
>> > + drm_mode_config_cleanup(drm);
>> +
>> > drm_dev_put(drm);
>> }
>>
>> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
>> index 3bd0f8a18e74..592aabc4a262 100644
>> --- a/drivers/gpu/drm/imx/imx-ldb.c
>> +++ b/drivers/gpu/drm/imx/imx-ldb.c
>> @@ -723,6 +723,12 @@ static void imx_ldb_unbind(struct device *dev, struct device *master,
>> > if (channel->panel)
>> > drm_panel_detach(channel->panel);
>>
>> > + if (!channel->connector.funcs)
>> > + continue;
>> +
>> > + channel->connector.funcs->destroy(&channel->connector);
>> > + channel->encoder.funcs->destroy(&channel->encoder);
>> +
>> > kfree(channel->edid);
>> > i2c_put_adapter(channel->ddc);
>> > }
>> diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
>> index cffd3310240e..8d6e89ce1edb 100644
>> --- a/drivers/gpu/drm/imx/imx-tve.c
>> +++ b/drivers/gpu/drm/imx/imx-tve.c
>> @@ -673,6 +673,9 @@ static void imx_tve_unbind(struct device *dev, struct device *master,
>> {
>> > struct imx_tve *tve = dev_get_drvdata(dev);
>>
>> > + tve->connector.funcs->destroy(&tve->connector);
>> > + tve->encoder.funcs->destroy(&tve->encoder);
>> +
>> > if (!IS_ERR(tve->dac_reg))
>> > regulator_disable(tve->dac_reg);
>> }
>> diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
>> index aefd04e18f93..6f11bffcde37 100644
>> --- a/drivers/gpu/drm/imx/parallel-display.c
>> +++ b/drivers/gpu/drm/imx/parallel-display.c
>> @@ -258,6 +258,9 @@ static void imx_pd_unbind(struct device *dev, struct device *master,
>> > if (imxpd->panel)
>> > drm_panel_detach(imxpd->panel);
>>
>> > + imxpd->encoder.funcs->destroy(&imxpd->encoder);
>> > + imxpd->connector.funcs->destroy(&imxpd->connector);
>> +
>> > kfree(imxpd->edid);
>> }
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "drm/imx: don't destroy mode objects manually on driver unbind"
2018-10-16 16:09 [PATCH] Revert "drm/imx: don't destroy mode objects manually on driver unbind" Stefan Agner
2018-10-16 16:45 ` Daniel Vetter
2018-10-16 16:51 ` Lucas Stach
@ 2018-10-17 11:25 ` Stefan Agner
2018-10-17 12:00 ` Stefan Agner
2 siblings, 1 reply; 6+ messages in thread
From: Stefan Agner @ 2018-10-17 11:25 UTC (permalink / raw)
To: p.zabel; +Cc: airlied, rmk+kernel, l.stach, dri-devel, linux-kernel
On 16.10.2018 18:09, Stefan Agner wrote:
> This reverts commit 8e3b16e2117409625b89807de3912ff773aea354.
>
> Using the component framework requires all components to undo in
> ->unbind what ->bind does. Unfortunately that particular commit
> broke this rule. In particular, this is an issue if a single
> component during probe fails. In that case, component_bind_all()
> calls unbind on already succussfully bound components, and then
> frees memory allocated using devm. If one of those components
> registered e.g. an encoder with the framework then this leads to
> use after free situations.
>
> Revert the commit to ensure that all components properly undo
> what ->bind does.
After Lucas comment mentioning HDMI unbind is not proper I looked
through all the unbind again. The other unbind functions need some
fixing too. I did not bother checking whether those were always broken
or just because things changed (the commit this is reverting was in
2016).... Here is what I found:
>
> Link:
> https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg233327.html
> Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
> drivers/gpu/drm/imx/imx-drm-core.c | 4 ++--
> drivers/gpu/drm/imx/imx-ldb.c | 6 ++++++
> drivers/gpu/drm/imx/imx-tve.c | 3 +++
> drivers/gpu/drm/imx/parallel-display.c | 3 +++
> 4 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c
> b/drivers/gpu/drm/imx/imx-drm-core.c
> index 5ea0c82f9957..caa6061a98ba 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -305,11 +305,11 @@ static void imx_drm_unbind(struct device *dev)
>
> drm_fb_cma_fbdev_fini(drm);
>
> - drm_mode_config_cleanup(drm);
> -
> component_unbind_all(drm->dev, drm);
> dev_set_drvdata(dev, NULL);
>
> + drm_mode_config_cleanup(drm);
> +
> drm_dev_put(drm);
> }
>
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> index 3bd0f8a18e74..592aabc4a262 100644
> --- a/drivers/gpu/drm/imx/imx-ldb.c
> +++ b/drivers/gpu/drm/imx/imx-ldb.c
> @@ -723,6 +723,12 @@ static void imx_ldb_unbind(struct device *dev,
> struct device *master,
> if (channel->panel)
> drm_panel_detach(channel->panel);
>
> + if (!channel->connector.funcs)
> + continue;
> +
> + channel->connector.funcs->destroy(&channel->connector);
> + channel->encoder.funcs->destroy(&channel->encoder);
There can be an encoder and bridge, or an encoder and connector. All of
them should be properly cleaned up.
So I guess this should look like this:
if (channel->panel)
drm_panel_detach(channel->panel);
if (channel->bridge)
drm_bridge_detach(channel->bridge);
if (channel->connector.funcs)
channel->connector.funcs->destroy(&channel->connector);
if (channel->encoder.funcs)
channel->encoder.funcs->destroy(&channel->encoder);
kfree(channel->edid);
i2c_put_adapter(channel->ddc);
The last two functions following are only strictly necessary when
connector is initialized. But its safe to call them with null, so I
would just call them always.
> +
> kfree(channel->edid);
> i2c_put_adapter(channel->ddc);
> }
> diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
> index cffd3310240e..8d6e89ce1edb 100644
> --- a/drivers/gpu/drm/imx/imx-tve.c
> +++ b/drivers/gpu/drm/imx/imx-tve.c
> @@ -673,6 +673,9 @@ static void imx_tve_unbind(struct device *dev,
> struct device *master,
> {
> struct imx_tve *tve = dev_get_drvdata(dev);
>
> + tve->connector.funcs->destroy(&tve->connector);
> + tve->encoder.funcs->destroy(&tve->encoder);
> +
Cleanup of tve->ddc missing.
> if (!IS_ERR(tve->dac_reg))
> regulator_disable(tve->dac_reg);
> }
> diff --git a/drivers/gpu/drm/imx/parallel-display.c
> b/drivers/gpu/drm/imx/parallel-display.c
> index aefd04e18f93..6f11bffcde37 100644
> --- a/drivers/gpu/drm/imx/parallel-display.c
> +++ b/drivers/gpu/drm/imx/parallel-display.c
> @@ -258,6 +258,9 @@ static void imx_pd_unbind(struct device *dev,
> struct device *master,
> if (imxpd->panel)
> drm_panel_detach(imxpd->panel);
>
And in this case a bridge detach is missing.
Will send a v2 with that addressed.
--
Stefan
> + imxpd->encoder.funcs->destroy(&imxpd->encoder);
> + imxpd->connector.funcs->destroy(&imxpd->connector);
> +
> kfree(imxpd->edid);
> }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "drm/imx: don't destroy mode objects manually on driver unbind"
2018-10-17 11:25 ` Stefan Agner
@ 2018-10-17 12:00 ` Stefan Agner
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Agner @ 2018-10-17 12:00 UTC (permalink / raw)
To: p.zabel; +Cc: airlied, rmk+kernel, l.stach, dri-devel, linux-kernel
On 17.10.2018 13:25, Stefan Agner wrote:
> On 16.10.2018 18:09, Stefan Agner wrote:
>> This reverts commit 8e3b16e2117409625b89807de3912ff773aea354.
>>
>> Using the component framework requires all components to undo in
>> ->unbind what ->bind does. Unfortunately that particular commit
>> broke this rule. In particular, this is an issue if a single
>> component during probe fails. In that case, component_bind_all()
>> calls unbind on already succussfully bound components, and then
>> frees memory allocated using devm. If one of those components
>> registered e.g. an encoder with the framework then this leads to
>> use after free situations.
>>
>> Revert the commit to ensure that all components properly undo
>> what ->bind does.
>
> After Lucas comment mentioning HDMI unbind is not proper I looked
> through all the unbind again. The other unbind functions need some
> fixing too. I did not bother checking whether those were always broken
> or just because things changed (the commit this is reverting was in
> 2016).... Here is what I found:
>
>>
>> Link:
>> https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg233327.html
>> Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>> drivers/gpu/drm/imx/imx-drm-core.c | 4 ++--
>> drivers/gpu/drm/imx/imx-ldb.c | 6 ++++++
>> drivers/gpu/drm/imx/imx-tve.c | 3 +++
>> drivers/gpu/drm/imx/parallel-display.c | 3 +++
>> 4 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c
>> b/drivers/gpu/drm/imx/imx-drm-core.c
>> index 5ea0c82f9957..caa6061a98ba 100644
>> --- a/drivers/gpu/drm/imx/imx-drm-core.c
>> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
>> @@ -305,11 +305,11 @@ static void imx_drm_unbind(struct device *dev)
>>
>> drm_fb_cma_fbdev_fini(drm);
>>
>> - drm_mode_config_cleanup(drm);
>> -
>> component_unbind_all(drm->dev, drm);
>> dev_set_drvdata(dev, NULL);
>>
>> + drm_mode_config_cleanup(drm);
>> +
>> drm_dev_put(drm);
>> }
>>
>> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
>> index 3bd0f8a18e74..592aabc4a262 100644
>> --- a/drivers/gpu/drm/imx/imx-ldb.c
>> +++ b/drivers/gpu/drm/imx/imx-ldb.c
>> @@ -723,6 +723,12 @@ static void imx_ldb_unbind(struct device *dev,
>> struct device *master,
>> if (channel->panel)
>> drm_panel_detach(channel->panel);
>>
>> + if (!channel->connector.funcs)
>> + continue;
>> +
>> + channel->connector.funcs->destroy(&channel->connector);
>> + channel->encoder.funcs->destroy(&channel->encoder);
>
> There can be an encoder and bridge, or an encoder and connector. All of
> them should be properly cleaned up.
>
> So I guess this should look like this:
>
> if (channel->panel)
> drm_panel_detach(channel->panel);
>
> if (channel->bridge)
> drm_bridge_detach(channel->bridge);
Actually, when a bridge is attached to an encoder, which is the case
when unbind is getting called, then encoder cleanup takes care of
drm_bridge_detach.
>
> if (channel->connector.funcs)
> channel->connector.funcs->destroy(&channel->connector);
>
> if (channel->encoder.funcs)
> channel->encoder.funcs->destroy(&channel->encoder);
>
> kfree(channel->edid);
> i2c_put_adapter(channel->ddc);
>
> The last two functions following are only strictly necessary when
> connector is initialized. But its safe to call them with null, so I
> would just call them always.
>
>
>> +
>> kfree(channel->edid);
>> i2c_put_adapter(channel->ddc);
>> }
>> diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
>> index cffd3310240e..8d6e89ce1edb 100644
>> --- a/drivers/gpu/drm/imx/imx-tve.c
>> +++ b/drivers/gpu/drm/imx/imx-tve.c
>> @@ -673,6 +673,9 @@ static void imx_tve_unbind(struct device *dev,
>> struct device *master,
>> {
>> struct imx_tve *tve = dev_get_drvdata(dev);
>>
>> + tve->connector.funcs->destroy(&tve->connector);
>> + tve->encoder.funcs->destroy(&tve->encoder);
>> +
>
> Cleanup of tve->ddc missing.
>
>> if (!IS_ERR(tve->dac_reg))
>> regulator_disable(tve->dac_reg);
>> }
>> diff --git a/drivers/gpu/drm/imx/parallel-display.c
>> b/drivers/gpu/drm/imx/parallel-display.c
>> index aefd04e18f93..6f11bffcde37 100644
>> --- a/drivers/gpu/drm/imx/parallel-display.c
>> +++ b/drivers/gpu/drm/imx/parallel-display.c
>> @@ -258,6 +258,9 @@ static void imx_pd_unbind(struct device *dev,
>> struct device *master,
>> if (imxpd->panel)
>> drm_panel_detach(imxpd->panel);
>>
>
> And in this case a bridge detach is missing.
Same here.
>
> Will send a v2 with that addressed.
Still valid.
--
Stefan
>
> --
> Stefan
>
>> + imxpd->encoder.funcs->destroy(&imxpd->encoder);
>> + imxpd->connector.funcs->destroy(&imxpd->connector);
>> +
>> kfree(imxpd->edid);
>> }
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-17 12:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-16 16:09 [PATCH] Revert "drm/imx: don't destroy mode objects manually on driver unbind" Stefan Agner
2018-10-16 16:45 ` Daniel Vetter
2018-10-16 16:51 ` Lucas Stach
2018-10-17 10:52 ` Stefan Agner
2018-10-17 11:25 ` Stefan Agner
2018-10-17 12:00 ` Stefan Agner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).