Re: [PATCH v4 01/40] drm/gpuvm: Don't require obj lock in destructor path

[Danilo Krummrich <dakr@kernel.org>]

On Thu, May 15, 2025 at 10:54:27AM +0200, Danilo Krummrich wrote:
> Hi Rob,
> 
> Can you please CC me on patches for GPUVM?
> 
> On Wed, May 14, 2025 at 10:53:15AM -0700, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> > 
> > See commit a414fe3a2129 ("drm/msm/gem: Drop obj lock in
> > msm_gem_free_object()") for justification.
> 
> Please write a proper commit message that explains the problem and the solution.
> Please don't just refer to another commit and leave it to the reviewer of the
> patch to figure this out.
> 
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >  drivers/gpu/drm/drm_gpuvm.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> 
> > diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> > index f9eb56f24bef..1e89a98caad4 100644
> > --- a/drivers/gpu/drm/drm_gpuvm.c
> > +++ b/drivers/gpu/drm/drm_gpuvm.c
> > @@ -1511,7 +1511,9 @@ drm_gpuvm_bo_destroy(struct kref *kref)
> >  	drm_gpuvm_bo_list_del(vm_bo, extobj, lock);
> >  	drm_gpuvm_bo_list_del(vm_bo, evict, lock);
> >  
> > -	drm_gem_gpuva_assert_lock_held(obj);
> > +	if (kref_read(&obj->refcount) > 0)
> > +		drm_gem_gpuva_assert_lock_held(obj);
> > +
> >  	list_del(&vm_bo->list.entry.gem);
> 
> This seems wrong.
> 
> A VM_BO object keeps a reference of the underlying GEM object, so this should
> never happen.
> 
> This function calls drm_gem_object_put() before it returns.

I noticed your subsequent patch that allows VM_BO structures to have weak
references to GEM objects.

However, even with that this seems wrong. If the reference count of the GEM
object is zero when drm_gpuvm_bo_destroy() is called it means that the GEM
object is dead. However, until drm_gpuvm_bo_destroy() is called the GEM object
potentially remains to be on the extobj and eviced list, which means that other
code paths might fetch it from those lists and consider it to be a valid GEM
object.