Re: [PATCH v7 04/34] 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:
- [Medium] drm/atomic: non-idempotent list_del() in drm_atomic_private_obj_fini()
--

commit 72c7eb0713fd45624f9e5b2b360640bc1c381470
Author: Maxime Ripard <mripard@kernel.org>

drm/atomic: Fix private obj error handling

This commit fixes drm_atomic_private_obj_init() to remove the object from
privobj_list and clean up the modeset lock if atomic_create_state() fails. It
also adds a list_empty() check to drm_atomic_private_obj_fini() to avoid a
double list_del() when teardown runs after a failed initialization.

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 9bb761eda59f4..8f9b17395b905 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c

[ ... ]

> @@ -1056,6 +1072,15 @@ EXPORT_SYMBOL(drm_atomic_private_obj_init);
>  void
>  drm_atomic_private_obj_fini(struct drm_private_obj *obj)
>  {
> +	/*
> +	 * Some callers ignore drm_atomic_private_obj_init() failures
> +	 * and unconditionally register a teardown action that calls
> +	 * this function. Make it idempotent so a failed init followed
> +	 * by fini does not cause a double list_del.
> +	 */
> +	if (list_empty(&obj->head))
> +		return;
> +
>  	list_del(&obj->head);

[Severity: Medium]
Should drm_atomic_private_obj_fini() in drivers/gpu/drm/drm_atomic.c use
list_del_init(&obj->head) instead of list_del(&obj->head) so that calling
drm_atomic_private_obj_fini() more than once is truly idempotent?

Because list_del(&obj->head) poisons obj->head.next with LIST_POISON1, a
second call to drm_atomic_private_obj_fini() after a successful
drm_atomic_private_obj_init() will see list_empty(&obj->head) evaluate to
false. It will then execute list_del(&obj->head) on the poisoned list node and
call obj->funcs->atomic_destroy_state(obj, obj->state) a second time on
already-freed state (as well as kfree(obj->name) in the subsequent commit
adding drm_private_obj names).

In drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c, this happens when
dpu_kms_hw_init() succeeds in dpu_kms_global_obj_init() ->
drm_atomic_private_obj_init() and then fails at a later initialization step:

dpu_kms_hw_init()
  error:
    _dpu_kms_hw_destroy(dpu_kms)
      dpu_kms_global_obj_fini(dpu_kms)
        drm_atomic_private_obj_fini(&dpu_kms->global_state)

When the error propagates back to msm_drm_init() in
drivers/gpu/drm/msm/msm_drv.c, teardown calls _dpu_kms_hw_destroy() a second
time:

msm_drm_init()
  msm_drm_uninit()
    msm_drm_kms_uninit()
      dpu_kms_destroy()
        _dpu_kms_hw_destroy(dpu_kms)
          dpu_kms_global_obj_fini(dpu_kms)
            drm_atomic_private_obj_fini(&dpu_kms->global_state)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-drm-state-readout-v7-0-25e5163d75c3@kernel.org?part=4