Re: [PATCH v2 56/59] drm/nouveau: Convert to atomic_create_state

[sashiko-bot@kernel.org]

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 <mripard@kernel.org>

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-0b7e4e85b1e8@kernel.org?part=56