Re: [PATCH v6 01/28] drm/atomic: Fix private obj error handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm: double list_del() on private obj init failure
--

commit 4bcc9dc639c9860732e1906f0390e6fdeeb98b4e
Author: Maxime Ripard <mripard@kernel.org>

drm/atomic: Fix private obj error handling

drm_atomic_private_obj_init() adds the object to the privobj_list before
calling atomic_create_state(). If atomic_create_state() fails, this patch
adds an error path that removes the object from the list to avoid a dangling
entry.

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 9bb761eda59f4..9973becb75aee 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1029,6 +1029,8 @@ int drm_atomic_private_obj_init(struct drm_device *dev,
>  				const struct drm_private_state_funcs *funcs)
>  {
>  	struct drm_private_state *state;
> +	int ret;
> +
>  	memset(obj, 0, sizeof(*obj));
>  
>  	drm_modeset_lock_init(&obj->lock);
> @@ -1038,12 +1040,19 @@ int drm_atomic_private_obj_init(struct drm_device *dev,
>  	list_add_tail(&obj->head, &dev->mode_config.privobj_list);
>  
>  	state = obj->funcs->atomic_create_state(obj);
> -	if (IS_ERR(state))
> -		return PTR_ERR(state);
> +	if (IS_ERR(state)) {
> +		ret = PTR_ERR(state);
> +		goto err_list_del;
> +	}
>  
>  	obj->state = state;
>  
>  	return 0;
> +
> +err_list_del:
> +	list_del(&obj->head);

[Severity: High]
Does this error path introduce a double list_del() panic during driver
teardown?

Several callers of drm_atomic_private_obj_init(), such as vc4_ctm_obj_init()
and drm_dp_mst_topology_mgr_init(), ignore its return value. They
unconditionally register their teardown functions, which perform their own
list_del(&obj->head) via drm_atomic_private_obj_fini().

For instance, in drivers/gpu/drm/vc4/vc4_kms.c:vc4_ctm_obj_init():

	drm_atomic_private_obj_init(&vc4->base, &vc4->ctm_manager,
				    &vc4_ctm_state_funcs);

	return drmm_add_action_or_reset(&vc4->base, vc4_ctm_obj_fini, NULL);

If atomic_create_state() fails here, we delete the list head and return an
error, but the caller ignores it. Then, at teardown,
drm_atomic_private_obj_fini() executes a second list_del(&obj->head) on the
poisoned list head, causing a panic.

Will this unwinding cause crashes for these existing callers if initialization
fails?

> +	drm_modeset_lock_fini(&obj->lock);
> +	return ret;
>  }
>  EXPORT_SYMBOL(drm_atomic_private_obj_init);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-drm-state-readout-v6-0-6cc02cd10998@kernel.org?part=1