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

[Samuel Pitoiset <samuel.pitoiset-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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@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.

>
> 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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx