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

[Koenig, Christian]

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.

Christian.

>
>>> Considering the examples of flaky or outright missing drmAuth in prime
>>> open-source projects (mesa, kmscube, libva) we can reasonably assume
>>> other projects exbibit the same problem.
>>>
>>> For these and/or other reasons, two DRM drivers have dropped DRM_AUTH
>>> since day one.
>>>
>>> Since we are interested in providing consistent and predictable
>>> behaviour to user-space, dropping DRM_AUTH seems to be the most
>>> effective way forward.
>> Well and what do you do with drivers which doesn't implement render nodes?
>>
> AFAICT there is a single non-DC driver which does not expose - QXL.
> I would expect it runs on a rather customised stack.
>
> Would be great to fix that, but ETIME and it's the only exception to the
> rule.
>
>
>>> Of course, I'd be more than happy to hear for any other way to achieve
>>> the goal outlined.
>>>
>>> Based on the series, other maintainers agree with the idea/goal here.
>>> Amdgpu not following the same pattern would compromise predictability
>>> across drivers and complicate userspace, so I would kindly ask you to
>>> reconsider.
>>>
>>> Alternatively, I see two ways to special case:
>>>    - New flag annotating amdgpu/radeon - akin to the one proposed earlier
>>>    - Check for amdgpu/radeon in core DRM
>> I perfectly agree that I don't want any special handling for amdgpu/radeon.
>>
>> My key point is that with DRM_AUTH we created an authorization and
>> authentication mess in DRM which is functionality which doesn't belong
>> into the DRM subsystem in the first place.
>>
> Precisely and I've outlined below how to resolve this in the long run.
>
>>> Now on your pain point - not allowing render iocts via the primary node,
>>> and how this patch could make it harder to achieve that goal.
>>>
>>> While I love the idea, there are number of obstacles that prevent us
>>> from doing so at this time:
>>>    - Ensuring both old and new userspace does not regress
>> Yeah, agree totally. That's why I said we should probably start doing
>> this for only for upcoming hardware generations.
>>
> That will side-step the regression issue, but will enforce driver
> specific behaviour outlined before.
>
>>>    - Drivers (QXL, others?) do not expose a render node
>> Well isn't that is a rather big problem for the removal of DRM_AUTH in
>> general?
>>
>> E.g. at least QXL would then expose functionality on the primary node
>> without authentication.
>>
> With this series QXL remains functionally unchanged. I would love to fix
> that as well yet ETIME as mentioned above.
>
>>>    - We want to avoid driver specific behaviour
>>>
>>> The only way forward that I can see is:
>>>    - Address QXL/others to expose render nodes
>>>    - Add a Kconfig toggle to disable !KMS ioctls via the primary node
>>>    - Jump-start ^^ with distribution X
>>>    - Fix user-space fallouts
>>>    - X months down the line, flip the Kconfig
>>>    - In case of regressions - revert the KConfig, goto Fix user-space...
>> Well that at least sounds like a plan :) Let's to this!
>>
> We're talking about months if not years until it comes to fruition. We
> need something quicker.
>
> That said, I'm quite happy to take the first stab, yet this is not a
> replacement for this series.
>
>>> That said, the proposal will not conflict with the DRM_AUTH removal. If
>>> anything it is step 0.5 of the grand master plan.
>> That's the point I strongly disagree on.
>>
>> By lowering the DRM_AUTH restriction you are encouraging userspace to
>> use the shortcut and use the primary node for rendering instead of
>> fixing the code and using the render node.
>>
> Have to disagree here. After working on the user-space side and fixing
> issues in various projects I can honestly say that most user-space is
> sane and developers _care_ about doing things correctly.
>
>> So at the end of the day userspace will most likely completely drop
>> support for the render node, simply for the reason that it became
>> superfluous. You can just open up the primary node and get the same
>> functionality.
>>
> I think you're being overly pessimistic here. This is coming from a
> person who is often closer to the pessimistic side of things.
>
> As a middle ground how about we do the following:
>   - Get this series as-is, alongside
>   - picking the first three items from the grand master plan
>     - happy to start a discussion about QXL
>     - a patch for the Kconfig toggle, is coming in a few minutes
>     - happy to prod my distribution of choice (Arch) and work with them
>
> What do you think?
>
> Thanks
> Emil
>