Re: [PATCH] drm/exec: don't immediately add prelocked obj to array of locked objs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/exec: silent unlock of shared dma_resv when ignoring duplicates
--

commit ffbebd24a9a712b1b60d1b56c904b2fe3d32a8d5
Author: Christian König <ckoenig.leichtzumerken@gmail.com>

drm/exec: don't immediately add prelocked obj to array of locked objs

This commit defers adding a prelocked object to the array of locked objects
until the driver explicitly requests it again. This prevents holding the last
reference to objects that are about to be destroyed if they are never
requested again in a dynamic locking loop.

> diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c
> index 2453ec41360f4..a910b0ad17ce4 100644
> --- a/drivers/gpu/drm/drm_exec.c
> +++ b/drivers/gpu/drm/drm_exec.c
> @@ -118,6 +127,14 @@ EXPORT_SYMBOL(drm_exec_fini);
>  bool drm_exec_cleanup(struct drm_exec *exec)
>  {
>  	if (likely(!exec->contended)) {
> +
> +		/*
> +		 * The contended and prelocked object might never given to
> +		 * drm_exec_lock_obj() again. This can happen when the object
> +		 * is about to be cleaned up and we now hold the last reference
> +		 * to it. Make sure that we don't unecessarily keep it around.
> +		 */
> +		drm_exec_drop_prelocked(exec);

[Severity: High]
Could this unconditionally unlock a shared dma_resv that was successfully
locked for another object?

If the locking loop is dynamic and object B becomes prelocked due to
contention, its dma_resv is locked. During the retry, if the driver requests
object A (which shares the same dma_resv as B) but does not request B again,
this will unlock B's dma_resv at the end of the loop, which also unlocks A.

[ ... ]

> @@ -212,35 +229,31 @@ int drm_exec_lock_obj(struct drm_exec *exec, struct drm_gem_object *obj)
>  	if (exec->prelocked == obj) {
>  		drm_gem_object_put(exec->prelocked);
>  		exec->prelocked = NULL;
> -		return 0;
> -	}
> +	} else {
> +		if (exec->flags & DRM_EXEC_INTERRUPTIBLE_WAIT)
> +			ret = dma_resv_lock_interruptible(obj->resv,
> +							  &exec->ticket);
> +		else
> +			ret = dma_resv_lock(obj->resv, &exec->ticket);
> +
> +		if (unlikely(ret == -EDEADLK)) {
> +			drm_gem_object_get(obj);
> +			exec->contended = obj;
> +			return -EDEADLK;
> +		}
>  
> -	if (exec->flags & DRM_EXEC_INTERRUPTIBLE_WAIT)
> -		ret = dma_resv_lock_interruptible(obj->resv, &exec->ticket);
> -	else
> -		ret = dma_resv_lock(obj->resv, &exec->ticket);
> +		if (unlikely(ret == -EALREADY) &&
> +		    exec->flags & DRM_EXEC_IGNORE_DUPLICATES)
> +			return 0;

[Severity: High]
Does this skip adding object A to the locked objects array if its shared
dma_resv is already locked by the prelocked object B?

If DRM_EXEC_IGNORE_DUPLICATES is set, dma_resv_lock(A->resv) returns -EALREADY
because B (the prelocked object) holds the lock. This early return of 0
implies A is safely locked, but A is never added to exec->objects.

Combined with drm_exec_drop_prelocked() in drm_exec_cleanup(), B's dma_resv
will be unlocked. The driver will then proceed with its operations believing A
is locked, but its dma_resv has been silently unlocked. Can this lead to data
races or use-after-free for A?

> -	if (unlikely(ret == -EDEADLK)) {
> -		drm_gem_object_get(obj);
> -		exec->contended = obj;
> -		return -EDEADLK;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007124657.9664-1-christian.koenig@amd.com?part=1