Re: [PATCH v9 10/61] drm/bridge: Remove redundant error check in drm_bridge_helper_reset_crtc()

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

Pre-existing issues:
- [High] Unconditional unlocking of a context-managed lock causes lock stripping and double unlocks.
- [High] drm_atomic_get_connector_for_encoder() returns a connector without holding a reference, leading to a Use-After-Free.
--

--- Patch [10]: [PATCH v9 10/61] drm/bridge: Remove redundant error check in drm_bridge_helper_reset_crtc() ---
commit 9b5cf69b4268daeb09910d2aa6efb179fd6ada06
Author: Cristian Ciocaltea <[email protected]>

drm/bridge: Remove redundant error check in drm_bridge_helper_reset_crtc()

This commit removes the no-op error check after
drm_atomic_helper_reset_crtc() since the goto target is the immediately
following label. The return value is already propagated correctly
without it.

> diff --git a/drivers/gpu/drm/drm_bridge_helper.c b/drivers/gpu/drm/drm_bridge_helper.c
> index 420f29cf3e543..0a3c8fee66b32 100644
> --- a/drivers/gpu/drm/drm_bridge_helper.c
> +++ b/drivers/gpu/drm/drm_bridge_helper.c
> @@ -50,8 +50,6 @@ int drm_bridge_helper_reset_crtc(struct drm_bridge *bridge,
>  
>  	crtc = connector->state->crtc;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This is a pre-existing issue, but can this dereference cause a Use-After-Free?

Looking at drm_atomic_get_connector_for_encoder() called earlier in this
function, it finds a matching connector and breaks out of the
drm_for_each_connector_iter() loop. It then calls
drm_connector_list_iter_end(), which drops the reference on the found
connector.

The function then returns this connector without acquiring a new reference
via drm_connector_get().

If a concurrent hotplug event (like an MST display unplug) unregisters and
frees the connector before we dereference connector->state here, would it
result in a Use-After-Free?

>  	ret = drm_atomic_helper_reset_crtc(crtc, ctx);
> -	if (ret)
> -		goto out;
>  
>  out:
>  	drm_modeset_unlock(&dev->mode_config.connection_mutex);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but can this unconditional unlock
lead to a double-unlock?

Earlier in this function, drm_atomic_get_connector_for_encoder() is called
and passed the acquire context ctx. Since connection_mutex was already locked
in ctx, the inner function's lock attempt returns -EALREADY (which is
treated as success).

However, at the end of drm_atomic_get_connector_for_encoder(), it
unconditionally calls drm_modeset_unlock(). This unlinks the lock from the
context and fully unlocks the WW mutex, stripping the lock from
drm_bridge_helper_reset_crtc().

When we reach this out label, does calling drm_modeset_unlock() again on the
already unlocked mutex trigger a double-unlock warning or crash?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10