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

[Emil Velikov]

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.

>
> > If we want to check that FD2HANDLE returns EBADF for invalid fd (-1 or
> > otherwise) - sure, but that's unrelated to the goal here.
>
> The idea is to check for something that happens only once the drm_ioctl()
> core function actually calls ioctl->func. Just to make the test more
> robust against changes in drm_ioctl() - I always aim for a know outcome
> instead of checking for errno != ESOMETHING. Checking for a specific to
> happen is a stronger test than checking for a specific thing to not happen
> (since that leaves the door open for unrelated breakage with the same
> result).
>
Cannot agree more about robust ioctls checking. If we want to validate
that FD2HANDLE of -1 returns EBADF that should be in some prime_*
test, right?


> If you chmod and do igt_drop_root(); drm_open_any(); it'll work.

Right, the test does igt_drop_root(); drm_open_any();

It's upto the machine maintainer to chmod or equivalent. Although if
you prefer I could add the chmod to the test itself?
It feels very nasty and I'm not sure if it will be possible to undo.

Thanks
Emil