[Libva] [PATCH] drm: improve check for authentication.

[Gwenole Beauchesne]

Hi,

2013/7/16 Daniel Vetter <daniel at ffwll.ch>:
> On Tue, Jul 16, 2013 at 03:32:53PM +0200, Gwenole Beauchesne wrote:
>> Hi,
>>
>> 2013/7/16 Daniel Vetter <daniel at ffwll.ch>:
>> > On Tue, Jul 16, 2013 at 2:38 PM, Gwenole Beauchesne <gb.devel at gmail.com> wrote:
>> >>
>> >> 2013/7/16 Daniel Vetter <daniel at ffwll.ch>:
>> >>> On Mon, Jul 15, 2013 at 6:30 PM, Gwenole Beauchesne <gb.devel at gmail.com> wrote:
>> >>
>> >>>> On Linux systems, the drmGetClient() function would return the thread ID
>> >>>> instead of the actual process ID.
>> >>>>
>> >>>> Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne at intel.com>
>> >>>
>> >>> Oh dear, why is it always libva. I've prepared a patch to kill this
>> >>> ioctl (almost) completely since it's imo rather pointless. Now I've
>> >>> just discovered that libva uses it and will make it much harder to
>> >>> fake things in the kernel! Or have I just misread the code and it's
>> >>> actually possible to return -EINVAL for this ioctl and libva will
>> >>> transparently fall back to doing the X11 auth stuff?
>> >>
>> >> Yes, we will transparently fallback to X11 auth. In this case, we
>> >> should not check auth status again if X11 auth was successful.
>> >
>> > Re-reading the code you recheck the auth status even if
>> > va_drm_authenticate succeeded, so I think completely breaking the
>> > GET_CLIENT ioctl won't work out.
>>
>> Yes, this is why I said "we should not check auth status again", thus
>> implying that another patch is needed. :)
>
> Oh, for kernel work that doesn't help since libva has shipped in this
> configuration, so we get to support it for the next 10 years ;-)

Ah, sorry for that. Though, updating libva is generally just fine and harmless.

>> >> However, drmGetClient() was used to check we are already
>> >> authenticated. What would the alternative be?
>> >
>> > I've just wondered a bit why libva can't keep track of the auth status
>> > like mesa and only attempt the auth dance once.
>>
>> There are some provisions to cache the resulting VA/DRM display too,
>> but this is not done yet and it seems that some other layers (driver,
>> or middleware) would not expect that behaviour on the other hand.
>>
>> BTW, if no X display server is running, are we ensured nowadays that
>> drmAuthMagic() would not silently fail while still returning zero?
>
> I think currently you can't reliably use a drm device without either a
> display server to authenticate you or being root. Everything else is a
> pretty messy hack.

I think the existing behaviour is to also allow the first client as
is, without the need to being root. Others are going to fail, and I am
fine with it(*). At least, that currently works that way on my system.

(*) and I really mean me, others obviously want to be able to address
the GPU from multiple other processes. My opinion and understanding is
that, this behaviour is out of libva scope and I don't really want to
replicate all various joys of authentication there, and a third-party
application is needed anyway to serve as the arbiter. I mean, for X we
have the Xserver, for Wayland, we have weston. For real headless
system, we would need someone else.

My other understanding, or at least expectation, is that render nodes
were supposed to fix that problem/need?

>> > So imo userspace
>> > doesn't really need to know this information. Anyway it'll all die
>> > with the advent of rendernodes, the only downside is that it'll make
>> > transparently using rendernodes instead of of the legacy nodes for drm
>> > clients a bit more awkward ...
>>
>> Do you mean, anything more awkward than adding an ioctl to determine
>> if we have a rendernode or a legacy one, or trying to determine the
>> device path from the fd if no ioctl is available for that?
>
> Imo modern userspace should just not care about whether auth fails or not.
> And iirc mesa doesn't really check that, but mesa also has a pretty
> massive abstraction for the differen windowing systems you can run egl on
> top of.

You are right, I was actually preparing a patch to nuke
va_drm_is_authenticated() altogether with just trying to call
va_drm_authenticate() and live with it. It should fail gracefully,
otherwise. There is no real reason for the current code, well, I don't
remember any. :)

My worry is really about headless cases, there is no issue for X11-based auth.

> I expect that we need to teach gbm (or similar layers) some smarts
> for running headless clients on rendernodes (so that it tries to open the
> rendernode if they're available before trying legacy nodes).

That was Kristian's plan too IIRC, but I really have stale info.

> But with a display server I really hope X/Wayland could just point the
> client at the respective rendernode and stuff would Just Work.

Unfortunately, I don't think this is going to happen since this is a
really system-specific feature.

Regards,
Gwenole.