Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/nouveau: Assigning ERR_PTR to connector->state causes panic in cleanup - [High] drm/nouveau: Invalid kfree of embedded connector state on non-atomic devices - [Medium] drm/nouveau: MST connectors lose DRM properties on initialization --
commit 5c80f0bbb55c00fa7eb36613e602205469912331 Author: Maxime Ripard <[email protected]> drm/nouveau: Convert to atomic_create_state The nouveau connector reset implementation creates a custom state subclass, but only initializes a pristine state and sets driver-specific defaults without resetting any hardware. This is equivalent to what atomic_create_state expects. Convert to it. > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c > b/drivers/gpu/drm/nouveau/dispnv50/disp.c > index e91130f93db67..39a23d0e9be56 100644 > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c [ ... ] > @@ -1281,8 +1281,11 @@ 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); > - nouveau_conn_attach_properties(&mstc->connector); > + mstc->connector.state = > mstc->connector.funcs->atomic_create_state(&mstc->connector); [Severity: Medium] Will dropping this call to nouveau_conn_attach_properties() cause MST connectors to silently lose access to their DRM properties? Before this patch, MST connectors would get properties like underscan_property, scaling_mode_property, dithering_mode, and vibrant_hue registered here. Without this call, userspace compositors might lose access to these standard digital output properties. [Severity: High] If atomic_create_state() returns an error pointer, this assigns it directly to mstc->connector.state. > + if (IS_ERR(mstc->connector.state)) { > + drm_connector_cleanup(&mstc->connector); Could passing this error pointer to drm_connector_cleanup() result in a kernel panic? If we take this error path, drm_connector_cleanup() checks if connector->state is set (which is true for ERR_PTR), and calls atomic_destroy_state(). This passes the error pointer down to nouveau_conn_atomic_destroy_state(), which will dereference the invalid pointer and panic. > + return PTR_ERR(mstc->connector.state); > + } > > drm_for_each_crtc(crtc, dev) { > if (!(mstm->outp->dcb->heads & drm_crtc_mask(crtc))) > diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c > b/drivers/gpu/drm/nouveau/nouveau_connector.c > index cc3caf8de64b0..59c8091c67967 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; > } [Severity: High] On older non-atomic hardware (pre-NV50), this returns a pointer to the statically embedded nv_connector->properties_state. The caller nouveau_connector_create() now unconditionally assigns this returned pointer to connector->state. When the connector is later destroyed or reset, drm_connector_cleanup() will invoke nouveau_conn_atomic_destroy_state(), which unconditionally calls kfree() on the state pointer. Can calling kfree() on this statically embedded struct pointer cause memory corruption and a kernel panic? [ ... ] > @@ -1521,7 +1519,12 @@ nouveau_connector_create(struct drm_device *dev, int > index) > } > } > > - connector->funcs->reset(connector); > + connector->state = connector->funcs->atomic_create_state(connector); > + if (IS_ERR(connector->state)) { > + ret = PTR_ERR(connector->state); > + goto drm_conn_err; [Severity: High] Similar to the issue in nv50_mstc_new(), if atomic_create_state() fails, the error pointer is assigned to connector->state. The goto drm_conn_err path calls drm_connector_cleanup(), which will attempt to clean up the state using the error pointer. Does this lead to a panic during the error path cleanup? > + } > + > nouveau_conn_attach_properties(connector); > > /* Default scaling mode */ -- Sashiko AI review · https://sashiko.dev/#/patchset/20261014-drm-no-more-connector-reset-v1-0-586c9c026...@kernel.org?part=70
