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

[Christian König]

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.

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