On Tue, Mar 03, 2020 at 02:46:51PM +0000, Emil Velikov wrote:
> On Tue, 3 Mar 2020 at 13:13, Petri Latvala <petri.latvala at intel.com> wrote:
> >
> > On Mon, Mar 02, 2020 at 05:33:14PM +0000, Emil Velikov wrote:
> > > From: Emil Velikov <emil.velikov at collabora.com>
> > >
> > > This test adds three distinct subtests:
> > > - drop/set master as root
> > > - drop/set master as non-root
> > > - drop/set master for a shared fd
> > >
> > > Currently the second subtest will fail, with kernel patch to address
> > > that has been submitted.
> > >
> > > v2: Add to the autotools build
> > > v3:
> > > - Add igt_describe()
> > > - Use igt_fixture() for tweak_perm
> > > - Enhance comments
> > >
> > > Cc: Petri Latvala <petri.latvala at intel.com>
> > > Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> > > ---
> > > tests/Makefile.sources | 1 +
> > > tests/core_setmaster.c | 206 +++++++++++++++++++++++++++++++++++++++++
> > > tests/meson.build | 1 +
> > > 3 files changed, 208 insertions(+)
> > > create mode 100644 tests/core_setmaster.c
> > >
> > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > > index b87d6333..5da36a91 100644
> > > --- a/tests/Makefile.sources
> > > +++ b/tests/Makefile.sources
> > > @@ -18,6 +18,7 @@ TESTS_progs = \
> > > core_getclient \
> > > core_getstats \
> > > core_getversion \
> > > + core_setmaster \
> > > core_setmaster_vs_auth \
> > > debugfs_test \
> > > dmabuf \
> > > diff --git a/tests/core_setmaster.c b/tests/core_setmaster.c
> > > new file mode 100644
> > > index 00000000..58ed9771
> > > --- /dev/null
> > > +++ b/tests/core_setmaster.c
> > > @@ -0,0 +1,206 @@
> > > +/*
> > > + * Copyright © 2020 Collabora, Ltd.
> > > + *
> > > + * Permission is hereby granted, free of charge, to any person obtaining a
> > > + * copy of this software and associated documentation files (the "Software"),
> > > + * to deal in the Software without restriction, including without limitation
> > > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > > + * and/or sell copies of the Software, and to permit persons to whom the
> > > + * Software is furnished to do so, subject to the following conditions:
> > > + *
> > > + * The above copyright notice and this permission notice (including the next
> > > + * paragraph) shall be included in all copies or substantial portions of the
> > > + * Software.
> > > + *
> > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > > + * IN THE SOFTWARE.
> > > + *
> > > + * Authors:
> > > + * Emil Velikov <emil.l.velikov at gmail.com>
> > > + *
> > > + */
> > > +
> > > +/*
> > > + * Testcase: Check that drop/setMaster behaves correctly wrt root/user access
> > > + *
> > > + * Test checks if the ioctls succeed or fail, depending if the applications was
> > > + * run with root, user privileges or if we have separate privileged arbitrator.
> > > + */
> > > +
> > > +#include "igt.h"
> > > +#include <unistd.h>
> > > +#include <stdlib.h>
> > > +#include <stdio.h>
> > > +#include <string.h>
> > > +#include <sys/stat.h>
> > > +
> > > +IGT_TEST_DESCRIPTION("Check that Drop/SetMaster behaves correctly wrt root/user"
> > > + " access");
> > > +
> > > +static bool is_master(int fd)
> > > +{
> > > + /* FIXME: replace with drmIsMaster once we bumped libdrm version */
> > > + return drmAuthMagic(fd, 0) != -EACCES;
> > > +}
> > > +
> > > +static void check_drop_set(void)
> > > +{
> > > + int master;
> > > +
> > > + master = __drm_open_driver(DRIVER_ANY);
> > > +
> > > + /* Ensure we have a valid device. This is _extremely_ unlikely to
> > > + * trigger as tweak_perm() aims to ensure we have the correct rights.
> > > + * Although:
> > > + * - according to the manual igt_drop_root() + igt_skip() is broken
> >
> > I asked for clarification on this and didn't get a reply. I'm not
> > seeing such a mention. Well, there is the text warning about
> > igt_drop_root() and atexit handlers and recommending the use of
> > igt_drop_root() in a child process to avoid issues, and this is in a
> > child process.
> >
> > Whatever I'm missing here surely also applies to igt_assert as well?
> >
> Darn it ... got the wrong function which explains the confusion.
>
> In particular igt_skip() isn't propagated properly within a
> igt_fork(), as documented [1].
> This results in a) the test failing [2] (instead of skipping) with b)
> semi-magical trace.
>
> If the IGT team is happy with those pitfalls, I can switch to igt_require :-)
>
> [1] https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/lib/igt_core.h#L1045
> [2] igt_skip: Assertion `"!test_child` failed
Ah, now I see!
Indeed skip isn't propagated properly and we should get that fixed at
some point. No need to hold your patch as hostage though.
With a comment that states that skip is not properly propagated from
child processes, keep the igt_assert and __drm_open_driver() usage in
check_drop_set(). There's igt_assert_fd() btw for a shorthand check
for fd != -1.
The second subtest can use igt_require though, by way of using
drm_open_driver().
>
>
> >
> > > + * - there is _no_ guarantee that we'll open a device handled by
> > > + * tweak_perm()
> >
> > This is a point that came to mind later, the building blocks for that
> > guarantee are in igt_device_scan.
> >
> > 1) If filter already set (test being run with --device), use that
> > filter (igt_device_filter_get or so). Otherwise loop through the
> > generated card%u names, using drm:/dev/dri/card%u as the
> > filter. igt_device_scan.c stuff already has a list of all possible
> > devices but I don't see an easy way to get to the list.
> >
> > 2) igt_device_card_match() gets you a card
> >
> > 3) card->card should be the path to /dev/dri to chmod on
> >
> > 4) igt_open_card(card) gives you an fd to that card
> >
> Not entirely sure what you're suggesting here. Can you please elaborate?
lib/igt_device_scan.[ch] is for device selection and you can use that
to make sure chown has hit the device drm_open_driver() is
opening. Although I wrote that comment thinking you only chmod one of
them but that was a misreading from my part, ignore this comment. You
chmod all of them. Even with --device usage chmoding all devices will
ensure the only way for chmodding not to be done is not having the
proper kernel modules modprobed. Which drm_open_driver will do. Yikes.
Which luckily brings us to the best of both worlds. In the igt_fixture
before the first subtest:
/* Ensure modules are loaded and the device file exists */
close(drm_open_driver(DRIVER_ANY));
Now having the igt_assert (as opposed to igt_require()) in the child
process makes even more sense.
>
> >
> > > + * - having a device is integral part of the test
> >
> > It's an integral part of all tests. The issue with the use of
> > igt_require vs igt_assert is whether it's a prerequisite or the thing
> > you're actually testing.
> >
> The latter. I can reword that comment to "successfully opening a
> device is part of the test" or anything that you'd prefer.
I would prefer that yes. The skip propagation is enough to justify
igt_assert though. And having that module reloading drm_open_driver()
in the fixture.
>
> >
> > > + */
> > > + igt_assert_neq(master, -1);
> > > +
> > > + /* At this point we're master capable due to:
> > > + * - being root - always
> > > + * - normal user - as the only drm only drm client (on this VT)
> > > + */
> > > + igt_assert_eq(is_master(master), true);
> >
> > Just igt_assert(is_master(master))
> >
> Sure fixed, alongside the s/ts/test/ type in the summary.
>
> Is there some dos and don'ts about which igt_assert* functions to use?
> Would it make sense to write short list + mass conversion to the
> recommended ones?
In general, use igt_assert_eq(x, y) instead of igt_assert(x == y) to
get better error messages on failure, as it prints the values of both
operands. However, for values that are already just booleans, plain
igt_assert() reads a bit better.
--
Petri Latvala