Re: [PATCH v10 8/9] drm/atomic-uapi: Add DRM_MODE_ATOMIC_RESET flag

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/atomic: lease revocation race in
  drm_atomic_commit_fill_with_defaults()
--

commit b8c56650ab4657eb654cfbc63cf4622958dcbfc9
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 the
DRM_CAP_ATOMIC_RESET capability to reset all KMS object states to their
defaults before applying the properties supplied in an atomic commit.

> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 1eebcf6f5e05b..fe6d8cc533bc9 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
[ ... ]
> @@ -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: Medium]
Can concurrent lease revocation during drm_atomic_commit_fill_with_defaults()
populate a partial pipeline into state and cause a NULL pointer dereference in
crtc_or_fake_commit()?

Each drm_lease_held() call in drm_atomic_commit_fill_with_defaults() acquires
and drops dev->mode_config.idr_mutex on its own, while _drm_lease_revoke()
empties master->leases under idr_mutex without taking modeset locks.  If a
lessor revokes the lease between the plane loop and the CRTC loop, an active
plane is added to the commit with a reset state (new_plane_state->crtc == NULL,
old_plane_state->crtc == crtc) without adding its CRTC:

CPU0 (lessee)                               CPU1 (lessor)
drm_mode_atomic_ioctl()
  drm_atomic_commit_fill_with_defaults()
    drm_for_each_plane()
      drm_lease_held(..., plane) -> true
      drm_lease_held(..., crtc) -> true
      drm_atomic_commit_set_plane_state()
                                            drm_mode_revoke_lease_ioctl()
                                              _drm_lease_revoke()
                                                idr_destroy(&master->leases)
    drm_for_each_crtc()
      drm_lease_held(..., crtc) -> false
    drm_for_each_connector_iter()
      drm_lease_held(...) -> false
  drm_atomic_commit()
    ...
      drm_atomic_helper_plane_changed()
        crtc_state = drm_atomic_get_new_crtc_state(state,
                                                   old_plane_state->crtc)
        WARN_ON(!crtc_state)
      ...
      drm_atomic_helper_setup_commit()
        crtc_or_fake_commit(state, old_plane_state->crtc)
          new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc)
          new_crtc_state->commit  <-- NULL pointer dereference

Unlike drm_atomic_get_plane_state(), the plane loop in
drm_atomic_commit_fill_with_defaults() relies on the subsequent CRTC loop to
add old_plane_state->crtc to the commit, and the connector loop's -ENOENT
partial-pipeline guard is also bypassed once drm_lease_held() returns false for
the CRTC.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-drm-reset-state-flag-v10-0-cefaa1ed056d@kernel.org?part=8