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

From: Emil Velikov <emil.l.velikov@gmail.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 2/7] drm: Protect master->unique with dev->master_mutex
Date: Fri, 9 Dec 2016 14:54:34 +0000	[thread overview]
Message-ID: <CACvgo51PFdmbLhbqvdrqO3cVKO4jmYLHW61xgZCB5x9=FTPBmA@mail.gmail.com> (raw)
In-Reply-To: <20161209141944.22121-2-daniel.vetter@ffwll.ch>

On 9 December 2016 at 14:19, Daniel Vetter <daniel.vetter@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@ffwll.ch>
> Date:   Fri Jun 17 09:33:21 2016 +0200
>
>     drm: Protect authmagic with master_mutex
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
There's a comment/wild idea below, but the patch itself is

Reviewed-by: Emil Velikov <emil.l.velikov@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.
 - 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.

if (drm_core_check_feature(dev, DRIVER_MODESET))
   return 0;

Thanks
Emil
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx