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

[Daniel Vetter]

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. Here's the general recipe:

1. run ioctl in a very specific way.

2. make sure the outcome is _exaclty_ what you suspect. Best case this
means success, slightly worse is a specific errno. Really bad is EINVAL,
because we have way too much code in the kernel that throws EINVAL.

3. Change one thing and one thing only in your ioctl command (could be an
argument, could be environment like CAP_SYS_ADMIN), re-run it.

4. Make sure it fails in a very specific fashion.

If you instead have an unspecific testcase that just checks for errno !=
ESOMETHING, then if you're unlucky, and there's some unrelated breakage,
then your test still passes, but it stops checking what you think it
shouold check.

E.g. in this case here if you break both the access check _and_ something
else in fd2handle, then the result might still be errno != ESOMETHING, and
so you won't spot the issue.

Of course having perfect test coverage for everything else would prevent
that, but engineering reality is that you'll never have perfect enough
test coverage, and even after 5+ years of working hard, igt definitely
doesn't have perfect test coverage. Hence imo better to write more robust
and more specific tests.

Being strict in what you emit and forgiving in what you accept is not a
great principle for testcases (or kernel uapi design in general, same
reasons why I don't like the new drmIsMaster idea).
-Daniel

> 
> >
> > > 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch