RE: [PATCH] drm/amdgpu: add the checking to avoid NULL pointer dereference

["Sharma, Deepak" <Deepak.Sharma@xxxxxxx>]

在 2018/11/24 2:10, Koenig, Christian 写道:
> Am 23.11.18 um 15:10 schrieb Zhou, David(ChunMing):
>> 在 2018/11/23 21:30, Koenig, Christian 写道:
>>> Am 23.11.18 um 14:27 schrieb Zhou, David(ChunMing):
>>>> 在 2018/11/22 19:25, Christian König 写道:
>>>>> Am 22.11.18 um 07:56 schrieb Sharma, Deepak:
>>>>>> when returned fence is not valid mostly due to userspace ignored 
>>>>>> previous error causes NULL pointer dereference.
>>>>> Again, this is clearly incorrect. The my other mails on the 
>>>>> earlier patch.
>>>> Sorry for I didn't get your history, but looks from the patch 
>>>> itself, it is still a valid patch, isn't it?
>>> No, the semantic of amdgpu_ctx_get_fence() is that we return NULL 
>>> when the fence is already signaled.
>>>
>>> So this patch could totally break userspace because it changes the 
>>> behavior when we try to sync to an already signaled fence.
>> Ah, I got your meaning, how about attached patch?
> Yeah something like this, but I would just give the 
> DRM_SYNCOBJ_CREATE_SIGNALED instead.
>
> I mean that's what this flag is good for isn't it?
Yeah, I give a flag initally when creating patch, but as you know, there is a swich case not be able to use that flag:

     case AMDGPU_FENCE_TO_HANDLE_GET_SYNC_FILE_FD:
         fd = get_unused_fd_flags(O_CLOEXEC);
         if (fd < 0) {
             dma_fence_put(fence);
             return fd;
         }

         sync_file = sync_file_create(fence);
         dma_fence_put(fence);
         if (!sync_file) {
             put_unused_fd(fd);
             return -ENOMEM;
         }

         fd_install(fd, sync_file->file);
         info->out.handle = fd;
         return 0;

So I change to stub fence instead.

-David

I have not applied this patch.
The issue was trying to address is when amdgpu_cs_ioctl() failed due to low memory (ENOMEM) but userspace chose to proceed and called amdgpu_cs_fence_to_handle_ioctl().
In amdgpu_cs_fence_to_handle_ioctl(), fence is null and later causing NULL pointer dereference, this patch was to avoid that and system panic
But I understand now that its a valid case retuning NULL if fence was already signaled but need to handle case so avoid kernel panic. Seems David patch should fix this, I will test it tomorrow. 

-Deepak
>
> Christian.
>
>> -David
>>> If that patch was applied then please revert it immediately.
>>>
>>> Christian.
>>>
>>>> -David
>>>>> If you have already pushed the patch then please revert.
>>>>>
>>>>> Christian.
>>>>>
>>>>>> Signed-off-by: Deepak Sharma <Deepak.Sharma@xxxxxxx>
>>>>>> ---
>>>>>>       drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 ++
>>>>>>       1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>>> index 024dfbd87f11..14166cd8a12f 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>>> @@ -1403,6 +1403,8 @@ static struct dma_fence 
>>>>>> *amdgpu_cs_get_fence(struct amdgpu_device *adev,
>>>>>>             fence = amdgpu_ctx_get_fence(ctx, entity, user->seq_no);
>>>>>>           amdgpu_ctx_put(ctx);
>>>>>> +    if(!fence)
>>>>>> +        return ERR_PTR(-EINVAL);
>>>>>>             return fence;
>>>>>>       }

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx