diff mbox series

[v11,15/74] drm/bridge: Remove redundant error check in drm_bridge_helper_reset_crtc()

Message ID 20260901-dw-hdmi-qp-scramb-v11-15-bc12954a0688@collabora.com (mailing list archive)
State New
Headers
Series Add HDMI 2.0 support to DW HDMI QP TX |

Commit Message

Cristian Ciocaltea Sept. 1, 2026, 6:50 p.m. UTC 
Remove the no-op error check after drm_atomic_helper_reset_crtc() since
the goto target is the immediately following label and the return value
is already propagated correctly without it.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Tested-by: Maud Spierings <maud_spierings@hotmail.com>
Tested-by: Diederik de Haas <diederik@cknow-tech.com>  # NanoPC-T6 LTS, Rock 5B
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/drm_bridge_helper.c | 2 --
 1 file changed, 2 deletions(-)

Comments

sashiko-bot@kernel.org Sept. 1, 2026, 7:21 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] drm_atomic_get_connector_for_encoder returns a connector pointer without holding a reference, leading to a Use-After-Free.
- [High] Double-unlock of connection_mutex due to nested drm_modeset_unlock calls.
--

commit d2f773b296aa81ea66367361aef51be8375a8cce
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
drm/bridge: Remove redundant error check in drm_bridge_helper_reset_crtc()

This commit removes a redundant error check after calling
drm_atomic_helper_reset_crtc() in drm_bridge_helper_reset_crtc(), as the
error path and successful path both immediately fall through to the same
unlock label.

> 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: Critical]
This is a pre-existing issue, but does drm_atomic_get_connector_for_encoder()
return the connector pointer without holding a reference to it?

When drm_atomic_get_connector_for_encoder() iterates over connectors, it calls
drm_connector_list_iter_end() before returning, which explicitly drops the
reference to the iterator's current connector. The pointer returned to this
function is then dereferenced here when accessing connector->state->crtc.

Could a concurrent connector hot-unplug event (such as an MST display
disconnect) trigger a use-after-free before this dereference?

>  	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 is it possible for this unlock
to cause a double-unlock of connection_mutex?

drm_bridge_helper_reset_crtc() acquires connection_mutex via its ctx. It then
calls drm_atomic_get_connector_for_encoder(), which attempts to acquire the
same mutex with the same ctx. This succeeds immediately with -EALREADY, but
before returning, drm_atomic_get_connector_for_encoder() unconditionally calls
drm_modeset_unlock(). 

Since the lock is dropped in the helper, won't this call to
drm_modeset_unlock() result in a double-unlock, as well as potential data
races for the code executing between the two unlocks?

>  	return ret;
>  }
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_bridge_helper.c b/drivers/gpu/drm/drm_bridge_helper.c
index 420f29cf3e54..0a3c8fee66b3 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;
 	ret = drm_atomic_helper_reset_crtc(crtc, ctx);
-	if (ret)
-		goto out;
 
 out:
 	drm_modeset_unlock(&dev->mode_config.connection_mutex);