[Intel-gfx] [PATCH 2/7] drm: Protect master->unique with dev->master_mutex

[Daniel Vetter]

On Fri, Dec 09, 2016 at 02:54:34PM +0000, Emil Velikov wrote:
> On 9 December 2016 at 14:19, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> > No one looks at the major/minor versions except the unique/busid
> > stuff. If we protect that with the master_mutex (since it also affects
> > the unique of each master, oh well) we can mark these two IOCTL with
> > DRM_UNLOCKED.
> >
> > While doing this I realized that the comment for the magic_map is
> > outdated, I've forgotten to update it in:
> >
> > commit d2b34ee62b409a03c6fe43c07b779983be51d017
> > Author: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Date:   Fri Jun 17 09:33:21 2016 +0200
> >
> >     drm: Protect authmagic with master_mutex
> >
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Emil Velikov <emil.l.velikov at gmail.com>
> There's a comment/wild idea below, but the patch itself is
> 
> Reviewed-by: Emil Velikov <emil.l.velikov at gmail.com>
> 
> 
> > @@ -340,6 +344,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
> >         struct drm_set_version *sv = data;
> >         int if_version, retcode = 0;
> >
> > +       mutex_lock(&dev->master_mutex);
> >         if (sv->drm_di_major != -1) {
> >                 if (sv->drm_di_major != DRM_IF_MAJOR ||
> >                     sv->drm_di_minor < 0 || sv->drm_di_minor > DRM_IF_MINOR) {
> > @@ -374,6 +379,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
> >         sv->drm_di_minor = DRM_IF_MINOR;
> >         sv->drm_dd_major = dev->driver->major;
> >         sv->drm_dd_minor = dev->driver->minor;
> > +       mutex_unlock(&dev->master_mutex);
> >
> Was going to shout "error paths" only to realise that we clobber the
> user provided storage, even on error. Realistically one could use that
> info, but from a _very_ quick look I did see any.
> 
> Seems like we have all the "used by KMS drivers" ioctls converted to
> DRM_UNLOCKED, so I'm just wondering:
> Is it worth, respinning things to:
>  - annotate the legacy-only ioctls (mostly as sanity-check) and do
> global lock on those, dropping any DRM_UNLOCKED references.

Yeah, that would make sense as a follow-up after this series. There's
already a patch to enforce lockless behaviour for all new ioctls. We'd
need a DRM_LEGACY_LOCKED to annotate the few places that still need the
drm bkl.

>  - if !legacy-only ioctl, bail out from drm_ioctl() (again, mostly a
> sanity check) and dropping the boiler plate from the individual
> ioctls.
> Not too sure if this one is correct though.

The boiler-plate isn't the same everywhere, sometimes it checks
DRIVER_LEGACY, sometimes also the nouveau special case and the return code
isn't the same everywhere (I'm not sure where that all matters). My
preference for this would be to leave it as is.
-Daniel

> 
> if (drm_core_check_feature(dev, DRIVER_MODESET))
>    return 0;
> 
> Thanks
> Emil

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch