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 <[email protected]> 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/[email protected]?part=1
