| Message ID | 20161209141944.22121-2-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On Fri, Dec 09, 2016 at 02:54:34PM +0000, Emil Velikov wrote: > 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. 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
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 17e941a533bd..42a17ea5d801 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -115,11 +115,15 @@ static int drm_getunique(struct drm_device *dev, void *data, struct drm_unique *u = data; struct drm_master *master = file_priv->master; + mutex_lock(&master->dev->master_mutex); if (u->unique_len >= master->unique_len) { - if (copy_to_user(u->unique, master->unique, master->unique_len)) + if (copy_to_user(u->unique, master->unique, master->unique_len)) { + mutex_unlock(&master->dev->master_mutex); return -EFAULT; + } } u->unique_len = master->unique_len; + mutex_unlock(&master->dev->master_mutex); return 0; } @@ -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); return retcode; } @@ -528,7 +534,7 @@ EXPORT_SYMBOL(drm_ioctl_permit); static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_VERSION, drm_version, DRM_UNLOCKED|DRM_RENDER_ALLOW|DRM_CONTROL_ALLOW), - DRM_IOCTL_DEF(DRM_IOCTL_GET_UNIQUE, drm_getunique, 0), + DRM_IOCTL_DEF(DRM_IOCTL_GET_UNIQUE, drm_getunique, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_GET_MAGIC, drm_getmagic, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_IRQ_BUSID, drm_irq_by_busid, DRM_MASTER|DRM_ROOT_ONLY), DRM_IOCTL_DEF(DRM_IOCTL_GET_MAP, drm_legacy_getmap_ioctl, DRM_UNLOCKED), @@ -536,7 +542,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_GET_STATS, drm_getstats, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_GET_CAP, drm_getcap, DRM_UNLOCKED|DRM_RENDER_ALLOW), DRM_IOCTL_DEF(DRM_IOCTL_SET_CLIENT_CAP, drm_setclientcap, 0), - DRM_IOCTL_DEF(DRM_IOCTL_SET_VERSION, drm_setversion, DRM_MASTER), + DRM_IOCTL_DEF(DRM_IOCTL_SET_VERSION, drm_setversion, DRM_UNLOCKED | DRM_MASTER), DRM_IOCTL_DEF(DRM_IOCTL_SET_UNIQUE, drm_invalid_op, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), DRM_IOCTL_DEF(DRM_IOCTL_BLOCK, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h index 610223b0481b..155588eb8ccf 100644 --- a/include/drm/drm_auth.h +++ b/include/drm/drm_auth.h @@ -33,10 +33,7 @@ * * @refcount: Refcount for this master object. * @dev: Link back to the DRM device - * @unique: Unique identifier: e.g. busid. Protected by drm_global_mutex. - * @unique_len: Length of unique field. Protected by drm_global_mutex. - * @magic_map: Map of used authentication tokens. Protected by struct_mutex. - * @lock: DRI lock information. + * @lock: DRI1 lock information. * @driver_priv: Pointer to driver-private information. * * Note that master structures are only relevant for the legacy/primary device @@ -45,8 +42,20 @@ struct drm_master { struct kref refcount; struct drm_device *dev; + /** + * @unique: Unique identifier: e.g. busid. Protected by struct + * &drm_device master_mutex. + */ char *unique; + /** + * @unique_len: Length of unique field. Protected by struct &drm_device + * master_mutex. + */ int unique_len; + /** + * @magic_map: Map of used authentication tokens. Protected by struct + * &drm_device master_mutex. + */ struct idr magic_map; struct drm_lock_data lock; void *driver_priv;
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> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_ioctl.c | 12 +++++++++--- include/drm/drm_auth.h | 17 +++++++++++++---- 2 files changed, 22 insertions(+), 7 deletions(-)