Re: [PATCH 1/2] drm/amdgpu: make sure BOs are locked in amdgpu_vm_get_memory

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

Am 06.06.23 um 03:11 schrieb Chen, Guchun:
> [Public]
>
> Acked-by: Guchun Chen <guchun.chen@xxxxxxx> for this series.
>
> A simple question is we don't need to hold the lock if bo locations are not changed?

Well we take the look to make sure that BO locations don't change.

Otherwise we potentially access freed memory when looking at the resource.

Regards,
Christian.

> Regards,
> Guchun
>
> > -----Original Message-----
> > From: Christian König <ckoenig.leichtzumerken@xxxxxxxxx>
> > Sent: Monday, June 5, 2023 5:11 PM
> > To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; mikhail.v.gavrilov@xxxxxxxxx; Chen,
> > Guchun <Guchun.Chen@xxxxxxx>
> > Subject: [PATCH 1/2] drm/amdgpu: make sure BOs are locked in
> > amdgpu_vm_get_memory
> >
> > We need to grab the lock of the BO or otherwise can run into a crash when
> > we try to inspect the current location.
> >
> > Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 69 +++++++++++++++---------
> > --
> >   1 file changed, 39 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > index 3c0310576b3b..2c8cafec48a4 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > @@ -920,42 +920,51 @@ int amdgpu_vm_update_range(struct
> > amdgpu_device *adev, struct amdgpu_vm *vm,
> >        return r;
> >   }
> >
> > +static void amdgpu_vm_bo_get_memory(struct amdgpu_bo_va *bo_va,
> > +                                 struct amdgpu_mem_stats *stats) {
> > +     struct amdgpu_vm *vm = bo_va->base.vm;
> > +     struct amdgpu_bo *bo = bo_va->base.bo;
> > +
> > +     if (!bo)
> > +             return;
> > +
> > +     /*
> > +      * For now ignore BOs which are currently locked and potentially
> > +      * changing their location.
> > +      */
> > +     if (bo->tbo.base.resv != vm->root.bo->tbo.base.resv &&
> > +         !dma_resv_trylock(bo->tbo.base.resv))
> > +             return;
> > +
> > +     amdgpu_bo_get_memory(bo, stats);
> > +     if (bo->tbo.base.resv != vm->root.bo->tbo.base.resv)
> > +         dma_resv_unlock(bo->tbo.base.resv);
> > +}
> > +
> >   void amdgpu_vm_get_memory(struct amdgpu_vm *vm,
> >                          struct amdgpu_mem_stats *stats)
> >   {
> >        struct amdgpu_bo_va *bo_va, *tmp;
> >
> >        spin_lock(&vm->status_lock);
> > -     list_for_each_entry_safe(bo_va, tmp, &vm->idle, base.vm_status) {
> > -             if (!bo_va->base.bo)
> > -                     continue;
> > -             amdgpu_bo_get_memory(bo_va->base.bo, stats);
> > -     }
> > -     list_for_each_entry_safe(bo_va, tmp, &vm->evicted, base.vm_status)
> > {
> > -             if (!bo_va->base.bo)
> > -                     continue;
> > -             amdgpu_bo_get_memory(bo_va->base.bo, stats);
> > -     }
> > -     list_for_each_entry_safe(bo_va, tmp, &vm->relocated,
> > base.vm_status) {
> > -             if (!bo_va->base.bo)
> > -                     continue;
> > -             amdgpu_bo_get_memory(bo_va->base.bo, stats);
> > -     }
> > -     list_for_each_entry_safe(bo_va, tmp, &vm->moved, base.vm_status)
> > {
> > -             if (!bo_va->base.bo)
> > -                     continue;
> > -             amdgpu_bo_get_memory(bo_va->base.bo, stats);
> > -     }
> > -     list_for_each_entry_safe(bo_va, tmp, &vm->invalidated,
> > base.vm_status) {
> > -             if (!bo_va->base.bo)
> > -                     continue;
> > -             amdgpu_bo_get_memory(bo_va->base.bo, stats);
> > -     }
> > -     list_for_each_entry_safe(bo_va, tmp, &vm->done, base.vm_status) {
> > -             if (!bo_va->base.bo)
> > -                     continue;
> > -             amdgpu_bo_get_memory(bo_va->base.bo, stats);
> > -     }
> > +     list_for_each_entry_safe(bo_va, tmp, &vm->idle, base.vm_status)
> > +             amdgpu_vm_bo_get_memory(bo_va, stats);
> > +
> > +     list_for_each_entry_safe(bo_va, tmp, &vm->evicted, base.vm_status)
> > +             amdgpu_vm_bo_get_memory(bo_va, stats);
> > +
> > +     list_for_each_entry_safe(bo_va, tmp, &vm->relocated,
> > base.vm_status)
> > +             amdgpu_vm_bo_get_memory(bo_va, stats);
> > +
> > +     list_for_each_entry_safe(bo_va, tmp, &vm->moved, base.vm_status)
> > +             amdgpu_vm_bo_get_memory(bo_va, stats);
> > +
> > +     list_for_each_entry_safe(bo_va, tmp, &vm->invalidated,
> > base.vm_status)
> > +             amdgpu_vm_bo_get_memory(bo_va, stats);
> > +
> > +     list_for_each_entry_safe(bo_va, tmp, &vm->done, base.vm_status)
> > +             amdgpu_vm_bo_get_memory(bo_va, stats);
> >        spin_unlock(&vm->status_lock);
> >   }
> >
> > --
> > 2.34.1