Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

["Koenig, Christian" <Christian.Koenig@amd.com>]

Am 21.06.19 um 12:20 schrieb Emil Velikov:
> On 2019/06/21, Daniel Vetter wrote:
>> On Fri, Jun 21, 2019 at 11:25 AM Koenig, Christian
>> <Christian.Koenig@amd.com> wrote:
>>> Am 21.06.19 um 11:09 schrieb Daniel Vetter:
>>>> On Fri, Jun 21, 2019 at 07:12:14AM +0000, Koenig, Christian wrote:
>>>>> Am 20.06.19 um 18:30 schrieb Emil Velikov:
>>>>>> On 2019/06/14, Koenig, Christian wrote:
>>>>>>> Am 14.06.19 um 17:53 schrieb Emil Velikov:
>>>>>>>> On 2019/06/14, Koenig, Christian wrote:
>>>>>>>>> Am 14.06.19 um 14:09 schrieb Emil Velikov:
>>>>>>>>>> On 2019/05/27, Emil Velikov wrote:
>>>>>>>>>> [SNIP]
>>>>>>>>>> Hi Christian,
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> In the following, I would like to summarise and emphasize the need for
>>>>>>>>>> DRM_AUTH removal. I would kindly ask you to spend a couple of minutes
>>>>>>>>>> extra reading it.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Today DRM drivers* do not make any distinction between primary and
>>>>>>>>>> render node clients.
>>>>>>>>> That is actually not 100% correct. We have a special case where a DRM
>>>>>>>>> master is allowed to change the priority of render node clients.
>>>>>>>>>
>>>>>>>> Can you provide a link? I cannot find that code.
>>>>>>> See amdgpu_sched_ioctl().
>>>>>>>
>>>>>>>>>> Thus for a render capable driver, any premise of
>>>>>>>>>> separation, security or otherwise imposed via DRM_AUTH is a fallacy.
>>>>>>>>> Yeah, that's what I agree on. I just don't think that removing DRM_AUTH
>>>>>>>>> now is the right direction to take.
>>>>>>>>>
>>>>>>>> Could have been clearer - I'm talking about DRM_AUTH | DRM_RENDER_ALLOW
>>>>>>>> ioctls.
>>>>>>>>
>>>>>>>> That aside, can you propose an alternative solution that addresses this
>>>>>>>> and the second point just below?
>>>>>>> Give me a few days to work on this, it's already Friday 6pm here.
>>>>>>>
>>>>>> Hi Christian,
>>>>>>
>>>>>> Any progress? As mentioned earlier, I'm OK with writing the patches although
>>>>>> I would love to hear your plan.
>>>>> First of all I tried to disable DRM authentication completely with a
>>>>> kernel config option. Surprisingly that actually works out of the box at
>>>>> least on the AMDGPU stack.
>>>>>
>>>>> This effectively allows us to get rid of DRI2 and the related security
>>>>> problems. Only thing left for that is that I'm just not sure how to
>>>>> signal this to userspace so that the DDX wouldn't advertise DRI2 at all
>>>>> any more.
>>>>>
>>>>>
>>>>> As a next step I looked into if we can disable the command submission
>>>>> for DRM master. Turned out that this is relatively easy as well.
>>>>>
>>>>> All we have to do is to fix the bug Michel pointed out about KMS handles
>>>>> for display and let the DDX use a render node instead of the DRM master
>>>>> for Glamor. Still need to sync up with Michel and/or Marek whats the
>>>>> best way of doing this.
>>>>>
>>>>>
>>>>> The last step (and that's what I haven't tried yet) would be to disable
>>>>> DRM authentication for Navi even when it is still compiled into the
>>>>> kernel. But that is more or less just a typing exercise.
>>>> So just to get this straight, we're now full on embracing a subsystem
>>>> split here:
>>>> - amdgpu plans to ditch dri2/rendering on primary nodes
>>>> - bunch of drivers (I think more than i915 by now) merged the DRM_AUTH
>>>>     removal
>>>> - others are just hanging in there somehow
>>>>
>>>> "best of both^W worlds" ftw?
>>> Yes, exactly!
>>>
>>> Think a step further, when this is upstream we can apply the DRM_AUTH
>>> removal to amdgpu as well.
>>>
> So this is effectively what I suggested/planned with a couple of caveats:
>   - making amdgpu behave differently from the rest of drm ;-(
>   - having the KConfig amdgpu only and merged before this DRM_AUTH

No this should apply to all drivers, not just amdgpu.

> While I suggested:
>   - keeping amdgpu consistent with the rest
>   - exposing the KConfig option for the whole of the kernel, and
>   - merging it alongside this work
>
> So I effectively waited for weeks, instead of simply going ahead and
> writing/submitting patches. That's a bit unfortunate...
>
>>> Because we simply made sure that userspace is using the render node for
>>> command submission and not the primary node.
>>>
>>> So nobody can go ahead and remove render node support any more :)
>> How does this work? I thought the entire problem of DRM_AUTH removal
>> for you was that it broke stuff, and you didn't want to deal with
>> that. We still have that exact problem afaics ... old userspace
>> doesn't simply vanish, even if you entirely nuke it going forward.
>>
>> Also, testing on the amdgpu stack isn't really testing a hole lot
>> here, it's all the various silly compositors out there I'd expect to
>> fall over. Will gbm/radeonsi/whatever just internally do all the
>> necessary dma-buf import/export between the primary node and display
>> node to keep this all working?
> If I understood Christian, feel free to correct me, the fact that my
> earlier patch broke RADV was not the argument. The problem was was the
> patch does.

Well partially. That RADV broke is unfortunate, but we have done so many 
customized userspace stuff both based on Mesa/DDX as well as closed 
source components that it is just highly likely that we would break 
something else as well.

> In particular, that user-space will "remove" render nodes.

Yes, that is my main concern here. You basically make render nodes 
superfluously. That gives userspace all legitimization to also remove 
support for them. That is not stupid or evil or whatever, userspace 
would just follow the kernel design.

> I'm really sad that amdgpu insists on standing out, hope one day it will
> converge. Yet since all other maintainers are ok with the series, I'll
> start merging patches in a few hours. I'm really sad that amdgpu wants
> to stand out, hope it will converge sooner rather than later.
>
> Christian, how would you like amdgpu handled - with a separate flag in
> the driver or shall we special case amdgpu within core drm?

No, I ask you to please stick around DRM_AUTH for at least amdgpu and 
radeon. And NOT enable any render node functionality for them on the 
primary node.

Thanks,
Christian.

>
> Thanks
> Emil

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel