[PATCH v2] xf86drm: Add drmIsMaster()

[Daniel Vetter]

On Thu, Jan 24, 2019 at 09:56:51AM +1300, Christopher James Halse Rogers wrote:
> On 24 January 2019 6:18:32 am NZDT, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> >On Wed, 23 Jan 2019 at 04:39, Christopher James Halse Rogers
> ><christopher.halse.rogers at canonical.com> wrote:
> >>
> >> We can't use drmSetMaster to query whether or not a drm fd is master
> >> because it requires CAP_SYS_ADMIN, even if the fd *is* a master fd.
> >>
> >> Pick DRM_IOCTL_MODE_ATTACHMODE as a long-deprecated ioctl that is
> >> DRM_MASTER but not DRM_ROOT_ONLY as the probe by which we can detect
> >> whether or not the fd is master.
> >>
> >> This is useful for code that might get master by open()ing the drm
> >device
> >> while no other master exists, but can't call drmSetMaster itself
> >because
> >> it's not running as root or is in a container, where container-root
> >isn't
> >> real-root.
> >>
> >> v2: Use the AUTH_MAGIC request rather than MODE_ATTACHMODE, as it's
> >more
> >>     clearly related to master status.
> >>
> >> Signed-off-by: Christopher James Halse Rogers
> ><christopher.halse.rogers at canonical.com>
> >> ---
> >>  xf86drm.c | 15 +++++++++++++++
> >>  xf86drm.h |  2 ++
> >>  2 files changed, 17 insertions(+)
> >>
> >> diff --git a/xf86drm.c b/xf86drm.c
> >> index 10df682b..adee5bd9 100644
> >> --- a/xf86drm.c
> >> +++ b/xf86drm.c
> >> @@ -2741,6 +2741,21 @@ drm_public int drmDropMaster(int fd)
> >>          return drmIoctl(fd, DRM_IOCTL_DROP_MASTER, NULL);
> >>  }
> >>
> >> +drm_public bool drmIsMaster(int fd)
> >> +{
> >> +        /* Detect master by attempting something that requires
> >master.
> >> +         *
> >> +         * Authenticating magic tokens requires master and 0 is
> >> +         * guaranteed to be an invalid magic number. Attempting this
> >on
> >> +         * a master fd will fail therefore fail with EINVAL because
> >0 is
> >> +         * invalid.
> >> +         *
> >> +         * A non-master fd will fail with EACCESS, as the kernel
> >checks for
> >> +         * master before attempting to do anything else.
> >> +         */
> >> +        return drmAuthMagic(fd, 0) == EINVAL;
> >What magic value is valid, is a DRM implementation detail, which we
> >don't need to depend upon.
> >
> >Instead we can check for EACCES, since we care if we have permissions
> >- aka are we master.
> >The function returns a negative errno, so I'd make this a:
> >
> >        return drmAuthMagic(fd, 0) != -EACCES;
> >
> >If you and Daniel agree, I'll squash this locally and push.
> 
> That's a much better idea, thanks!

I don't like checks for "something else happened", I much prefer checking
for something specific. Hence == -EINVAL over != EACCESS. But I guess in
this case here it doesn't matter.

We just need to make sure that the igt tests both ways, to make sure we'll
never ever break this. I'd still prefer the current version, imo easier to
write an igt to make sure it keeps working.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch