Re: [igt-dev] [PATCH i-g-t v3] ts/core_setmaster: new test for drop/set master semantics

[Petri Latvala <petri.latvala@intel.com>]

On Mon, Mar 02, 2020 at 05:33:14PM +0000, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@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@intel.com>
> Signed-off-by: Emil Velikov <emil.velikov@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@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?


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


> +	 * - 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.


> +	 */
> +	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))


-- 
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev