[PATCH] drm/amdgpu: Add a GEM_CREATE mask and bugfix

Wed Feb 19 22:03:22 UTC 2020

On 2020-02-19 3:20 a.m., Christian König wrote:
> Am 18.02.20 um 22:46 schrieb Luben Tuikov:
>> On 2020-02-17 10:08 a.m., Christian König wrote:
>>> Am 17.02.20 um 15:44 schrieb Alex Deucher:
>>>> On Fri, Feb 14, 2020 at 7:17 PM Luben Tuikov <luben.tuikov at amd.com> wrote:
>>>>> Add a AMDGPU_GEM_CREATE_MASK and use it to check
>>>>> for valid/invalid GEM create flags coming in from
>>>>> userspace.
>>>>>
>>>>> Fix a bug in checking whether TMZ is supported at
>>>>> GEM create time.
>>>>>
>>>>> Signed-off-by: Luben Tuikov <luben.tuikov at amd.com>
>>>>> ---
>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 ++---------
>>>>>    include/uapi/drm/amdgpu_drm.h           |  2 ++
>>>>>    2 files changed, 4 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> index b51a060c637d..74bb79e64fa3 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> @@ -221,21 +221,14 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>>>>>           int r;
>>>>>
>>>>>           /* reject invalid gem flags */
>>>>> -       if (flags & ~(AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
>>>>> -                     AMDGPU_GEM_CREATE_NO_CPU_ACCESS |
>>>>> -                     AMDGPU_GEM_CREATE_CPU_GTT_USWC |
>>>>> -                     AMDGPU_GEM_CREATE_VRAM_CLEARED |
>>>>> -                     AMDGPU_GEM_CREATE_VM_ALWAYS_VALID |
>>>>> -                     AMDGPU_GEM_CREATE_EXPLICIT_SYNC |
>>>>> -                     AMDGPU_GEM_CREATE_ENCRYPTED))
>>>>> -
>>>> I'd rather keep the list explicit so no one ends up forgetting to
>>>> update the mask the next time new flags are added.
>>> Additional to that this patch is bogus.
>> So the only "additional" thing you're contributing to the review of
>> this patch is calling it "bogus". Characterizing the patch with an adjective
>> as "bogus" is very disturbing. What's next?
> 
> Well the patch is technical incorrect and breaks the code in a very 
> subtle and hard to detect manner. Alex didn't noticed that either.
> 
> I'm not a native speaker of English, but as far as I have learned 
> "bogus" is the right adjective for that.
> 
>>> We intentionally filter out the flags here which userspace is allowed to
>>> specify in the GEM interface, but after this patch we would allow all
>>> flags to be specified.
>> I thought that that could be the case.
> 
> Then why did you send out a patch which is seriously broken like that?
> 
> I mean if I hadn't noticed it by chance we would have committed this and 
> added a potentially security problematic bug to the IOCTL interface.
> 
>> But in your review why not
>> recommend we have a mask to check user-settable flags? So the
>> actual flags are collected and visible in one place and well
>> understood that that is what is being checked and well-defined
>> by a macro with an appropriate name?
> 
> See the flags tested here are the flags currently accepted by the 
> amdgpu_gem_create_ioctl() function. It doesn't say anything about what 
> the kernel would accept in the future.
> 
> So when we move that into the UAPI header we give userspace a 
> technically incorrect value.
> 
>> Why did NO ONE comment on the bug fix below? No one.
> 
> Because you mixed up a style change into some bug fix. That people go 
> for the problematic parts during code review is not really surprising at 
> all.

I wouldn't have expected a petulant-childlike response.
I would've expected a leadership-like position, something like:
Split your patch into two. The second part is okay with the addition
of DRM_PRINT_ONCE, and the first part admits bits which we don't
want user space to control.

Regards,
Luben

> 
> Regards,
> Christian.
> 
>>
>> Regards,
>> Luben
>>
>>> Christian.
>>>
>>>
>>>
>>>> Alex
>>>>
>>>>> +       if (flags & ~AMDGPU_GEM_CREATE_MASK)
>>>>>                   return -EINVAL;
>>>>>
>>>>>           /* reject invalid gem domains */
>>>>>           if (args->in.domains & ~AMDGPU_GEM_DOMAIN_MASK)
>>>>>                   return -EINVAL;
>>>>>
>>>>> -       if (amdgpu_is_tmz(adev) && (flags & AMDGPU_GEM_CREATE_ENCRYPTED)) {
>>>>> +       if (!amdgpu_is_tmz(adev) && flags & AMDGPU_GEM_CREATE_ENCRYPTED) {
>>>>>                   DRM_ERROR("Cannot allocate secure buffer since TMZ is disabled\n");
>>>>>                   return -EINVAL;
>>>>>           }
>>>>> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
>>>>> index eaf94a421901..c8463cdf4448 100644
>>>>> --- a/include/uapi/drm/amdgpu_drm.h
>>>>> +++ b/include/uapi/drm/amdgpu_drm.h
>>>>> @@ -141,6 +141,8 @@ extern "C" {
>>>>>     */
>>>>>    #define AMDGPU_GEM_CREATE_ENCRYPTED            (1 << 10)
>>>>>
>>>>> +#define AMDGPU_GEM_CREATE_MASK                  ((1 << 11)-1)
>>>>> +
>>>>>    struct drm_amdgpu_gem_create_in  {
>>>>>           /** the requested memory size */
>>>>>           __u64 bo_size;
>>>>> --
>>>>> 2.25.0.232.gd8437c57fa
>>>>>
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx at lists.freedesktop.org
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7Cb1fdc3970a224fc61fdc08d7b3bb4613%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175489240077250&sdata=rE8A6jKAIX081ZhxxcMc%2BpGdXvsLUdrAW4AkLsTpNxg%3D&reserved=0
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx at lists.freedesktop.org
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7Cb1fdc3970a224fc61fdc08d7b3bb4613%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175489240077250&sdata=rE8A6jKAIX081ZhxxcMc%2BpGdXvsLUdrAW4AkLsTpNxg%3D&reserved=0
> 