lkml.org 
[lkml]   [2023]   [Feb]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] drm/amdgpu: Fix potential race processing vm->freed
From
Am 06.02.23 um 19:21 schrieb Rob Clark:
> On Mon, Feb 6, 2023 at 8:05 AM Christian König <christian.koenig@amd.com> wrote:
>> Am 06.02.23 um 16:52 schrieb Rob Clark:
>>> On Mon, Feb 6, 2023 at 2:15 AM Christian König <christian.koenig@amd.com> wrote:
>>>> Am 03.02.23 um 19:10 schrieb Rob Clark:
>>>>> From: Rob Clark <robdclark@chromium.org>
>>>>>
>>>>> If userspace calls the AMDGPU_CS ioctl from multiple threads, because
>>>>> the vm is global to the drm_file, you can end up with multiple threads
>>>>> racing in amdgpu_vm_clear_freed(). So the freed list should be
>>>>> protected with the status_lock, similar to other vm lists.
>>>> Well this is nonsense. To process the freed list the VM root PD lock
>>>> must be held anyway.
>>>>
>>>> If we have a call path where this isn't true then we have a major bug at
>>>> a different place here.
>>> I'm not super familiar w/ the amdgpu cs parser stuff, but the only
>>> thing that I'm seeing that protects things is the bo_list_mutex and it
>>> isn't clear to me that this is 1:1 with the vm (it looks like it is
>>> not).
>> Do you have a backtrace?
>>
>> Take a look at the reservation object of vm->root.bo. This should always
>> be locked first before doing *anything* in a CS.
>>
>> If that isn't the case we have a much worse problem.
> In this case, maybe an dma_resv_assert_held() would be a good idea?

We should already have that. Which makes me really wonder what the heck
is going on here.

Christian.

>
> BR,
> -R
>
>>> (I cc'd you on the bug report, jfyi)
>> I unfortunately only get a permission denied when I try to access that one.
>>
>> Regards,
>> Christian.
>>
>>> BR,
>>> -R
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
>>>>> Signed-off-by: Rob Clark <robdclark@chromium.org>
>>>>> ---
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 33 ++++++++++++++++++++++----
>>>>> 1 file changed, 29 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> index b9441ab457ea..aeed7bc1512f 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> @@ -1240,10 +1240,19 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>>>>> struct amdgpu_bo_va_mapping *mapping;
>>>>> uint64_t init_pte_value = 0;
>>>>> struct dma_fence *f = NULL;
>>>>> + struct list_head freed;
>>>>> int r;
>>>>>
>>>>> - while (!list_empty(&vm->freed)) {
>>>>> - mapping = list_first_entry(&vm->freed,
>>>>> + /*
>>>>> + * Move the contents of the VM's freed list to a local list
>>>>> + * that we can iterate without racing against other threads:
>>>>> + */
>>>>> + spin_lock(&vm->status_lock);
>>>>> + list_replace_init(&vm->freed, &freed);
>>>>> + spin_unlock(&vm->status_lock);
>>>>> +
>>>>> + while (!list_empty(&freed)) {
>>>>> + mapping = list_first_entry(&freed,
>>>>> struct amdgpu_bo_va_mapping, list);
>>>>> list_del(&mapping->list);
>>>>>
>>>>> @@ -1258,6 +1267,15 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>>>>> amdgpu_vm_free_mapping(adev, vm, mapping, f);
>>>>> if (r) {
>>>>> dma_fence_put(f);
>>>>> +
>>>>> + /*
>>>>> + * Move any unprocessed mappings back to the freed
>>>>> + * list:
>>>>> + */
>>>>> + spin_lock(&vm->status_lock);
>>>>> + list_splice_tail(&freed, &vm->freed);
>>>>> + spin_unlock(&vm->status_lock);
>>>>> +
>>>>> return r;
>>>>> }
>>>>> }
>>>>> @@ -1583,11 +1601,14 @@ int amdgpu_vm_bo_unmap(struct amdgpu_device *adev,
>>>>> mapping->bo_va = NULL;
>>>>> trace_amdgpu_vm_bo_unmap(bo_va, mapping);
>>>>>
>>>>> - if (valid)
>>>>> + if (valid) {
>>>>> + spin_lock(&vm->status_lock);
>>>>> list_add(&mapping->list, &vm->freed);
>>>>> - else
>>>>> + spin_unlock(&vm->status_lock);
>>>>> + } else {
>>>>> amdgpu_vm_free_mapping(adev, vm, mapping,
>>>>> bo_va->last_pt_update);
>>>>> + }
>>>>>
>>>>> return 0;
>>>>> }
>>>>> @@ -1671,7 +1692,9 @@ int amdgpu_vm_bo_clear_mappings(struct amdgpu_device *adev,
>>>>> tmp->last = eaddr;
>>>>>
>>>>> tmp->bo_va = NULL;
>>>>> + spin_lock(&vm->status_lock);
>>>>> list_add(&tmp->list, &vm->freed);
>>>>> + spin_unlock(&vm->status_lock);
>>>>> trace_amdgpu_vm_bo_unmap(NULL, tmp);
>>>>> }
>>>>>
>>>>> @@ -1788,7 +1811,9 @@ void amdgpu_vm_bo_del(struct amdgpu_device *adev,
>>>>> amdgpu_vm_it_remove(mapping, &vm->va);
>>>>> mapping->bo_va = NULL;
>>>>> trace_amdgpu_vm_bo_unmap(bo_va, mapping);
>>>>> + spin_lock(&vm->status_lock);
>>>>> list_add(&mapping->list, &vm->freed);
>>>>> + spin_unlock(&vm->status_lock);
>>>>> }
>>>>> list_for_each_entry_safe(mapping, next, &bo_va->invalids, list) {
>>>>> list_del(&mapping->list);

\
 
 \ /
  Last update: 2023-03-27 00:11    [W:0.041 / U:0.960 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site