[igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Emil Velikov]

On Thu, 24 Jan 2019 at 11:04, Daniel Vetter <daniel at ffwll.ch> wrote:
>
> On Wed, Jan 23, 2019 at 08:01:45PM +0000, Emil Velikov wrote:
> > On Wed, 23 Jan 2019 at 16:44, Daniel Vetter <daniel at ffwll.ch> wrote:
> > >
> > > On Wed, Jan 23, 2019 at 03:55:39PM +0000, Emil Velikov wrote:
> > > > On Wed, 23 Jan 2019 at 11:33, Daniel Vetter <daniel at ffwll.ch> wrote:
> > > >
> > > > > > > How do you envision the card* to renderD* helper? Should I go with the
> > > > > > > minor(rdev) & 0x80 hack, use the libdrm helpers or something else?
> > > > > >
> > > > > > I think it's minor(rdev) + 0x80, but yeah I think that's official uapi now
> > > > > > :-/
> > > > > >
> > > > Fwiw: The libdrm helpers use something else have been around for ages.
> > > > Mesa uses them not sure for others :-(
> > > >
> > > > That said, I could remove all the crazy libdrm code and use
> > > > minor(rdev) + 0x80. Are you OK with that?
> > >
> > > I think cleaning up the igt drm_open hilarity is a different project ...
> > > minor+0x80 has my ack.
> >
> > I mentioned libdrm your reply says igt drm_open. Slightly confused,
> > but it's not relevant so meh.
> >
> > > > > > I guess if you want to check for -EACCESS make a 2nd test (or merge them)
> > > > > > which makes sure that if we have a non-render node, then we get an
> > > > > > EACCESS. Would mean a quick testrun on a display-only drm driver (those
> > > > > > are all non-rendernode) to make sure it works. And would be an even better
> > > > > > test I think.
> > > > This 2nd test is exactly what I wrote with this patch, is it not?
> > > > Admittedly it's missing the !DRM_CAP_PRIME case, which I will fix for
> > > > v2.
> > >
> > > You check for != EACCESS, this idea is to 1) make sure we have a
> > > !DRIVER_RENDER driver and 2) check that we still get an EACCESS (like on
> > > old kernels). So different test. I think ...
> > >
> > I fear we might be talking past each other. What I'm proposing is:
> >
> > /*
> >  * Updated kernels allow render capable, unauthenticated
> >  * master to issue DRM_AUTH ioctls (like the above), as long as
> >  * they are annotated as DRM_RENDER_ALLOW.
> >  *
> >  * Otherwise we return EACCES.
> >  *
> >  * Here we are not interested in the specific validation done by
> >  * FD2HANDLE ioctl itself, but only about EACCES being thrown by
> >  * core DRM
> >  */
> > if (getcap(DRM_CAP_PRIME)) // has PRIME
> >        igt_assert(errno != EACCES);
> > else
> >        igt_assert(errno == EACCES);
> >
> > This way we check exactly for what's expected.
>
> The problem why I don't like errno != EACCESS checks is aliasing of
> failures.

Fully agree in the general case, although here:
 - we want to validate the drm_permit() handling
 - there is no specific ioctl that only exercises the above function,
so we select a random one (FD2HANDLE)
 - the function itself returns EACCES and we don't care about the
FD2HANDLE specifics

True, some FD2HANDLE failures may be aliased by the != EACCES check.
Yet those should really be in a prime test.

How about i write a simple EBADF test under tests/prime*.c and keep
the EACCES in tests/core_auth.c (a)?
Alternatively, I could keep the EBADF in tests/core_auth.c although
I'll add a chunky comment that, in practise, we only care about EACCES
(b).

Which one do you prefer?

Thanks
Emil