Re: [PATCH v8 16/39] drm/atomic_sro: Add function to install state into drm objects

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/atomic_sro: bad private_objs loop bound in
  drm_atomic_sro_install_state()
- [Medium] drm/atomic_sro: premature state destruction in
  drm_atomic_sro_install_state()
--

commit 9bc3fe0d50e4075338d8a443036b9d7e790c5ea0
Author: Maxime Ripard <mripard@kernel.org>

drm/atomic_sro: Add function to install state into drm objects

This commit adds drm_atomic_sro_install_state() to install readout states
into DRM objects and introduces the optional atomic_sro_install_state hook
for planes, CRTCs, connectors, and private objects.

> diff --git a/drivers/gpu/drm/drm_atomic_state_readout.c b/drivers/gpu/drm/drm_atomic_state_readout.c
> index 07ddc315b13d9..de0f27186bea9 100644
> --- a/drivers/gpu/drm/drm_atomic_state_readout.c
> +++ b/drivers/gpu/drm/drm_atomic_state_readout.c

[ ... ]

> @@ -581,3 +582,145 @@ void drm_atomic_sro_set_private_obj_state(struct drm_atomic_sro_state *state,
>  		       obj_state, state);
>  }
>  EXPORT_SYMBOL(drm_atomic_sro_set_private_obj_state);
> +
> +/**
> + * drm_atomic_sro_install_state - install readout state into DRM objects
> + * @state: SRO state to install
> + *
> + * Takes a &struct drm_atomic_sro_state built by
> + * &drm_mode_config_helper_funcs.atomic_sro_build_state and installs its
> + * contents as the current state of each DRM object (setting
> + * &drm_crtc.state, &drm_plane.state, &drm_connector.state and
> + * &drm_private_obj.state).
> + *
> + * For each object, the optional atomic_sro_install_state hook is
> + * called before the state pointer is updated, allowing drivers to
> + * perform any needed action.
> + */
> +void drm_atomic_sro_install_state(struct drm_atomic_sro_state *state)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i <= state->max_connector_index; i++) {
> +		const struct drm_connector_funcs *conn_funcs;
> +		struct drm_connector_state *conn_state;
> +		struct drm_connector *conn;
> +
> +		conn = state->connectors[i].ptr;
> +		if (!conn)
> +			continue;
> +
> +		conn_funcs = conn->funcs;
> +		if (!conn_funcs)
> +			continue;
> +
> +		if (conn->state) {
> +			conn_funcs->atomic_destroy_state(conn, conn->state);
> +			conn->state = NULL;
> +		}
> +
> +		conn_state = state->connectors[i].state;
> +		if (!conn_state)
> +			continue;
> +
> +		if (conn_funcs->atomic_sro_install_state)
> +			conn_funcs->atomic_sro_install_state(conn, conn_state);
> +
> +		conn->state = conn_state;
> +		state->connectors[i].state = NULL;
> +		state->connectors[i].ptr = NULL;
> +		drm_connector_put(conn);
> +	}

[Severity: Medium]
Does destroying conn->state (and crtc->state, plane->state, and obj->state
in the loops below) before checking whether the new state pointer in the
SRO container is non-NULL leave the object with a NULL state pointer?

If state->connectors[i].state is NULL, the old state has already been
destroyed and nulled when the loop continues, leaving the KMS object
without a state and causing NULL pointer dereferences in later
atomic_duplicate_state() calls.  It also leaves the object's state pointer
NULL while atomic_sro_install_state() runs.

[ ... ]

> +	for (i = 0; i < count_private_obj(state->dev); i++) {
> +		const struct drm_private_state_funcs *obj_funcs;
> +		struct drm_private_state *obj_state;
> +		struct drm_private_obj *obj;
> +
> +		obj = state->private_objs[i].ptr;
> +		if (!obj)
> +			continue;
> +
> +		obj_funcs = obj->funcs;
> +		if (!obj_funcs)
> +			continue;
> +
> +		if (obj->state) {
> +			obj_funcs->atomic_destroy_state(obj, obj->state);
> +			obj->state = NULL;
> +		}
> +
> +		obj_state = state->private_objs[i].state;
> +		if (!obj_state)
> +			continue;
> +
> +		if (obj_funcs->atomic_sro_install_state)
> +			obj_funcs->atomic_sro_install_state(obj, obj_state);
> +
> +		obj->state = obj_state;
> +		state->private_objs[i].state = NULL;
> +		state->private_objs[i].ptr = NULL;
> +	}
> +	state->num_private_objs = 0;
> +}

[Severity: Medium]
Should this loop bound be state->num_private_objs instead of
count_private_obj(state->dev)?

drm_atomic_sro_state_init() allocates state->private_objs for the count at
allocation time, and drm_atomic_sro_set_private_obj_state() populates up to
state->num_private_objs entries.  Bounding the loop by
count_private_obj(state->dev) re-walks dev->mode_config.privobj_list on
every iteration, scans unpopulated slots, and reads past the end of
state->private_objs if a private object was added after
drm_atomic_sro_state_alloc().  In addition, zeroing state->num_private_objs
at the end prevents drm_atomic_sro_state_free() from freeing any
obj_state skipped by an early continue.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-drm-state-readout-v8-0-cbe78624e1bc@kernel.org?part=16