Re: [PATCH v5] drm/ingenic: fix bridge allocation

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing drm_bridge_remove() in error paths and driver unbind leads to memory leaks and dangling pointers in the global bridge_list.
--

--- Patch [1]: [PATCH v5] drm/ingenic: fix bridge allocation ---
commit 25461fe1d8c0e90d5647e8b8309dd0a052459895
Author: H. Nikolaus Schaller <[email protected]>

drm/ingenic: fix bridge allocation

This commit fixes an issue with the bridge allocation API by replacing direct
struct drm_bridge allocation with devm_drm_bridge_alloc(). It also updates the
bridge lifecycle by calling drm_bridge_add() prior to drm_bridge_attach().

> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 738a80b2550ff..47f02d655b3c6 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
[ ... ]
> @@ -1316,13 +1321,27 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>  
>  		drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
>  
> -		ib->bridge.funcs = &ingenic_drm_bridge_funcs;
> +		bridge_alloc = devm_drm_bridge_alloc(priv->dev,
> +						     struct ingenic_drm_bridge_alloc,
> +						     bridge,
> +						     &ingenic_drm_bridge_funcs);
> +		if (IS_ERR(bridge_alloc)) {
> +			ret = PTR_ERR(bridge_alloc);
> +			goto err_drvdata;
> +		}
> +
> +		ib->bridge = &bridge_alloc->bridge;
> +		ib->bridge->of_node = priv->dev->of_node;
> +
>  		ib->next_bridge = bridge;
>  
> -		ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
> +		drm_bridge_add(ib->bridge);
> +
> +		ret = drm_bridge_attach(encoder, ib->bridge, NULL,
>  					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>  		if (ret) {
>  			dev_err(dev, "Unable to attach bridge\n");
> +			drm_bridge_remove(ib->bridge);
>  			goto err_drvdata;
>  		}

[Severity: High]
Does this code leak the bridge and leave a dangling pointer if a subsequent
initialization step fails?

Right after this drm_bridge_attach() block, ingenic_drm_bind() calls
drm_bridge_connector_init(). If that initialization fails (or if any later
loop iteration fails), the code jumps to err_drvdata:

		connector = drm_bridge_connector_init(drm, encoder);
		if (IS_ERR(connector)) {
			dev_err(dev, "Unable to init connector\n");
			ret = PTR_ERR(connector);
			goto err_drvdata;
		}

Because drm_bridge_remove() is only called when drm_bridge_attach() fails,
the bridge added via drm_bridge_add() is never removed from the global
bridge_list on these later error paths.

Additionally, does the driver leave the bridge in the global list during device
unbind? Looking at ingenic_drm_unbind(), there appears to be no matching call
to drm_bridge_remove() for the bridges registered in the bind loop. This could
lead to a use-after-free oops if the module is unloaded, leaving bridge->funcs
pointing to unmapped memory.

Could devm_drm_bridge_add() be used here instead to ensure the bridge is
automatically removed from the global list on error paths and during unbind?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/f77ed4e1547e452668a6549e3966471a6b638a6b.1787593585.git.hns@goldelico.com?part=1