Re: [PATCH RFC v2 9/9] drm/atomic-uapi: Add DRM_MODE_ATOMIC_RESET flag

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm_atomic_commit_fill_with_defaults() accesses colorop state without acquiring the required plane mutex, causing a lock assertion failure and data race.
--

--- Patch [9]: [PATCH RFC v2 9/9] drm/atomic-uapi: Add DRM_MODE_ATOMIC_RESET flag ---
commit 8b754afef124bddf6efaa487526713fc14275d85
Author: Sebastian Wick <[email protected]>
Subject: drm/atomic-uapi: Add DRM_MODE_ATOMIC_RESET flag

This commit introduces the DRM_MODE_ATOMIC_RESET flag for the atomic ioctl.
It allows userspace to reset all KMS object states to their defaults before
applying property changes.

> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 1050dddadb17e..cdcc6efb9a00f 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1675,6 +1682,12 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  	fence_state = NULL;
>  	num_fences = 0;
>  
> +	if (arg->flags & DRM_MODE_ATOMIC_RESET) {
> +		ret = drm_atomic_commit_fill_with_defaults(state);

[Severity: High]
Does calling drm_atomic_commit_fill_with_defaults() here trigger a lock
assertion failure and potential data race for colorops?

Looking at drm_atomic_commit_fill_with_defaults() in drm_atomic.c, the loop
iterating over colorops asserts that the plane's mutex is held, but never
acquires it:

    drm_for_each_colorop(colorop, dev) {
        ...
        drm_modeset_lock_assert_held(&colorop->plane->mutex);
        ret = drm_atomic_commit_set_colorop_state(commit, colorop,
                                                  colorop_state);
        ...
    }

The subsequent loops for planes, CRTCs, and connectors in the same function
correctly call drm_modeset_lock() before operating on their respective
objects.

Since the plane mutex is neither held prior to calling
drm_atomic_commit_fill_with_defaults() nor acquired inside the colorop loop,
could this lead to state corruption or a use-after-free during concurrent
atomic commits?

> +		if (ret)
> +			goto out;
> +	}
> +
>  	for (i = 0; i < arg->count_objs; i++) {
>  		uint32_t obj_id, count_props;
>  		struct drm_mode_object *obj;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9