| Message ID | 20250409214422.1751825-1-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | drm/meson: fix resource cleanup in meson_drv_bind_master() on error | expand |
Hi Martin, On Thu, 10 Apr 2025 at 03:30, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > > meson_drv_bind_master() does not free resources in the order they are > allocated. This can lead to crashes such as: > Unable to handle kernel NULL pointer dereference at virtual address 00000000000000c8 > [...] > Hardware name: Beelink GT-King Pro (DT) > pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > pc : meson_dw_hdmi_unbind+0x10/0x24 [meson_dw_hdmi] > lr : component_unbind+0x38/0x60 > [...] > Call trace: > meson_dw_hdmi_unbind+0x10/0x24 [meson_dw_hdmi] > component_unbind+0x38/0x60 > component_unbind_all+0xb8/0xc4 > meson_drv_bind_master+0x1ec/0x514 [meson_drm] > meson_drv_bind+0x14/0x20 [meson_drm] > try_to_bring_up_aggregate_device+0xa8/0x160 > __component_add+0xb8/0x1a8 > component_add+0x14/0x20 > meson_dw_hdmi_probe+0x1c/0x28 [meson_dw_hdmi] > platform_probe+0x68/0xdc > really_probe+0xc0/0x39c > __driver_probe_device+0x7c/0x14c > driver_probe_device+0x3c/0x120 > __driver_attach+0xc4/0x200 > bus_for_each_dev+0x78/0xd8 > driver_attach+0x24/0x30 > bus_add_driver+0x110/0x240 > driver_register+0x68/0x124 > __platform_driver_register+0x24/0x30 > meson_dw_hdmi_platform_driver_init+0x20/0x1000 [meson_dw_hdmi] > do_one_initcall+0x50/0x1bc > do_init_module+0x54/0x1fc > load_module+0x788/0x884 > init_module_from_file+0x88/0xd4 > __arm64_sys_finit_module+0x248/0x340 > invoke_syscall+0x48/0x104 > el0_svc_common.constprop.0+0x40/0xe0 > do_el0_svc+0x1c/0x28 > el0_svc+0x30/0xcc > el0t_64_sync_handler+0x120/0x12c > el0t_64_sync+0x190/0x194 > > Clean up resources in the error path in the same order and under the > same conditions as they were allocated to fix this. > > Reported-by: Furkan Kardame <f.kardame@manjaro.org> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > This issue was reported off-list so I'm unable to provide a link to the > report. > > I'm not sure which Fixes tag fits best. My preference so far is > Fixes: 6a044642988b ("drm/meson: fix unbind path if HDMI fails to bind") > but I'll happily take other suggestions as well. > Thanks for this Fix. Reviewed-by: Anand Moon <linux.amoon@gmail.com> Tested-by: Anand Moon <linux.amoon@gmail.com> I have tested with the following script on AML-S905X-CC --------- #! /bin/bash set +x cd /sys/bus/platform/drivers/meson-drm/ for i in $(seq 1 10); do echo "========================" $i echo d0100000.vpu > unbind /usr/bin/sleep 1 echo d0100000.vpu > bind /usr/bin/sleep 1 done Thanks -Anand
Hi Martin,
Thank you for the patch.
I encountered this issue some time ago as well and had a possible fix in my tree (see
below).
My apologies for not upstreaming it earlier.
While my fix is not as symmetric as yours—I like symmetry—it is somewhat simpler. It
did make the assumption that only calling component_unbind_all() was at fault and the the rest of the
code was correct. Therefore, calling one of the following functions:
meson_encoder_dsi_remove()
meson_encoder_hdmi_remove()
meson_encoder_cvbs_remove()
in case their counterpart was not called, should not result in any issues.
I just verified, and, as far as I understand, all of these functions do a check to confirm
whether the encoder was initialized before proceeding with cleanup.
-----------------------------------------------------
diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c
index 81d2ee37e773..4e2d45a271c2 100644
--- a/drivers/gpu/drm/meson/meson_drv.c
+++ b/drivers/gpu/drm/meson/meson_drv.c
@@ -184,6 +184,7 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
const struct meson_drm_match_data *match;
struct meson_drm *priv;
struct drm_device *drm;
+ bool do_unbind = false;
struct resource *res;
void __iomem *regs;
int ret, i;
@@ -312,10 +313,9 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
ret = component_bind_all(dev, drm);
if (ret) {
dev_err(drm->dev, "Couldn't bind all components\n");
- /* Do not try to unbind */
- has_components = false;
goto exit_afbcd;
}
+ do_unbind = true;
}
ret = meson_encoder_hdmi_probe(priv);
@@ -378,7 +378,7 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
meson_encoder_hdmi_remove(priv);
meson_encoder_cvbs_remove(priv);
- if (has_components)
+ if (do_unbind)
component_unbind_all(dev, drm);
return ret;
-----------------------------
This patch has somewhat less code redundancy. I don’t have a strong
preference for either patch, so please feel free to choose whichever you
prefer.
If you decide to go with yours, I’ve provided one minor comment inline
for your consideration.
> -----Original Message-----
> From: linux-amlogic <linux-amlogic-bounces@lists.infradead.org> On Behalf
> Of Martin Blumenstingl
> Sent: Wednesday, April 9, 2025 11:44 PM
> To: linux-amlogic@lists.infradead.org; dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> jbrunet@baylibre.com; neil.armstrong@linaro.org; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Furkan Kardame
> <f.kardame@manjaro.org>
> Subject: [PATCH] drm/meson: fix resource cleanup in
> meson_drv_bind_master() on error
>
> meson_drv_bind_master() does not free resources in the order they are
> allocated. This can lead to crashes such as:
> Unable to handle kernel NULL pointer dereference at virtual address
> 00000000000000c8
> [...]
> Hardware name: Beelink GT-King Pro (DT)
> pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : meson_dw_hdmi_unbind+0x10/0x24 [meson_dw_hdmi]
> lr : component_unbind+0x38/0x60
> [...]
> Call trace:
> meson_dw_hdmi_unbind+0x10/0x24 [meson_dw_hdmi]
> component_unbind+0x38/0x60
> component_unbind_all+0xb8/0xc4
> meson_drv_bind_master+0x1ec/0x514 [meson_drm]
> meson_drv_bind+0x14/0x20 [meson_drm]
> try_to_bring_up_aggregate_device+0xa8/0x160
> __component_add+0xb8/0x1a8
> component_add+0x14/0x20
> meson_dw_hdmi_probe+0x1c/0x28 [meson_dw_hdmi]
> platform_probe+0x68/0xdc
> really_probe+0xc0/0x39c
> __driver_probe_device+0x7c/0x14c
> driver_probe_device+0x3c/0x120
> __driver_attach+0xc4/0x200
> bus_for_each_dev+0x78/0xd8
> driver_attach+0x24/0x30
> bus_add_driver+0x110/0x240
> driver_register+0x68/0x124
> __platform_driver_register+0x24/0x30
> meson_dw_hdmi_platform_driver_init+0x20/0x1000 [meson_dw_hdmi]
> do_one_initcall+0x50/0x1bc
> do_init_module+0x54/0x1fc
> load_module+0x788/0x884
> init_module_from_file+0x88/0xd4
> __arm64_sys_finit_module+0x248/0x340
> invoke_syscall+0x48/0x104
> el0_svc_common.constprop.0+0x40/0xe0
> do_el0_svc+0x1c/0x28
> el0_svc+0x30/0xcc
> el0t_64_sync_handler+0x120/0x12c
> el0t_64_sync+0x190/0x194
>
> Clean up resources in the error path in the same order and under the
> same conditions as they were allocated to fix this.
>
> Reported-by: Furkan Kardame <f.kardame@manjaro.org>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> This issue was reported off-list so I'm unable to provide a link to the
> report.
>
> I'm not sure which Fixes tag fits best. My preference so far is
> Fixes: 6a044642988b ("drm/meson: fix unbind path if HDMI fails to bind")
> but I'll happily take other suggestions as well.
>
>
> drivers/gpu/drm/meson/meson_drv.c | 31 +++++++++++++++++--------------
> 1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_drv.c
> b/drivers/gpu/drm/meson/meson_drv.c
> index 81d2ee37e773..031686fd4104 100644
> --- a/drivers/gpu/drm/meson/meson_drv.c
> +++ b/drivers/gpu/drm/meson/meson_drv.c
> @@ -314,35 +314,35 @@ static int meson_drv_bind_master(struct device
> *dev, bool has_components)
> dev_err(drm->dev, "Couldn't bind all
> components\n");
> /* Do not try to unbind */
> has_components = false;
Above two lines are no longer used, so can be removed.
> - goto exit_afbcd;
> + goto cvbs_encoder_remove;
> }
> }
>
> ret = meson_encoder_hdmi_probe(priv);
> if (ret)
> - goto exit_afbcd;
> + goto unbind_components;
>
> if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
> ret = meson_encoder_dsi_probe(priv);
> if (ret)
> - goto exit_afbcd;
> + goto hdmi_encoder_remove;
> }
>
> ret = meson_plane_create(priv);
> if (ret)
> - goto exit_afbcd;
> + goto dsi_encoder_remove;
>
> ret = meson_overlay_create(priv);
> if (ret)
> - goto exit_afbcd;
> + goto dsi_encoder_remove;
>
> ret = meson_crtc_create(priv);
> if (ret)
> - goto exit_afbcd;
> + goto dsi_encoder_remove;
>
> ret = request_irq(priv->vsync_irq, meson_irq, 0, drm->driver->name,
> drm);
> if (ret)
> - goto exit_afbcd;
> + goto dsi_encoder_remove;
>
> drm_mode_config_reset(drm);
>
> @@ -360,6 +360,16 @@ static int meson_drv_bind_master(struct device
> *dev, bool has_components)
>
> uninstall_irq:
> free_irq(priv->vsync_irq, drm);
> +dsi_encoder_remove:
> + if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
> + meson_encoder_dsi_remove(priv);
> +hdmi_encoder_remove:
> + meson_encoder_hdmi_remove(priv);
> +unbind_components:
> + if (has_components)
> + component_unbind_all(dev, drm);
> +cvbs_encoder_remove:
> + meson_encoder_cvbs_remove(priv);
> exit_afbcd:
> if (priv->afbcd.ops)
> priv->afbcd.ops->exit(priv);
> @@ -374,13 +384,6 @@ static int meson_drv_bind_master(struct device
> *dev, bool has_components)
> free_drm:
> drm_dev_put(drm);
>
> - meson_encoder_dsi_remove(priv);
> - meson_encoder_hdmi_remove(priv);
> - meson_encoder_cvbs_remove(priv);
> -
> - if (has_components)
> - component_unbind_all(dev, drm);
> -
> return ret;
> }
>
--
Best regards,
Martijn
diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c index 81d2ee37e773..031686fd4104 100644 --- a/drivers/gpu/drm/meson/meson_drv.c +++ b/drivers/gpu/drm/meson/meson_drv.c @@ -314,35 +314,35 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) dev_err(drm->dev, "Couldn't bind all components\n"); /* Do not try to unbind */ has_components = false; - goto exit_afbcd; + goto cvbs_encoder_remove; } } ret = meson_encoder_hdmi_probe(priv); if (ret) - goto exit_afbcd; + goto unbind_components; if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) { ret = meson_encoder_dsi_probe(priv); if (ret) - goto exit_afbcd; + goto hdmi_encoder_remove; } ret = meson_plane_create(priv); if (ret) - goto exit_afbcd; + goto dsi_encoder_remove; ret = meson_overlay_create(priv); if (ret) - goto exit_afbcd; + goto dsi_encoder_remove; ret = meson_crtc_create(priv); if (ret) - goto exit_afbcd; + goto dsi_encoder_remove; ret = request_irq(priv->vsync_irq, meson_irq, 0, drm->driver->name, drm); if (ret) - goto exit_afbcd; + goto dsi_encoder_remove; drm_mode_config_reset(drm); @@ -360,6 +360,16 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) uninstall_irq: free_irq(priv->vsync_irq, drm); +dsi_encoder_remove: + if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) + meson_encoder_dsi_remove(priv); +hdmi_encoder_remove: + meson_encoder_hdmi_remove(priv); +unbind_components: + if (has_components) + component_unbind_all(dev, drm); +cvbs_encoder_remove: + meson_encoder_cvbs_remove(priv); exit_afbcd: if (priv->afbcd.ops) priv->afbcd.ops->exit(priv); @@ -374,13 +384,6 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) free_drm: drm_dev_put(drm); - meson_encoder_dsi_remove(priv); - meson_encoder_hdmi_remove(priv); - meson_encoder_cvbs_remove(priv); - - if (has_components) - component_unbind_all(dev, drm); - return ret; }
meson_drv_bind_master() does not free resources in the order they are allocated. This can lead to crashes such as: Unable to handle kernel NULL pointer dereference at virtual address 00000000000000c8 [...] Hardware name: Beelink GT-King Pro (DT) pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : meson_dw_hdmi_unbind+0x10/0x24 [meson_dw_hdmi] lr : component_unbind+0x38/0x60 [...] Call trace: meson_dw_hdmi_unbind+0x10/0x24 [meson_dw_hdmi] component_unbind+0x38/0x60 component_unbind_all+0xb8/0xc4 meson_drv_bind_master+0x1ec/0x514 [meson_drm] meson_drv_bind+0x14/0x20 [meson_drm] try_to_bring_up_aggregate_device+0xa8/0x160 __component_add+0xb8/0x1a8 component_add+0x14/0x20 meson_dw_hdmi_probe+0x1c/0x28 [meson_dw_hdmi] platform_probe+0x68/0xdc really_probe+0xc0/0x39c __driver_probe_device+0x7c/0x14c driver_probe_device+0x3c/0x120 __driver_attach+0xc4/0x200 bus_for_each_dev+0x78/0xd8 driver_attach+0x24/0x30 bus_add_driver+0x110/0x240 driver_register+0x68/0x124 __platform_driver_register+0x24/0x30 meson_dw_hdmi_platform_driver_init+0x20/0x1000 [meson_dw_hdmi] do_one_initcall+0x50/0x1bc do_init_module+0x54/0x1fc load_module+0x788/0x884 init_module_from_file+0x88/0xd4 __arm64_sys_finit_module+0x248/0x340 invoke_syscall+0x48/0x104 el0_svc_common.constprop.0+0x40/0xe0 do_el0_svc+0x1c/0x28 el0_svc+0x30/0xcc el0t_64_sync_handler+0x120/0x12c el0t_64_sync+0x190/0x194 Clean up resources in the error path in the same order and under the same conditions as they were allocated to fix this. Reported-by: Furkan Kardame <f.kardame@manjaro.org> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- This issue was reported off-list so I'm unable to provide a link to the report. I'm not sure which Fixes tag fits best. My preference so far is Fixes: 6a044642988b ("drm/meson: fix unbind path if HDMI fails to bind") but I'll happily take other suggestions as well. drivers/gpu/drm/meson/meson_drv.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)