Re: [PATCH v7 14/16] drm/atomic-uapi: Add DRM_MODE_ATOMIC_RESET flag

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/atomic: zeroed adjusted_mode on DRM_MODE_ATOMIC_RESET without modeset
- [High] drm/atomic: cross-lease state corruption and events on DRM_MODE_ATOMIC_RESET
--

commit c819cce4a5454d4a632c82a26b77df3ff48e4432
Author: Sebastian Wick <sebastian.wick@redhat.com>

drm/atomic-uapi: Add DRM_MODE_ATOMIC_RESET flag

This commit introduces the DRM_MODE_ATOMIC_RESET flag and DRM_CAP_ATOMIC_RESET
capability so userspace can reset all KMS object states to their defaults
before applying the properties in an atomic commit. It also updates
prepare_signaling() to avoid attaching page flip events to CRTCs that remain
disabled across a reset commit.

> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 1eebcf6f5e05b..77d0713c7e2d6 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1440,10 +1440,29 @@ static int prepare_signaling(struct drm_device *dev,
>  
>  	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>  		s32 __user *fence_ptr;
> +		bool page_flip_event;
>  
>  		fence_ptr = get_out_fence_for_crtc(crtc_state->state, crtc);
>  
> -		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) {
> +		page_flip_event = arg->flags & DRM_MODE_PAGE_FLIP_EVENT;
> +
> +		/*
> +		 * A commit with the DRM_MODE_ATOMIC_RESET flag contains
> +		 * every CRTC of the device or lease, even the ones that are
> +		 * disabled before and after the commit. Those can't report
> +		 * a page flip, and drm_atomic_crtc_check() would reject the
> +		 * whole commit, so don't attach any event to them.
> +		 */
> +		if (page_flip_event && !fence_ptr &&
> +		    (arg->flags & DRM_MODE_ATOMIC_RESET) &&
> +		    !crtc_state->active) {
> +			struct drm_crtc_state *old_crtc_state;
> +
> +			old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
> +			page_flip_event = old_crtc_state->active;
> +		}

[Severity: High]
As reported by Sashiko on v6 [1], what happens when a DRM lessee calls
drm_mode_atomic_ioctl() with DRM_MODE_ATOMIC_RESET while KMS objects are
bound across the lease boundary?

In drm_atomic_commit_fill_with_defaults(), when a leased plane or leased
connector is currently bound to an unleased CRTC, the unleased CRTC is pulled
into the commit with its current state duplicated:

drivers/gpu/drm/drm_atomic.c:drm_atomic_commit_fill_with_defaults() {
    ...
		if (plane->state->crtc) {
			struct drm_crtc_state *crtc_state;

			crtc_state = drm_atomic_get_crtc_state(commit,
							       plane->state->crtc);
			if (IS_ERR(crtc_state))
				return PTR_ERR(crtc_state);
		}

		plane_state = plane->funcs->atomic_create_state(plane);
    ...
}

Because prepare_signaling() iterates over all CRTCs in state without checking
drm_lease_held(file_priv, crtc->base.id), any active unleased CRTC pulled into
the commit has crtc_state->active set to true and will have a page flip event
attached for the lessee's file_priv and increment c. Doesn't this leak vblank
events for an unleased CRTC to the lessee, allow a commit that leaves all of
the lessee's own CRTCs disabled to bypass the c == 0 check, and risk failing
with -EBUSY in stall_checks() if the lessor has a flip in flight on that CRTC?

In addition, when detaching a leased plane or connector from an unleased CRTC
via atomic_create_state(), drm_atomic_commit_fill_with_defaults() does not
clear drm_plane_mask(plane) from crtc_state->plane_mask or
drm_connector_mask(connector) from crtc_state->connector_mask on the
duplicated unleased CRTC state. Conversely, if an unleased connector or plane
is bound to a leased CRTC that gets reset, the leased CRTC's connector_mask
and plane_mask are zeroed while the unleased connector or plane state still
points to that CRTC:

drivers/gpu/drm/drm_atomic.c:drm_atomic_commit_fill_with_defaults() {
    ...
		if (!drm_lease_held(file_priv, connector->base.id)) {
			if (!connector->state->crtc)
				continue;

			if (!drm_atomic_get_new_crtc_state(commit,
							   connector->state->crtc))
				continue;

			connector_state = drm_atomic_get_connector_state(commit,
									 connector);
    ...
}

Won't these cross-lease bindings corrupt the CRTC state masks or cause
drm_atomic_helper_check_modeset() and drm_atomic_helper_check_planes() to
reject the lessee's reset commit with -EINVAL?

[ ... ]

> @@ -1688,6 +1715,17 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  	fence_state = NULL;
>  	num_fences = 0;
>  
> +	if (arg->flags & DRM_MODE_ATOMIC_RESET) {
> +		if (!drm_atomic_implements_create_state(dev)) {
> +			ret = -EOPNOTSUPP;
> +			goto out;
> +		}
> +
> +		ret = drm_atomic_commit_fill_with_defaults(file_priv, state);
> +		if (ret)
> +			goto out;
> +	}

[Severity: High]
When userspace issues a commit with DRM_MODE_ATOMIC_RESET that re-applies the
same mode and connector routing on an already-enabled CRTC, does
new_crtc_state->adjusted_mode get left all-zeroed?

In drm_atomic_commit_fill_with_defaults(), the fresh CRTC state is allocated
via crtc->funcs->atomic_create_state(crtc) with adjusted_mode zeroed, while
connector_state->best_encoder is preserved from connector->state->best_encoder
so that re-applying the current configuration avoids a modeset:

drivers/gpu/drm/drm_atomic.c:drm_atomic_commit_fill_with_defaults() {
    ...
		crtc_state = crtc->funcs->atomic_create_state(crtc);
    ...
		connector_state->best_encoder = connector->state->best_encoder;
    ...
}

When userspace supplies the existing MODE_ID, ACTIVE = 1, and CRTC_ID
properties, new_crtc_state->mode and new_crtc_state->enable match
old_crtc_state, and update_connector_routing() sees new_encoder ==
new_connector_state->best_encoder, leaving both mode_changed and
connectors_changed false. In drm_atomic_helper_check_modeset() -> mode_fixup(),
copying mode into adjusted_mode is then skipped:

drivers/gpu/drm/drm_atomic_helper.c:mode_fixup() {
    ...
	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
		if (!new_crtc_state->mode_changed &&
		    !new_crtc_state->connectors_changed)
			continue;

		drm_mode_copy(&new_crtc_state->adjusted_mode, &new_crtc_state->mode);
	}
    ...
}

With new_crtc_state->adjusted_mode remaining all zeros on an enabled CRTC,
won't subsequent checks and commit steps fail or crash—for example, dividing
by zero on mode->htotal in vc4_crtc_atomic_check(), rejecting valid plane
coordinates against adjusted_mode.hdisplay in omap_plane_atomic_check(), and
corrupting vblank timing constants in
drm_atomic_helper_calc_timestamping_constants()?

[1] https://lore.kernel.org/r/20261022-drm-reset-state-flag-v6-13-f36d640b7e76@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-drm-reset-state-flag-v7-0-4e55eaeae683@kernel.org?part=14