Re: [PATCH v7 01/34] drm/bridge-helper: Don't unlock the connection mutex after a backoff

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/bridge-helper: lock drop and NULL deref in drm_bridge_helper_reset_crtc()
--

commit 110ed6cb32b618d9ec32a80d2d60952c2bc006ae
Author: Maxime Ripard <mripard@kernel.org>

drm/bridge-helper: Don't unlock the connection mutex after a backoff

This commit removes the unconditional connection_mutex unlock in
drm_bridge_helper_reset_crtc() and re-acquires connection_mutex after calling
drm_atomic_get_connector_for_encoder(), leaving lock release to the caller via
drm_modeset_drop_locks() or drm_modeset_backoff().

> diff --git a/drivers/gpu/drm/drm_bridge_helper.c b/drivers/gpu/drm/drm_bridge_helper.c
> index 0a3c8fee66b32..6998e7c41d30e 100644
> --- a/drivers/gpu/drm/drm_bridge_helper.c
> +++ b/drivers/gpu/drm/drm_bridge_helper.c

[ ... ]

> @@ -33,26 +37,33 @@ int drm_bridge_helper_reset_crtc(struct drm_bridge *bridge,
>  	struct drm_crtc *crtc;
>  	int ret;
>  
> +	/*
> +	 * Every lock taken with @ctx is released either by a backoff or
> +	 * by the caller through drm_modeset_drop_locks(), so we must not
> +	 * unlock the connection mutex ourselves.
> +	 */
>  	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
>  	if (ret)
>  		return ret;
>  
>  	connector = drm_atomic_get_connector_for_encoder(encoder, ctx);
> -	if (IS_ERR(connector)) {
> -		ret = PTR_ERR(connector);
> -		goto out;
> -	}
> +	if (IS_ERR(connector))
> +		return PTR_ERR(connector);
> +
> +	/*
> +	 * drm_atomic_get_connector_for_encoder() may have backed off,
> +	 * releasing the connection mutex in the process. Take it back
> +	 * before looking at the connector state.
> +	 */
> +	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> +	if (ret)
> +		return ret;
>  
> -	if (!connector->state) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> +	if (!connector->state)
> +		return -EINVAL;
>  
>  	crtc = connector->state->crtc;
> -	ret = drm_atomic_helper_reset_crtc(crtc, ctx);
>  
> -out:
> -	drm_modeset_unlock(&dev->mode_config.connection_mutex);
> -	return ret;
> +	return drm_atomic_helper_reset_crtc(crtc, ctx);

[Severity: High]
Does drm_atomic_get_connector_for_encoder() actually back off on deadlock, or
does it unconditionally unlock connection_mutex before returning?

Looking at drivers/gpu/drm/drm_atomic.c:drm_atomic_get_connector_for_encoder(),
neither it nor drm_atomic_helper_reset_crtc() calls drm_modeset_backoff().
Instead, drm_atomic_get_connector_for_encoder() acquires connection_mutex
(which succeeds as a no-op via -EALREADY when already held in ctx) and then
unconditionally drops it at the end of the function:

drivers/gpu/drm/drm_atomic.c:drm_atomic_get_connector_for_encoder() {
	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
	if (ret)
		return ERR_PTR(ret);

	drm_connector_list_iter_begin(dev, &conn_iter);
	drm_for_each_connector_iter(connector, &conn_iter) {
		if (!connector->state)
			continue;

		if (encoder == connector->state->best_encoder) {
			out_connector = connector;
			break;
		}
	}
	drm_connector_list_iter_end(&conn_iter);
	drm_modeset_unlock(&dev->mode_config.connection_mutex);

	return out_connector;
}

Because drm_atomic_get_connector_for_encoder() drops both the connector list
iterator reference and connection_mutex before returning, there is an unlocked
window before drm_bridge_helper_reset_crtc() re-acquires connection_mutex. Also,
if drm_atomic_get_connector_for_encoder() finds no matching connector and
returns ERR_PTR(-EINVAL), connection_mutex has already been unlocked even if
the caller held it in ctx.

If a concurrent modeset or atomic commit runs during that unlocked window (for
example, while sn65dsi83_reset_work() -> sn65dsi83_reset_pipe() ->
drm_bridge_helper_reset_crtc() is executing) and disables the connector or
reassigns it to another encoder, connector->state remains non-NULL while
connector->state->best_encoder and connector->state->crtc can become NULL or
point to a different pipeline.

When drm_bridge_helper_reset_crtc() re-locks connection_mutex, it only checks
if (!connector->state) and neither re-verifies connector->state->best_encoder
== encoder nor checks that crtc is non-NULL before calling
drm_atomic_helper_reset_crtc(crtc, ctx):

drivers/gpu/drm/drm_atomic_helper.c:drm_atomic_helper_reset_crtc() {
	...
	state = drm_atomic_commit_alloc(crtc->dev);
	if (!state)
		return -ENOMEM;
	...
}

Can this race cause a NULL pointer dereference on crtc->dev in
drm_atomic_helper_reset_crtc(), or reset the wrong CRTC?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-drm-state-readout-v7-0-25e5163d75c3@kernel.org?part=1