Re: [PATCH 17/37] drm/display: bridge-connector: protect dynconn creation and destruction with a mutex

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing state check inside the locked section of drm_bridge_connector_add_connector() allows overwriting an existing connector
- [High] Unprotected access to `dynconn` in drm_bridge_connector_handle_hpd() leads to NULL pointer dereference
- [High] Use-After-Free caused by mixing `drmm` cleanup actions with dynamically freed connectors
- [Medium] Mixing goto-based error handling with scope-based guard() cleanup in drm_bridge_connector_add_connector()
- [Medium] Missing mutex_destroy() or drmm_mutex_init() for `dynconn_mutex`
--

commit 83c9dccacbc0bf010c7c667632a3090398d3e6ed
Author: Luca Ceresoli <luca.ceresoli@bootlin.com>
drm/display: bridge-connector: protect dynconn creation and destruction with a mutex

This commit introduces a mutex to mutually exclude connector creation and
destruction based on bridge hotplug events. It adds dynconn_mutex to
the drm_bridge_connector structure and uses guard() to protect the
relevant sections.

> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> index bd421b6beb7c4..b83da529cc7af 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
> @@ -152,6 +153,11 @@ struct drm_bridge_connector {
>  	 * @dynconn: The DRM connector added by the drm_bridge_connector
>  	 */
>  	struct drm_bridge_connector_dynconn *dynconn;
> +	/**
> +	 * @dynconn_mutex: Protect @dynconn from concurrent creation and
> +	 * destruction
> +	 */
> +	struct mutex dynconn_mutex;
>  };

Does drm_bridge_connector_handle_hpd() need to use this new mutex?

Since the connector lifecycle is now dynamic, if an asynchronous HPD event
fires concurrently with a bridge detachment (which calls
drm_bridge_connector_dynconn_release() and sets dynconn to NULL under the
mutex), could drm_bridge_connector_handle_hpd() fetch the connector without
the lock and trigger a NULL pointer dereference or use-after-free?

Could the dynamic lifecycle of the connector also cause a use-after-free
with drmm cleanup actions?

drm_bridge_connector_add_connector() calls
drm_bridge_connector_init_hdmi_audio_cec(), which uses
drmm_connector_hdmi_cec_register() to register a cleanup action tied to the
drm_device's lifetime.

Because the connector is now dynamically managed and freed via kfree when a
bridge is detached, won't the drmm cleanup action hold a stale pointer and
execute on an already-freed connector when the DRM device is unregistered?

[ ... ]
> @@ -1050,6 +1058,8 @@ static int drm_bridge_connector_add_connector(struct drm_bridge_connector *bridg
>  	int connector_type;
>  	int ret;
>  
> +	guard(mutex)(&bridge_connector->dynconn_mutex);
> +

Does introducing guard() here violate the cleanup subsystem guidelines?

The kernel cleanup subsystem guidelines forbid mixing goto-based error
handling with scope-based cleanups like guard() in the same function because
it creates confusing ownership semantics.

Later in this function we see:

	if (ret)
		goto err_put;

	return 0;

err_put:
	drm_connector_put(connector);
	bridge_connector->dynconn = NULL;
	return ret;

Could this mix of guard() and goto cause issues?

>  	struct drm_bridge_connector_dynconn *dynconn = kzalloc_obj(*dynconn);

Could concurrent hotplug events overwrite an existing connector here?

While later patches in the series introduce
drm_bridge_connector_try_complete() which checks
if (bridge_connector->dynconn), that lockless check happens outside of
dynconn_mutex.

If two threads pass that check simultaneously, they will sequentially
acquire dynconn_mutex here. Would the second thread then unconditionally
overwrite bridge_connector->dynconn with a new allocation, leaking the
first connector's memory and leaving a dangling registered connector?

[ ... ]
> @@ -1177,6 +1187,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  	if (!bridge_connector)
>  		return ERR_PTR(-ENOMEM);
>  
> +	mutex_init(&bridge_connector->dynconn_mutex);

Should this be drmm_mutex_init() instead?

bridge_connector is allocated with drmm_kzalloc(), meaning its memory is
tied to the DRM device's lifecycle. Since the registered cleanup action
drm_bridge_connector_fini() does not call mutex_destroy(), could freeing
the structure leave an active mutex and trigger kernel debug warnings when
CONFIG_DEBUG_MUTEXES is enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-drm-bridge-hotplug-v1-0-45e2bdb3dfb4@bootlin.com?part=17