[igt-dev] [PATCH i-g-t] lib/core_auth: mount namespace magic to make the test work everywhere

[Daniel Vetter]

On Fri, Feb 15, 2019 at 09:51:35AM +0000, Chris Wilson wrote:
> Quoting Daniel Vetter (2019-02-13 20:36:11)
> > We're creating our own namespace and then create a copy of the chardev
> > that anyone can access before dropping root. Should hopefully work on
> > any system.
> > 
> > This way we're also guaranteed to open the right device again.
> > 
> > v2: mount(2) instead of mount(3).
> > 
> > Cc: Emil Velikov <emil.velikov at collabora.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > ---
> >  tests/core_auth.c | 35 +++++++++++++++++++++--------------
> >  1 file changed, 21 insertions(+), 14 deletions(-)
> > 
> > diff --git a/tests/core_auth.c b/tests/core_auth.c
> > index 0b9073cb0fce..bc2754ec30af 100644
> > --- a/tests/core_auth.c
> > +++ b/tests/core_auth.c
> > @@ -36,6 +36,8 @@
> >  #include <fcntl.h>
> >  #include <inttypes.h>
> >  #include <errno.h>
> > +#include <sched.h>
> > +#include <sys/mount.h>
> >  #include <sys/stat.h>
> >  #include <sys/ioctl.h>
> >  #include <sys/time.h>
> > @@ -243,17 +245,24 @@ static void test_unauth_vs_render(int master)
> >  {
> >         int slave;
> >         uint32_t handle;
> > +       struct stat statbuf;
> > +       bool has_render;
> >  
> > -       /*
> > -        * FIXME: when drm_open_driver() fails to open() a node (insufficient
> > -        * permissions or otherwise, it will igt_skip.
> > -        * As of today, igt_skip and igt_fork do not work together.
> > -        */
> > -       slave = __drm_open_driver(DRIVER_ANY);
> > -       /*
> > -        * FIXME: relate to the master fd passed with the above open and fix
> > -        * all of IGT.
> > -        */
> > +       /* need to check for render nodes before we wreak the filesystem */
> > +       has_render = has_render_node(master);
> > +
> > +       /* create a card node matching master which (only) we can access as
> > +        * non-root */
> > +       do_or_die(fstat(master, &statbuf));
> > +       do_or_die(unshare(CLONE_NEWNS));
> 
> New mounts will occur in our private namespace, invisible to the rest of
> the system.
> 
> > +       do_or_die(mount(NULL, "/", NULL, MS_PRIVATE | MS_REC, NULL));
> 
> Make future modifications to / and beyond private to our namespace.
> 
> > +       do_or_die(mount("none", "/dev/dri", "tmpfs", 0, NULL));
> 
> Replace "/dev/dri" with an empty filesystem.
> 
> > +       umask(0);
> > +       do_or_die(mknod("/dev/dri/card", S_IFCHR | 0777, statbuf.st_rdev));
> 
> Execute? What are you planning next? ;)

More "set all the bits" kind of programming. I'll change to 0666.

> And make exactly one entry in that new fs, a device node matching the
> original.
> 
> > +
> > +       igt_drop_root();
> > +
> > +       slave = open("/dev/dri/card", O_RDWR);
> 
> And the unusual name doesn't matter because we open it directly :)
> 
> Since master is still open, this will be effectively a fresh open and a
> slave to exactly the same device node as master opened.
> 
> Looks fancy, but why didn't we just do a gem_reopen_driver(), i.e.
> open(/proc/self/fd/$master)?

I thought this would amount to a dup(), but looking at kernel code it's
really just a normal symbolic link. That still runs into the same
permission problem as drm_open_any(). What would work instead is dropping
CAP_SYS_ADMIN (we only need CAP_FOWNER to dodge the file permission
check). But I think fully dropping the root is the more genuine testcase
and less tied to implementation details of what exactly we check and when.

> Anyway, I like the private /dev/dri idea and I think it will come in
> very useful in cases where we need to check a pristine system.

Yeah, I think this might have uses elsewhere. For a library version we
probably want to use overlayfs. But that means mounting a tmpfs somewhere
else first (in a $tmpfile directory), because overlayfs needs some scratch
space. But then you could freely change files/permissions/whatever,
anywhere. Soemthing like

	igt_private_mountspace(); /* unshare + mount(MS_PRIVATE) */
	igt_private_directory("/dev/dri"); /* drops an overlayfs on /dev/dri */

	chmod(); /* or whatever else you feel like */

But felt like overkill for just this one here.

> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>

Thanks for your review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch