| Message ID | 1406129207-1302-8-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 23, 2014 at 05:26:42PM +0200, David Herrmann wrote: > The drm_file->is_master field is redundant as it's equivalent to: > drm_file->master && drm_file->master == drm_file->minor->master > > 1) "=>" > Whenever we set drm_file->is_master, we also set: > drm_file->minor->master = drm_file->master; > > Whenever we clear drm_file->is_master, we also call: > drm_master_put(&drm_file->minor->master); > which implicitly clears it to NULL. > > 2) "<=" > minor->master cannot be set if it is non-NULL. Therefore, it stays as > is unless a file drops it. > > If minor->master is NULL, it is only set by places that also adjust > drm_file->is_master. > > Therefore, we can safely drop is_master and replace it by an inline helper > that matches: > drm_file->master && drm_file->master == drm_file->minor->master > > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> Docbook for drm_is_master is missing, otherwise Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> And one question below which doesn't really matter for this patch here. See below -Daniel [snip] > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index d91e09f..e1bb585 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -387,8 +387,6 @@ struct drm_prime_file_private { > struct drm_file { > unsigned always_authenticated :1; > unsigned authenticated :1; > - /* Whether we're master for a minor. Protected by master_mutex */ > - unsigned is_master :1; > /* true when the client has asked us to expose stereo 3D mode flags */ > unsigned stereo_allowed :1; > /* > @@ -1034,7 +1032,7 @@ struct drm_device { > /** \name Locks */ > /*@{ */ > struct mutex struct_mutex; /**< For others */ > - struct mutex master_mutex; /**< For drm_minor::master and drm_file::is_master */ > + struct mutex master_mutex; /**< For drm_minor::master */ > /*@} */ > > /** \name Usage Counters */ > @@ -1172,6 +1170,11 @@ static inline bool drm_is_primary_client(const struct drm_file *file_priv) > return file_priv->minor->type == DRM_MINOR_LEGACY; > } > Docbook here please ... > +static inline bool drm_is_master(const struct drm_file *file) > +{ Hm, we don't have any means to synchronize is_master checks with concurrent ioctls and stuff. Do we care? Orthogonal issue really. > + return file->master && file->master == file->minor->master; > +} > + > /******************************************************************/ > /** \name Internal function definitions */ > /*@{*/ > -- > 2.0.2 >
Hi On Thu, Jul 24, 2014 at 11:52 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Jul 23, 2014 at 05:26:42PM +0200, David Herrmann wrote: >> The drm_file->is_master field is redundant as it's equivalent to: >> drm_file->master && drm_file->master == drm_file->minor->master >> >> 1) "=>" >> Whenever we set drm_file->is_master, we also set: >> drm_file->minor->master = drm_file->master; >> >> Whenever we clear drm_file->is_master, we also call: >> drm_master_put(&drm_file->minor->master); >> which implicitly clears it to NULL. >> >> 2) "<=" >> minor->master cannot be set if it is non-NULL. Therefore, it stays as >> is unless a file drops it. >> >> If minor->master is NULL, it is only set by places that also adjust >> drm_file->is_master. >> >> Therefore, we can safely drop is_master and replace it by an inline helper >> that matches: >> drm_file->master && drm_file->master == drm_file->minor->master >> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > > Docbook for drm_is_master is missing, otherwise Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > And one question below which doesn't really matter for this patch here. > See below Fixed, thanks! > [snip] > >> diff --git a/include/drm/drmP.h b/include/drm/drmP.h >> index d91e09f..e1bb585 100644 >> --- a/include/drm/drmP.h >> +++ b/include/drm/drmP.h >> @@ -387,8 +387,6 @@ struct drm_prime_file_private { >> struct drm_file { >> unsigned always_authenticated :1; >> unsigned authenticated :1; >> - /* Whether we're master for a minor. Protected by master_mutex */ >> - unsigned is_master :1; >> /* true when the client has asked us to expose stereo 3D mode flags */ >> unsigned stereo_allowed :1; >> /* >> @@ -1034,7 +1032,7 @@ struct drm_device { >> /** \name Locks */ >> /*@{ */ >> struct mutex struct_mutex; /**< For others */ >> - struct mutex master_mutex; /**< For drm_minor::master and drm_file::is_master */ >> + struct mutex master_mutex; /**< For drm_minor::master */ >> /*@} */ >> >> /** \name Usage Counters */ >> @@ -1172,6 +1170,11 @@ static inline bool drm_is_primary_client(const struct drm_file *file_priv) >> return file_priv->minor->type == DRM_MINOR_LEGACY; >> } >> > > Docbook here please ... >> +static inline bool drm_is_master(const struct drm_file *file) >> +{ > > Hm, we don't have any means to synchronize is_master checks with > concurrent ioctls and stuff. Do we care? Orthogonal issue really. We don't.. My drm-master reworks contains a comment about that. It's not really problematic as all ioctls run for a determinate time in kernel-code except for drm_read(), but drm_read() is per-file, not per-device, so we're fine. However, with unfortunate timings, we might really end up with problems. vmwgfx solves this by using separate locks and verifying them against the current master. it's not perfect and I'm not sure I like it better than no locks, but at least they were aware of the problem. Btw., the only thing I know how to solve it properly is to make "master_mutex" an rwlock and all f_op entries take the read-lock, while master modifications take the write-lock. Not sure it's worth it, though. Thanks David >> + return file->master && file->master == file->minor->master; >> +} >> + >> /******************************************************************/ >> /** \name Internal function definitions */ >> /*@{*/ >> -- >> 2.0.2 >> > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Thu, Jul 24, 2014 at 11:38:28PM +0200, David Herrmann wrote: > On Thu, Jul 24, 2014 at 11:52 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Jul 23, 2014 at 05:26:42PM +0200, David Herrmann wrote: > >> +static inline bool drm_is_master(const struct drm_file *file) > >> +{ > > > > Hm, we don't have any means to synchronize is_master checks with > > concurrent ioctls and stuff. Do we care? Orthogonal issue really. > > We don't.. My drm-master reworks contains a comment about that. It's > not really problematic as all ioctls run for a determinate time in > kernel-code except for drm_read(), but drm_read() is per-file, not > per-device, so we're fine. However, with unfortunate timings, we might > really end up with problems. > > vmwgfx solves this by using separate locks and verifying them against > the current master. it's not perfect and I'm not sure I like it better > than no locks, but at least they were aware of the problem. > > Btw., the only thing I know how to solve it properly is to make > "master_mutex" an rwlock and all f_op entries take the read-lock, > while master modifications take the write-lock. Not sure it's worth > it, though. Imo that's terrible. And I'm not even sure we need to care, e.g. if we do a master switch to a different compositor any ioctl the new compositor does will grab some locks, which will force the old ioctl to complete. Well mostly, and only if we don't do lockless tricks or lock-dropping and it's racy. I guess a proper fix would be to wait for all ioctls on a device to complete. The vfs doesn't have any cool infrastructure for this as part of the general revoke work that we could reuse? -Daniel
Hi On Fri, Jul 25, 2014 at 9:56 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Jul 24, 2014 at 11:38:28PM +0200, David Herrmann wrote: >> On Thu, Jul 24, 2014 at 11:52 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >> > On Wed, Jul 23, 2014 at 05:26:42PM +0200, David Herrmann wrote: >> >> +static inline bool drm_is_master(const struct drm_file *file) >> >> +{ >> > >> > Hm, we don't have any means to synchronize is_master checks with >> > concurrent ioctls and stuff. Do we care? Orthogonal issue really. >> >> We don't.. My drm-master reworks contains a comment about that. It's >> not really problematic as all ioctls run for a determinate time in >> kernel-code except for drm_read(), but drm_read() is per-file, not >> per-device, so we're fine. However, with unfortunate timings, we might >> really end up with problems. >> >> vmwgfx solves this by using separate locks and verifying them against >> the current master. it's not perfect and I'm not sure I like it better >> than no locks, but at least they were aware of the problem. >> >> Btw., the only thing I know how to solve it properly is to make >> "master_mutex" an rwlock and all f_op entries take the read-lock, >> while master modifications take the write-lock. Not sure it's worth >> it, though. > > Imo that's terrible. And I'm not even sure we need to care, e.g. if we do > a master switch to a different compositor any ioctl the new compositor > does will grab some locks, which will force the old ioctl to complete. > > Well mostly, and only if we don't do lockless tricks or lock-dropping and > it's racy. There is always a race! Like this: CPU A: 1: enter drm_ioctl() 2: check file->is_master 3: enter drm_some_ioctl() 4: acquire some DRM internal locks If CPU B acquires DRM-Master between step 2 and 3, CPU A might execute an ioctl with an arbitrary delay. Maybe CPU B even executed some ioctl after acquiring DRM-Master before CPU A had the chance to enter the ioctl it's about to execute. Not that I care much, but we have to remember that those races always exist. Given that DRM-Master is privileged, this is not really high-priority to fix.. > I guess a proper fix would be to wait for all ioctls on a device to > complete. The vfs doesn't have any cool infrastructure for this as part of > the general revoke work that we could reuse? What the VFS rework does is this: if (!atomic_inc_unless_negative(file->sth)) return -ENODEV; ret = file->f_op->some_op(); atomic_dec(file->sth); return ret; That is, it wraps all calls to a file-operation with an atomic-counter. However, there's only one counter per open-file, not one per file-operation. If we'd want that for DRM-Master, we couldn't use it as otherwise all file-operations would be blocked. Furthermore, VFS only allows disabling an open-file. Once disabled, you cannot enable it again. So I don't think a read/write lock is a bad idea. RCU doesn't work as our ioctls are way to heavy for rcu_read_lock(), SRCU is basically rw-sem for our use-case. A hand-crafted atomic counter is also equivalent to rw-sem. So yeah, it might lock nasty, but any other solution will just hide the fact that we have a read/write lock. Anyhow, I'm not working on a fix, so if no-one else looks at it, we can just ignore it. Thanks David
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 1ccf5cb..dec0c77 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3188,7 +3188,7 @@ int drm_mode_getfb(struct drm_device *dev, r->bpp = fb->bits_per_pixel; r->pitch = fb->pitches[0]; if (fb->funcs->create_handle) { - if (file_priv->is_master || capable(CAP_SYS_ADMIN) || + if (drm_is_master(file_priv) || capable(CAP_SYS_ADMIN) || drm_is_control_client(file_priv)) { ret = fb->funcs->create_handle(fb, file_priv, &r->handle); diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 0cc1827..7aa8121 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -307,7 +307,7 @@ static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv) return -EACCES; /* MASTER is only for master or control clients */ - if (unlikely((flags & DRM_MASTER) && !file_priv->is_master && + if (unlikely((flags & DRM_MASTER) && !drm_is_master(file_priv) && !drm_is_control_client(file_priv))) return -EACCES; diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index afba0bf..f65087e 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -286,7 +286,6 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor) goto out_close; } - priv->is_master = 1; /* take another reference for the copy in the local file priv */ priv->master = drm_master_get(priv->minor->master); priv->authenticated = 1; @@ -452,7 +451,7 @@ int drm_release(struct inode *inode, struct file *filp) mutex_lock(&dev->master_mutex); - if (file_priv->is_master) { + if (drm_is_master(file_priv)) { struct drm_master *master = file_priv->master; struct drm_file *temp; @@ -488,7 +487,6 @@ int drm_release(struct inode *inode, struct file *filp) /* drop the master reference held by the file priv */ if (file_priv->master) drm_master_put(&file_priv->master); - file_priv->is_master = 0; mutex_unlock(&dev->master_mutex); mutex_lock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c index f645268..786401c 100644 --- a/drivers/gpu/drm/drm_lock.c +++ b/drivers/gpu/drm/drm_lock.c @@ -111,7 +111,7 @@ int drm_lock(struct drm_device *dev, void *data, struct drm_file *file_priv) /* don't set the block all signals on the master process for now * really probably not the correct answer but lets us debug xkb * xserver for now */ - if (!file_priv->is_master) { + if (!drm_is_master(file_priv)) { sigemptyset(&dev->sigmask); sigaddset(&dev->sigmask, SIGSTOP); sigaddset(&dev->sigmask, SIGTSTP); diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 233ea20..18c9b3d 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -177,7 +177,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, int ret = 0; mutex_lock(&dev->master_mutex); - if (file_priv->is_master) + if (drm_is_master(file_priv)) goto out_unlock; if (file_priv->minor->master) { @@ -191,13 +191,10 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, } file_priv->minor->master = drm_master_get(file_priv->master); - file_priv->is_master = 1; if (dev->driver->master_set) { ret = dev->driver->master_set(dev, file_priv, false); - if (unlikely(ret != 0)) { - file_priv->is_master = 0; + if (unlikely(ret != 0)) drm_master_put(&file_priv->minor->master); - } } out_unlock: @@ -211,7 +208,7 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data, int ret = -EINVAL; mutex_lock(&dev->master_mutex); - if (!file_priv->is_master) + if (!drm_is_master(file_priv)) goto out_unlock; if (!file_priv->minor->master) @@ -221,7 +218,6 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data, if (dev->driver->master_drop) dev->driver->master_drop(dev, file_priv, false); drm_master_put(&file_priv->minor->master); - file_priv->is_master = 0; out_unlock: mutex_unlock(&dev->master_mutex); diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 60998fc..2dd19da 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1260,7 +1260,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, flags = 0; if (args->flags & I915_EXEC_SECURE) { - if (!file->is_master || !capable(CAP_SYS_ADMIN)) + if (!drm_is_master(file) || !capable(CAP_SYS_ADMIN)) return -EPERM; flags |= I915_DISPATCH_SECURE; @@ -1369,7 +1369,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, ret = i915_parse_cmds(ring, batch_obj, args->batch_start_offset, - file->is_master); + drm_is_master(file)); if (ret) goto err; diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c index 18b54ac..63c4d6f 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c @@ -990,7 +990,7 @@ static struct vmw_master *vmw_master_check(struct drm_device *dev, if (unlikely(ret != 0)) return ERR_PTR(-ERESTARTSYS); - if (file_priv->is_master) { + if (drm_is_master(file_priv)) { mutex_unlock(&dev->master_mutex); return NULL; } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d91e09f..e1bb585 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -387,8 +387,6 @@ struct drm_prime_file_private { struct drm_file { unsigned always_authenticated :1; unsigned authenticated :1; - /* Whether we're master for a minor. Protected by master_mutex */ - unsigned is_master :1; /* true when the client has asked us to expose stereo 3D mode flags */ unsigned stereo_allowed :1; /* @@ -1034,7 +1032,7 @@ struct drm_device { /** \name Locks */ /*@{ */ struct mutex struct_mutex; /**< For others */ - struct mutex master_mutex; /**< For drm_minor::master and drm_file::is_master */ + struct mutex master_mutex; /**< For drm_minor::master */ /*@} */ /** \name Usage Counters */ @@ -1172,6 +1170,11 @@ static inline bool drm_is_primary_client(const struct drm_file *file_priv) return file_priv->minor->type == DRM_MINOR_LEGACY; } +static inline bool drm_is_master(const struct drm_file *file) +{ + return file->master && file->master == file->minor->master; +} + /******************************************************************/ /** \name Internal function definitions */ /*@{*/
The drm_file->is_master field is redundant as it's equivalent to: drm_file->master && drm_file->master == drm_file->minor->master 1) "=>" Whenever we set drm_file->is_master, we also set: drm_file->minor->master = drm_file->master; Whenever we clear drm_file->is_master, we also call: drm_master_put(&drm_file->minor->master); which implicitly clears it to NULL. 2) "<=" minor->master cannot be set if it is non-NULL. Therefore, it stays as is unless a file drops it. If minor->master is NULL, it is only set by places that also adjust drm_file->is_master. Therefore, we can safely drop is_master and replace it by an inline helper that matches: drm_file->master && drm_file->master == drm_file->minor->master Signed-off-by: David Herrmann <dh.herrmann@gmail.com> --- drivers/gpu/drm/drm_crtc.c | 2 +- drivers/gpu/drm/drm_drv.c | 2 +- drivers/gpu/drm/drm_fops.c | 4 +--- drivers/gpu/drm/drm_lock.c | 2 +- drivers/gpu/drm/drm_stub.c | 10 +++------- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 ++-- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 2 +- include/drm/drmP.h | 9 ++++++--- 8 files changed, 16 insertions(+), 19 deletions(-)