Re: [PATCH v8 02/39] 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 2 potential
issues to consider:
- [Medium] drm/bridge-helper: connection_mutex dropped in
  drm_bridge_helper_reset_crtc()
- [Low] drm/bridge-helper: inaccurate backoff description in commit
  message
--

commit 9e2a98821e3fc25f6def63b07ed345888ea3f8d2
Author: Maxime Ripard <mripard@kernel.org>

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

This commit stops drm_bridge_helper_reset_crtc() from unlocking
connection_mutex on exit, re-acquires connection_mutex after
drm_atomic_get_connector_for_encoder(), and validates the connector's
best_encoder and crtc before resetting the CRTC.

> If either of those functions detects a deadlock, it backs off and drops
> every lock held by the context, including the connection mutex taken by
> drm_bridge_helper_reset_crtc() itself. The function however still
> unlocked the connection mutex unconditionally on its way out, unlocking
> a mutex it wasn't holding anymore.
> 
> The callers already have to release the context locks through
> drm_modeset_drop_locks() and to handle the -EDEADLK error, so we can
> leave the connection mutex release to them. We still need to take it
> back after drm_atomic_get_connector_for_encoder() returns, since it may
> have backed off, before looking at the connector state.

[Severity: Low]
Is this description of how connection_mutex gets dropped accurate?

Neither drm_atomic_get_connector_for_encoder() nor
drm_atomic_helper_reset_crtc() calls drm_modeset_backoff() on -EDEADLK;
drm_modeset_lock() only records ctx->contended and returns -EDEADLK for
the caller to back off.  Instead, as the new code comment notes,
drm_atomic_get_connector_for_encoder() unconditionally calls
drm_modeset_unlock(&dev->mode_config.connection_mutex) on its normal exit
path, which is why connection_mutex was already unlocked at the old
goto out label and why it must be re-acquired before inspecting
connector->state.

> diff --git a/drivers/gpu/drm/drm_bridge_helper.c b/drivers/gpu/drm/drm_bridge_helper.c
> index 0a3c8fee66b32..2efa4e1bc4b71 100644
> --- a/drivers/gpu/drm/drm_bridge_helper.c
> +++ b/drivers/gpu/drm/drm_bridge_helper.c
[ ... ]
> @@ -33,26 +37,45 @@ 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() drops the connection
> +	 * mutex on its way out, whether or not it was held through @ctx,
> +	 * so we need to 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)
> +		return -EINVAL;
>  
> -	if (!connector->state) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> +	/*
> +	 * A concurrent atomic commit may have run while the connection
> +	 * mutex was released above, and re-routed the connector to
> +	 * another encoder or CRTC. Make sure the connector is still
> +	 * tied to our encoder before resetting anything.
> +	 */
> +	if (connector->state->best_encoder != encoder)
> +		return -EINVAL;

[Severity: Medium]
Why lock connection_mutex before drm_atomic_get_connector_for_encoder()
if that helper immediately unlocks it on exit?

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_modeset_unlock(&dev->mode_config.connection_mutex);

	return out_connector;
}

Because drm_atomic_get_connector_for_encoder() drops connection_mutex
unconditionally, the first drm_modeset_lock() in
drm_bridge_helper_reset_crtc() is immediately undone.  If connection_mutex
was already held in ctx on entry (for example, slow-locked via
drm_modeset_backoff() after the second drm_modeset_lock() returned
-EDEADLK), drm_atomic_get_connector_for_encoder() drops that slow-locked
mutex and removes it from ctx->locked, so the second drm_modeset_lock()
acquires it again on the normal path and can hit -EDEADLK again.

Dropping and re-acquiring connection_mutex also opens a window where a
concurrent atomic commit can move encoder to a different connector,
causing the connector->state->best_encoder != encoder check to fail with
-EINVAL even though encoder is still active on the new connector.  If
drm_atomic_get_connector_for_encoder() leaves connection_mutex locked in
ctx for the caller to drop via drm_modeset_drop_locks(), connection_mutex
stays held across the lookup without a second lock acquisition or race
window.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-drm-state-readout-v8-0-cbe78624e1bc@kernel.org?part=2