Re: [PATCH v3 3/4] drm/panel: of_drm_find_panel() return a counted reference

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:

New issues:
- [Critical] Using `devm_add_action_or_reset()` to manage panel references inside component bind callbacks causes reference leaks, devres accumulation, and double-free on rebind.
- [High] Panel reference acquired in `tegra_output_probe()` is leaked in `tegra_dc_rgb_init()`.
- [High] Panel reference is leaked if `component_add()` or `analogix_dp_finish_probe()` fails in `exynos_dp_probe()`.
- [High] Panel reference is leaked if `component_add()` fails in `analogix_dp_aux_done_probing()`.
- [High] Panel reference is leaked if `component_add()` fails in Exynos FIMD / DECON probe error paths.
- [Medium] Panel reference is leaked when looping over multiple endpoints in `pl111_modeset_init()`.

Pre-existing issues:
- [High] An `ERR_PTR` is passed to `drm_panel_bridge_remove()` in the error path of `tve200_modeset_init()`, causing a kernel panic.
- [High] The panel bridge, encoder, and internal panel reference are leaked if encoder attachment fails in `rockchip_rgb_init()`.
- [High] Dereferencing `ERR_PTR` causes a kernel panic in the Ingenic DRM driver.
--

--- Patch [3]: [PATCH v3 3/4] drm/panel: of_drm_find_panel() return a counted reference ---
commit 5ff0041b676bfa0b0285edf9631f1c23402a5814
Author: Albert Esteve <aesteve@redhat.com>
Subject: drm/panel: of_drm_find_panel() return a counted reference

Callers of of_drm_find_panel() and drm_of_find_panel_or_bridge() receive a
pointer with no reference held, creating a window where the panel device can
be unregistered and freed between the lookup and first use. Fix the lookup
function by acquiring a reference with drm_panel_get() before returning,
under panel_lock. Callers are now responsible for calling drm_panel_put()
when they no longer need the pointer.

Link: [not provided in metadata]

> diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
> index b805403281504..a418011f7d4d6 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp.c
[ ... ]
> @@ -182,8 +182,15 @@ static int exynos_dp_probe(struct platform_device *pdev)
>  
>  out:
>  	dp->adp = analogix_dp_probe(dev, &dp->plat_data);
> -	if (IS_ERR(dp->adp))
> +	if (IS_ERR(dp->adp)) {
> +		/*
> +		 * The driver core does not invoke remove() for failed probes,
> +		 * so release the probe-time panel reference here.
> +		 */
> +		if (dp->plat_data.panel)
> +			drm_panel_put(dp->plat_data.panel);
>  		return PTR_ERR(dp->adp);
> +	}
>  
>  	if (dp->plat_data.panel || dp->plat_data.next_bridge)
>  		return component_add(&pdev->dev, &exynos_dp_ops);
>  	else
>  		return analogix_dp_finish_probe(dp->adp);

[Severity: High]
Does this leak the panel reference if component_add() or
analogix_dp_finish_probe() fails? 

The failure path for analogix_dp_probe() correctly drops the reference, but
the subsequent returns do not seem to handle failures. Because failed probes
do not trigger the remove() callback, the reference acquired via
of_drm_find_panel() might remain permanently leaked in those paths.

> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 42c86f195c66b..1887e01d29701 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
[ ... ]
> @@ -1297,9 +1297,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>  			goto err_drvdata;
>  		}
>  
> -		if (panel)
> +		if (panel) {
>  			bridge = devm_drm_panel_bridge_add_typed(dev, panel,
>  								 DRM_MODE_CONNECTOR_DPI);
> +			drm_panel_put(panel);
> +		}
>  
>  		ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder,
>  					NULL, DRM_MODE_ENCODER_DPI, NULL);

[Severity: High]
This is a pre-existing issue, but does this code cause a kernel panic if
devm_drm_panel_bridge_add_typed() fails? 

When the bridge allocation fails, it returns an ERR_PTR. This value is 
eventually assigned to ib->next_bridge and passed to drm_bridge_attach(). 
Since drm_bridge_attach() checks for NULL but not IS_ERR(), it will 
dereference the error pointer.

> diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
> index ac7b1d12a0f59..8ec659b3c08eb 100644
> --- a/drivers/gpu/drm/pl111/pl111_drv.c
> +++ b/drivers/gpu/drm/pl111/pl111_drv.c
[ ... ]
> @@ -145,6 +145,7 @@ static int pl111_modeset_init(struct drm_device *dev)
>  	if (panel) {
>  		bridge = drm_panel_bridge_add_typed(panel,
>  						    DRM_MODE_CONNECTOR_Unknown);
> +		drm_panel_put(panel);
>  		if (IS_ERR(bridge)) {
>  			ret = PTR_ERR(bridge);
>  			goto finish;

[Severity: Medium]
Will this overwrite and leak panel references when looping over multiple
endpoints? 

In pl111_modeset_init(), for_each_endpoint_of_node() iterates over endpoints
and calls drm_of_find_panel_or_bridge() on each. If multiple panels are
found, the assignment:
    panel = tmp_panel;
overwrites the previously acquired pointer without calling drm_panel_put()
on the old one, leaking all but the last panel found.

> diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c b/drivers/gpu/drm/rockchip/rockchip_rgb.c
> index add3123e5ce70..c908ea4cfa67a 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
[ ... ]
> @@ -139,6 +141,7 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
>  	if (panel) {
>  		bridge = drm_panel_bridge_add_typed(panel,
>  						    DRM_MODE_CONNECTOR_LVDS);
> +		drm_panel_put(panel);
>  		if (IS_ERR(bridge))
>  			return ERR_CAST(bridge);
>  	}

[Severity: High]
This is a pre-existing issue, but does the error path fail to clean up the
bridge created by drm_panel_bridge_add_typed()? 

If a panel is found, a bridge is allocated and a panel reference is acquired.
If subsequent calls like drm_bridge_attach() or drm_bridge_connector_init()
fail, the code jumps to err_free_encoder. This cleans up the encoder but 
appears to miss calling drm_panel_bridge_remove() to free the bridge and drop
the panel reference.

> diff --git a/drivers/gpu/drm/sun4i/sun4i_lvds.c b/drivers/gpu/drm/sun4i/sun4i_lvds.c
> index 6716e895ae8a4..e1b342c922224 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_lvds.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_lvds.c
[ ... ]
> @@ -113,6 +118,14 @@ int sun4i_lvds_init(struct drm_device *drm, struct sun4i_tcon *tcon)
>  		return 0;
>  	}
>  
> +	if (lvds->panel) {
> +		ret = devm_add_action_or_reset(tcon->dev,
> +					       sun4i_panel_put_action,
> +					       lvds->panel);
> +		if (ret)
> +			return ret;
> +	}
> +

[Severity: Critical]
Can using devm_add_action_or_reset() here cause lifecycle mismatches?

This code is called during sun4i_tcon_bind(), which is a component bind
callback. Devres actions are tied to the device's probe/remove lifecycle,
not the component bind/unbind lifecycle. 

When the component unbinds, the action will not execute, leaking the panel
reference. If the component rebinds, it adds another action. When the device
is finally removed, the accumulated actions will execute multiple times,
causing a double-free or refcount underflow on the panel.

> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
> index dfb6acc42f02e..0066bec5a9e5a 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
[ ... ]
> @@ -205,6 +210,14 @@ int sun4i_rgb_init(struct drm_device *drm, struct sun4i_tcon *tcon)
>  		return 0;
>  	}
>  
> +	if (rgb->panel) {
> +		ret = devm_add_action_or_reset(tcon->dev,
> +					       sun4i_panel_put_action,
> +					       rgb->panel);
> +		if (ret)
> +			return ret;
> +	}
> +

[Severity: Critical]
Can using devm_add_action_or_reset() here also cause lifecycle mismatches for
the same reasons as in sun4i_lvds.c?

> diff --git a/drivers/gpu/drm/tve200/tve200_drv.c b/drivers/gpu/drm/tve200/tve200_drv.c
> index 562f3f11812a3..f858c58ccb994 100644
> --- a/drivers/gpu/drm/tve200/tve200_drv.c
> +++ b/drivers/gpu/drm/tve200/tve200_drv.c
[ ... ]
> @@ -84,6 +84,7 @@ static int tve200_modeset_init(struct drm_device *dev)
>  	if (panel) {
>  		bridge = drm_panel_bridge_add_typed(panel,
>  						    DRM_MODE_CONNECTOR_Unknown);
> +		drm_panel_put(panel);
>  		if (IS_ERR(bridge)) {
>  			ret = PTR_ERR(bridge);
>  			goto out_bridge;

[Severity: High]
This is a pre-existing issue, but does this lead to a kernel panic in the
error path? 

If drm_panel_bridge_add_typed() fails, it returns an ERR_PTR. The code then
jumps to out_bridge and unconditionally calls drm_panel_bridge_remove(bridge).
Since drm_panel_bridge_remove() only checks if (!bridge) before dereferencing
the pointer, passing an ERR_PTR will cause a crash.


Additional observations about panel references in related files:

[Severity: High]
Could the panel reference acquired in tegra_output_probe() be leaked in
tegra_dc_rgb_init()? 

When tegra_dc_rgb_init() (in drivers/gpu/drm/tegra/rgb.c) wraps the panel in a
bridge using devm_drm_panel_bridge_add(), it sets output->panel = NULL. This
seems to break the cleanup path in tegra_output_remove(), which checks
if (output->panel) before calling drm_panel_put(), resulting in the probe-time
reference being permanently leaked.

[Severity: High]
Can the panel reference leak in the error paths of decon_probe() and
fimd_probe()? 

The exynos_dpi_probe() function acquires a panel reference and relies on the
callers to explicitly call exynos_dpi_remove() to release it. However, if
component_add() fails in exynos7_drm_decon.c:decon_probe() or
exynos_drm_fimd.c:fimd_probe(), they branch to an error path that skips
calling exynos_dpi_remove(), potentially leaking the reference.

[Severity: High]
Looking at analogix_dp_aux_done_probing() in analogix_dp_core.c, does this
code leak the panel reference if component_add() fails? 

A reference is acquired via drm_of_find_panel_or_bridge(), but if
component_add() fails, the error is propagated back without dropping the
reference.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-drm_refcount_wiring-v3-0-023900c32e01@redhat.com?part=3