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 <[email protected]> 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/[email protected]?part=1
