[PATCH 1/2] drm/amdgpu: drop multiple bo_unreserve() calls in amdgpu_gem_op_ioctl()

[Christian König]

Am 10.02.2017 um 11:22 schrieb Samuel Pitoiset:
>
>
> On 02/10/2017 11:19 AM, Christian König wrote:
>> Am 10.02.2017 um 11:11 schrieb Samuel Pitoiset:
>>>
>>>
>>> On 02/10/2017 03:55 AM, zhoucm1 wrote:
>>>>
>>>>
>>>> On 2017年02月10日 06:28, Samuel Pitoiset wrote:
>>>>> Move amdgpu_bo_unreserve() outside of the switch. While we are
>>>>> at it, add a missing break in the default case.
>>>>>
>>>>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 7 ++-----
>>>>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> index 1dc59aafec71..ae4658a10e2c 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> @@ -660,7 +660,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>>>>> void *data,
>>>>>           info.alignment = robj->tbo.mem.page_alignment << 
>>>>> PAGE_SHIFT;
>>>>>           info.domains = robj->prefered_domains;
>>>>>           info.domain_flags = robj->flags;
>>>>> -        amdgpu_bo_unreserve(robj);
>>>>>           if (copy_to_user(out, &info, sizeof(info)))
>>>>>               r = -EFAULT;
>>>> NAK, your this change will break our previous deadlock fix for 
>>>> ww_mutex
>>>> and mm->mmap_sem if I remember correctly.
>>>
>>> Mmh, really? Can you pinpoint the commit? I don't see anything obvious
>>> in the history about that.
>>
>> David is right here. Not sure when we fixed that, but in general calling
>> copy_to/from_user while a BO is reserved is illegal.
>>
>> Otherwise somebody could send the kernel a memory mapped BO as address
>> and the copy_to/from_user would just deadlock because it tries to
>> reserve a BO while another (or the same) BO is already reserved in the
>> call path.
>
> Okay, makes more sense.
>
> Thanks for the review. My goal is not to introduce new regressions but 
> I didn't know that.
>
> Maybe this should be explained in the code? Just a thought.

We do have a comment in the TTM mapping code I think.

But copy_to/from_user and BO reservation is just so common that we would 
need to add the same comment on a whole bunch of different places and 
that doesn't make to much sense.

But in this particular case a comment might make sense because I think 
somebody proposed the same patch before. Ah! Enlightenment, that also 
explains why you can't find it in the history. We just rejected the same 
patch multiple times now :)

Regards,
Christian.

>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Thanks.
>>>
>>>>
>>>> Regards,
>>>> David Zhou
>>>>>           break;
>>>>> @@ -668,7 +667,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>>>>> void *data,
>>>>>       case AMDGPU_GEM_OP_SET_PLACEMENT:
>>>>>           if (amdgpu_ttm_tt_get_usermm(robj->tbo.ttm)) {
>>>>>               r = -EPERM;
>>>>> -            amdgpu_bo_unreserve(robj);
>>>>>               break;
>>>>>           }
>>>>>           robj->prefered_domains = args->value &
>>>>> (AMDGPU_GEM_DOMAIN_VRAM |
>>>>> @@ -677,14 +675,13 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>>>>> void *data,
>>>>>           robj->allowed_domains = robj->prefered_domains;
>>>>>           if (robj->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>>>>               robj->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>>>>> -
>>>>> -        amdgpu_bo_unreserve(robj);
>>>>>           break;
>>>>>       default:
>>>>> -        amdgpu_bo_unreserve(robj);
>>>>>           r = -EINVAL;
>>>>> +        break;
>>>>>       }
>>>>>   +    amdgpu_bo_unreserve(robj);
>>>>>   out:
>>>>>       drm_gem_object_unreference_unlocked(gobj);
>>>>>       return r;
>>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx