diff mbox series

[v11,14/74] drm/bridge: Fix unlocked list access in drm_bridge_attach()

Message ID 20260901-dw-hdmi-qp-scramb-v11-14-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 
drm_bridge_attach() inspects bridge->list to warn about bridges being
attached without a previous drm_bridge_add() call, but it does so
without holding bridge_lock.

The list head can be concurrently written by drm_bridge_add(),
drm_bridge_remove() and __drm_bridge_free() while they operate on
neighbouring entries of bridge_list or bridge_lingering_list, all of
them under bridge_lock.  Hence the unlocked read is a data race, even
though it is unlikely to change the outcome of list_empty() in practice.

Perform the check with bridge_lock held.

Fixes: 76f1a9711b83 ("drm/bridge: add warning for bridges attached without being added")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/drm_bridge.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

sashiko-bot@kernel.org Sept. 1, 2026, 7:16 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Mixing goto-based error handling with scope-based cleanup helpers (`scoped_guard`) in the same function.
--

commit a312843839f1a20e85a81d94af26656e4dacb18f
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

drm/bridge: Fix unlocked list access in drm_bridge_attach()

This patch fixes an unlocked read of bridge->list in drm_bridge_attach()
by performing the check under bridge_lock. It uses the scoped_guard()
cleanup helper to manage the lock.

> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index a8b6df5c13eaf..10dde3600c70a 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -606,8 +606,10 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  	if (!bridge->container)
>  		DRM_WARN("DRM bridge corrupted or not allocated by devm_drm_bridge_alloc()\n");
>  
> -	if (list_empty(&bridge->list))
> -		DRM_WARN("Missing drm_bridge_add() before attach\n");
> +	scoped_guard(mutex, &bridge_lock) {
> +		if (list_empty(&bridge->list))
> +			DRM_WARN("Missing drm_bridge_add() before attach\n");
> +	}

[Severity: Low]
Does introducing scoped_guard() here conflict with the existing goto-based
error handling later in drm_bridge_attach()?

The kernel's cleanup.h guidelines forbid mixing goto-based cleanup and
scope-based cleanup helpers in the same function. drm_bridge_attach()
currently uses goto labels for resource unwinding:

    if (bridge->funcs->attach) {
        ret = bridge->funcs->attach(bridge, encoder, flags);
        if (ret < 0)
            goto err_reset_bridge;
    }

While this specific change doesn't cause a direct runtime leak since the
goto statements don't jump into or out of the scoped_guard block scope,
could this structural mixing cause issues later? Would it be safer to use
standard locking (mutex_lock and mutex_unlock) here to avoid mixing the
two paradigms?

>  
>  	drm_bridge_get(bridge);
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index a8b6df5c13ea..10dde3600c70 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -606,8 +606,10 @@  int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 	if (!bridge->container)
 		DRM_WARN("DRM bridge corrupted or not allocated by devm_drm_bridge_alloc()\n");
 
-	if (list_empty(&bridge->list))
-		DRM_WARN("Missing drm_bridge_add() before attach\n");
+	scoped_guard(mutex, &bridge_lock) {
+		if (list_empty(&bridge->list))
+			DRM_WARN("Missing drm_bridge_add() before attach\n");
+	}
 
 	drm_bridge_get(bridge);