[igt-dev] [PATCH i-g-t v3] ts/core_setmaster: new test for drop/set master semantics
Emil Velikov
emil.l.velikov at gmail.com
Wed Mar 4 13:26:49 UTC 2020
On Wed, 4 Mar 2020 at 11:25, Petri Latvala <petri.latvala at intel.com> wrote:
>
> 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.
>
AFAICT I'm the only person who ever dares running tests as non-root,
which explains the current state ;-)
> 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().
Ack, comment tweaked.
> There's igt_assert_fd() btw for a shorthand check
> for fd != -1.
>
See my comment below.
> The second subtest can use igt_require though, by way of using
> drm_open_driver().
>
Do you really mean "second" aka "master-drop-set-user" here?
Above you're agree with using igt_assert(), yet here you suggest
(again) for igt_require()...
I'm confused :-\
drm_open_driver() does not look like a good solution here.
In particular we're testing core DRM functionality so anything GEM
(like in the embedded gem_quiescent_gpu call) is irrelevant.
Additionally, both gem_quiescent_gpu and the atexit handler require
root to drop the caches, et al.
Thinking about it, you're likely talking about the 3rd subtest aka
"master-drop-set-shared-fd". Sure we can use igt_require() there,
although I'd rather keep the__drm_open_driver() + igt_require()
combination.
Do you agree?
>
> >
> >
> > >
> > > > + * - 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.
>
Thinking about it close(drm_open_driver(...)) looks like a workaround,
instead of addressing the issue.
Even though it seems like it might work, the modprobe machinery seems partial:
- in the non i915 case, modprobe failure is a _debug_ message
- the DRIVER_FOO to module name mapping is partial
- for some drivers, the module name differs from the name in drmGetVersion()
- some drivers have changed their module name
So I'd suggest omitting the workaround and leaving the igt_assert() in
check_drop_set() do it's job.
>
> >
> > >
> > > > + * - 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.
>
Comment fixed.
>
> >
> > >
> > > > + */
> > > > + 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.
>
>From a quick look:
- 700+ instances of igt_assert(... == ...)
- 200+ instances of igt_assert(... != ...)
- 10+ instances of igt_assert_[n]eq(... {true,false})
- 200+ instances of igt_assert_fd permutations >= 0, < 0, -1
While I agree on the readability aspect, as-is we're in a pretty weird
spot. How about we a) document b) add basic CI plumbing (so reviewers
don't need to spend brain cycles) and c) update the existing codebase
in few large commits?
None of which seems like a blocker for this patch IMHO. Although I can
produce a follow-up patch or two addressing some of the
inconsistencies.
-Emil
More information about the igt-dev
mailing list