Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] drm/nouveau: invalid free via nouveau_conn_atomic_create_state() on pre-NV50 - [High] drm/nouveau: missing nvif teardown on error in nouveau_connector_create() - [Medium] drm/nouveau: memory leak of mstc on error in nv50_mstc_new() --
commit 37ea1f2bbde92fcd5731b31085bba035feef8a42 Author: Maxime Ripard <[email protected]> drm/nouveau: Convert to atomic_create_state This commit converts the nouveau connector implementation from using the reset callback to using the atomic_create_state callback for initializing connector state. > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c > b/drivers/gpu/drm/nouveau/dispnv50/disp.c > index e91130f93db67..947d0ff4ba029 100644 > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c [ ... ] > @@ -1281,7 +1282,13 @@ nv50_mstc_new(struct nv50_mstm *mstm, struct > drm_dp_mst_port *port, > > drm_connector_helper_add(&mstc->connector, &nv50_mstc_help); > > - mstc->connector.funcs->reset(&mstc->connector); > + conn_state = > mstc->connector.funcs->atomic_create_state(&mstc->connector); > + if (IS_ERR(conn_state)) { > + drm_connector_cleanup(&mstc->connector); > + return PTR_ERR(conn_state); > + } [Severity: Medium] Earlier in nv50_mstc_new(), mstc and *pmstc are allocated with kzalloc_obj(*mstc), and the error path for drm_connector_dynamic_init() frees *pmstc and resets it to NULL: if (!(mstc = *pmstc = kzalloc_obj(*mstc))) return -ENOMEM; mstc->mstm = mstm; mstc->port = port; ret = drm_connector_dynamic_init(dev, &mstc->connector, &nv50_mstc, DRM_MODE_CONNECTOR_DisplayPort, NULL); if (ret) { kfree(*pmstc); *pmstc = NULL; return ret; } When atomic_create_state() fails here, drm_connector_cleanup() only cleans up the embedded drm_connector and zeroes mstc->connector, while the caller nv50_mstm_add_connector() discards mstc on a non-zero return. Does this error path leak mstc and leave a dangling pointer in *pmstc? > + mstc->connector.state = conn_state; > + > nouveau_conn_attach_properties(&mstc->connector); > > drm_for_each_crtc(crtc, dev) { > diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c > b/drivers/gpu/drm/nouveau/nouveau_connector.c > index cc3caf8de64b0..e4ab9849de4d4 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_connector.c > +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c > @@ -242,21 +242,17 @@ nouveau_conn_atomic_duplicate_state(struct > drm_connector *connector) > return &asyc->state; > } > > -void > -nouveau_conn_reset(struct drm_connector *connector) > +struct drm_connector_state * > +nouveau_conn_atomic_create_state(struct drm_connector *connector) > { > struct nouveau_connector *nv_connector = nouveau_connector(connector); > struct nouveau_conn_atom *asyc; > > if (drm_drv_uses_atomic_modeset(connector->dev)) { > if (WARN_ON(!(asyc = kzalloc_obj(*asyc)))) > - return; > + return ERR_PTR(-ENOMEM); > > - if (connector->state) > - nouveau_conn_atomic_destroy_state(connector, > - connector->state); > - > - __drm_atomic_helper_connector_reset(connector, &asyc->state); > + __drm_atomic_helper_connector_state_init(&asyc->state, > connector); > } else { > asyc = &nv_connector->properties_state; > } > @@ -278,6 +274,8 @@ nouveau_conn_reset(struct drm_connector *connector) > break; > } > } > + > + return &asyc->state; > } [Severity: Critical] On pre-nv50 display hardware (where disp->disp.object.oclass < NV50_DISP), nv04_display_create() clears DRIVER_ATOMIC and uses nouveau_mode_config_funcs without an atomic_commit callback, so drm_drv_uses_atomic_modeset(connector->dev) is false. Previously, nouveau_conn_reset() only called __drm_atomic_helper_connector_reset() in the atomic branch, leaving connector->state as NULL on pre-nv50 while initializing the embedded nv_connector->properties_state in place. With this change, nouveau_conn_atomic_create_state() returns &nv_connector->properties_state.state, and nouveau_connector_create() unconditionally assigns it to connector->state. When nouveau_display_create() subsequently calls drm_mode_config_reset(), drm_mode_config_connector_reset_with_create_state() sees the non-NULL connector->state and passes it to nouveau_conn_atomic_destroy_state(): drivers/gpu/drm/nouveau/nouveau_connector.c:nouveau_conn_atomic_destroy_state() { struct nouveau_conn_atom *asyc = nouveau_conn_atom(state); __drm_atomic_helper_connector_destroy_state(&asyc->state); kfree(asyc); } The same destruction happens in drm_connector_cleanup(), and having connector->state non-NULL on a non-atomic driver also causes helpers such as drm_connector_get_encoder() to inspect connector->state->best_encoder instead of connector->encoder. Won't returning &nv_connector->properties_state.state and assigning it to connector->state on pre-nv50 hardware pass an interior pointer of struct nouveau_connector to kfree() and break legacy encoder lookups? [ ... ] > @@ -1521,7 +1520,13 @@ nouveau_connector_create(struct drm_device *dev, int > index) > } > } > > - connector->funcs->reset(connector); > + conn_state = connector->funcs->atomic_create_state(connector); > + if (IS_ERR(conn_state)) { > + ret = PTR_ERR(conn_state); > + goto drm_conn_err; > + } [Severity: High] Before reaching atomic_create_state() in nouveau_connector_create(), nv_connector->conn, nv_connector->hpd, and nv_connector->irq may already be constructed via nvif_conn_ctor() and nvif_conn_event_ctor(): if (nvif_object_constructed(&nv_connector->conn.object)) { ret = nvif_conn_event_ctor(&nv_connector->conn, "kmsHotplug", nouveau_connector_hotplug, NVIF_CONN_EVENT_V0_PLUG | NVIF_CONN_EVENT_V0_UNPLUG, &nv_connector->hpd); if (ret == 0) connector->polled = DRM_CONNECTOR_POLL_HPD; if (nv_connector->aux.transfer) { ret = nvif_conn_event_ctor(&nv_connector->conn, "kmsDpIrq", nouveau_connector_irq, NVIF_CONN_EVENT_V0_IRQ, &nv_connector->irq); if (ret) { nvif_event_dtor(&nv_connector->hpd); nvif_conn_dtor(&nv_connector->conn); goto drm_conn_err; } } } However, the drm_conn_err label only cleans up connector and frees nv_connector: drm_conn_err: drm_connector_cleanup(connector); kfree(nv_connector); return ERR_PTR(ret); If atomic_create_state() fails and jumps to drm_conn_err without calling nvif_event_dtor(&nv_connector->irq), nvif_event_dtor(&nv_connector->hpd), and nvif_conn_dtor(&nv_connector->conn), can this leak those nvif objects and leave active events referencing freed nv_connector memory? > + connector->state = conn_state; > + > nouveau_conn_attach_properties(connector); > > /* Default scaling mode */ [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20261022-drm-no-more-connector-reset-v2-0-0b7e4e85b...@kernel.org?part=56
