Messages in this thread | | | From | Desmond Cheong Zhi Xi <> | Subject | Re: [PATCH 1/2] drm: avoid races with modesetting rights | Date | Sun, 15 Aug 2021 15:10:16 +0800 |
| |
On 13/8/21 11:49 pm, Daniel Vetter wrote: > On Fri, Aug 13, 2021 at 04:54:49PM +0800, Desmond Cheong Zhi Xi wrote: >> In drm_client_modeset.c and drm_fb_helper.c, >> drm_master_internal_{acquire,release} are used to avoid races with DRM >> userspace. These functions hold onto drm_device.master_mutex while >> committing, and bail if there's already a master. >> >> However, ioctls can still race between themselves. A >> time-of-check-to-time-of-use error can occur if an ioctl that changes >> the modeset has its rights revoked after it validates its permissions, >> but before it completes. >> >> There are three ioctls that can affect modesetting permissions: >> >> - DROP_MASTER ioctl removes rights for a master and its leases >> >> - REVOKE_LEASE ioctl revokes rights for a specific lease >> >> - SET_MASTER ioctl sets the device master if the master role hasn't >> been acquired yet >> >> All these races can be avoided by introducing an SRCU that acts as a >> barrier for ioctls that can change modesetting permissions. Processes >> that perform modesetting should hold a read lock on the new >> drm_device.master_barrier_srcu, and ioctls that change these >> permissions should call synchronize_srcu before returning. >> >> This ensures that any process that might have seen old permissions are >> flushed out before DROP_MASTER/REVOKE_LEASE/SET_MASTER ioctls return >> to userspace. >> >> Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> > > This looks pretty solid, but I think there's one gap where we can still > race. Scenario. > > Process A has a drm fd with master rights and two threads: > - thread 1 does a long-running display operation (like a modeset or > whatever) > - thread 2 does a drop-master > > Then we start a new process B, which acquires master in drm_open (there is > no other one left). This is like setmaster ioctl, but your > DRM_MASTER_FLUSH bit doesn't work there. >
Ah, I see the race. I think a good place to plug this would be in drm_master_open using the drm_master_flush (or drm_master_unlock_and_flush) that you suggested below.
> The other thing is that for modeset stuff (which this all is) srcu is > probably massive overkill, and a simple rwsem should be good enough too. > Maybe even better, since the rwsem guarantees that no new reader can start > once you try to acquire the write side. >
Makes sense, I'll switch to a rwsem then.
> Finally, and this is a bit a bikeshed: I don't like much how > DRM_MASTER_FLUSH leaks the need of these very few places into the very > core drm_ioctl function. One idea I had was to use task_work in a special > function, roughly > > void master_flush() > { > down_write(master_rwsem); > up_write(master_rwms); > } > void drm_master_flush() > { > init_task_work(fpriv->master_flush_work, master_flush) > task_work_add(fpriv->master_flush_work); > /* if task_work_add fails we're exiting, at which point the lack > * of master flush doesn't matter); > } > > And maybe put a comment above the function explaining why and how this > works. > > We could even do a drm_master_unlock_and_flush helper, since that's really > what everyone wants, and it would make it very clear which master state > changes need this flush. Instead of setting a flag bit in an ioctl table > very far away ... > > Thoughts? > -Daniel >
Sounds good. I wasn't aware of the task_work_add mechanism to queue work before the task returns to usermode, but this seems like a more explicit way to flush.
Thanks for the feedback, Daniel. I'll fix this up in a v2.
>> --- >> drivers/gpu/drm/drm_auth.c | 17 ++++++++++++++--- >> drivers/gpu/drm/drm_client_modeset.c | 10 ++++++---- >> drivers/gpu/drm/drm_drv.c | 2 ++ >> drivers/gpu/drm/drm_fb_helper.c | 20 ++++++++++++-------- >> drivers/gpu/drm/drm_internal.h | 5 +++-- >> drivers/gpu/drm/drm_ioctl.c | 25 +++++++++++++++++++++---- >> include/drm/drm_device.h | 11 +++++++++++ >> include/drm/drm_ioctl.h | 7 +++++++ >> 8 files changed, 76 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c >> index 60a6b21474b1..004506608e76 100644 >> --- a/drivers/gpu/drm/drm_auth.c >> +++ b/drivers/gpu/drm/drm_auth.c >> @@ -29,6 +29,7 @@ >> */ >> >> #include <linux/slab.h> >> +#include <linux/srcu.h> >> >> #include <drm/drm_auth.h> >> #include <drm/drm_drv.h> >> @@ -448,21 +449,31 @@ void drm_master_put(struct drm_master **master) >> EXPORT_SYMBOL(drm_master_put); >> >> /* Used by drm_client and drm_fb_helper */ >> -bool drm_master_internal_acquire(struct drm_device *dev) >> +bool drm_master_internal_acquire(struct drm_device *dev, int *idx) >> { >> + *idx = srcu_read_lock(&dev->master_barrier_srcu); >> + >> mutex_lock(&dev->master_mutex); >> if (dev->master) { >> mutex_unlock(&dev->master_mutex); >> + srcu_read_unlock(&dev->master_barrier_srcu, *idx); >> return false; >> } >> + mutex_unlock(&dev->master_mutex); >> >> return true; >> } >> EXPORT_SYMBOL(drm_master_internal_acquire); >> >> /* Used by drm_client and drm_fb_helper */ >> -void drm_master_internal_release(struct drm_device *dev) >> +void drm_master_internal_release(struct drm_device *dev, int idx) >> { >> - mutex_unlock(&dev->master_mutex); >> + srcu_read_unlock(&dev->master_barrier_srcu, idx); >> } >> EXPORT_SYMBOL(drm_master_internal_release); >> + >> +/* Used by drm_ioctl */ >> +void drm_master_flush(struct drm_device *dev) >> +{ >> + synchronize_srcu(&dev->master_barrier_srcu); >> +} >> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c >> index ced09c7c06f9..9885f36f71b7 100644 >> --- a/drivers/gpu/drm/drm_client_modeset.c >> +++ b/drivers/gpu/drm/drm_client_modeset.c >> @@ -1165,13 +1165,14 @@ int drm_client_modeset_commit(struct drm_client_dev *client) >> { >> struct drm_device *dev = client->dev; >> int ret; >> + int idx; >> >> - if (!drm_master_internal_acquire(dev)) >> + if (!drm_master_internal_acquire(dev, &idx)) >> return -EBUSY; >> >> ret = drm_client_modeset_commit_locked(client); >> >> - drm_master_internal_release(dev); >> + drm_master_internal_release(dev, idx); >> >> return ret; >> } >> @@ -1215,8 +1216,9 @@ int drm_client_modeset_dpms(struct drm_client_dev *client, int mode) >> { >> struct drm_device *dev = client->dev; >> int ret = 0; >> + int idx; >> >> - if (!drm_master_internal_acquire(dev)) >> + if (!drm_master_internal_acquire(dev, &idx)) >> return -EBUSY; >> >> mutex_lock(&client->modeset_mutex); >> @@ -1226,7 +1228,7 @@ int drm_client_modeset_dpms(struct drm_client_dev *client, int mode) >> drm_client_modeset_dpms_legacy(client, mode); >> mutex_unlock(&client->modeset_mutex); >> >> - drm_master_internal_release(dev); >> + drm_master_internal_release(dev, idx); >> >> return ret; >> } >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >> index 7a5097467ba5..c313f0674db3 100644 >> --- a/drivers/gpu/drm/drm_drv.c >> +++ b/drivers/gpu/drm/drm_drv.c >> @@ -574,6 +574,7 @@ static void drm_dev_init_release(struct drm_device *dev, void *res) >> mutex_destroy(&dev->clientlist_mutex); >> mutex_destroy(&dev->filelist_mutex); >> mutex_destroy(&dev->struct_mutex); >> + cleanup_srcu_struct(&dev->master_barrier_srcu); >> drm_legacy_destroy_members(dev); >> } >> >> @@ -612,6 +613,7 @@ static int drm_dev_init(struct drm_device *dev, >> mutex_init(&dev->filelist_mutex); >> mutex_init(&dev->clientlist_mutex); >> mutex_init(&dev->master_mutex); >> + init_srcu_struct(&dev->master_barrier_srcu); >> >> ret = drmm_add_action(dev, drm_dev_init_release, NULL); >> if (ret) >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >> index 3ab078321045..0d594bc15f18 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -1116,13 +1116,14 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info) >> struct drm_fb_helper *fb_helper = info->par; >> struct drm_device *dev = fb_helper->dev; >> int ret; >> + int idx; >> >> if (oops_in_progress) >> return -EBUSY; >> >> mutex_lock(&fb_helper->lock); >> >> - if (!drm_master_internal_acquire(dev)) { >> + if (!drm_master_internal_acquire(dev, &idx)) { >> ret = -EBUSY; >> goto unlock; >> } >> @@ -1136,7 +1137,7 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info) >> ret = setcmap_legacy(cmap, info); >> mutex_unlock(&fb_helper->client.modeset_mutex); >> >> - drm_master_internal_release(dev); >> + drm_master_internal_release(dev, idx); >> unlock: >> mutex_unlock(&fb_helper->lock); >> >> @@ -1160,9 +1161,10 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, >> struct drm_device *dev = fb_helper->dev; >> struct drm_crtc *crtc; >> int ret = 0; >> + int idx; >> >> mutex_lock(&fb_helper->lock); >> - if (!drm_master_internal_acquire(dev)) { >> + if (!drm_master_internal_acquire(dev, &idx)) { >> ret = -EBUSY; >> goto unlock; >> } >> @@ -1204,7 +1206,7 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, >> ret = -ENOTTY; >> } >> >> - drm_master_internal_release(dev); >> + drm_master_internal_release(dev, idx); >> unlock: >> mutex_unlock(&fb_helper->lock); >> return ret; >> @@ -1474,12 +1476,13 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, >> struct drm_fb_helper *fb_helper = info->par; >> struct drm_device *dev = fb_helper->dev; >> int ret; >> + int idx; >> >> if (oops_in_progress) >> return -EBUSY; >> >> mutex_lock(&fb_helper->lock); >> - if (!drm_master_internal_acquire(dev)) { >> + if (!drm_master_internal_acquire(dev, &idx)) { >> ret = -EBUSY; >> goto unlock; >> } >> @@ -1489,7 +1492,7 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, >> else >> ret = pan_display_legacy(var, info); >> >> - drm_master_internal_release(dev); >> + drm_master_internal_release(dev, idx); >> unlock: >> mutex_unlock(&fb_helper->lock); >> >> @@ -1948,6 +1951,7 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config); >> int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) >> { >> int err = 0; >> + int idx; >> >> if (!drm_fbdev_emulation || !fb_helper) >> return 0; >> @@ -1959,13 +1963,13 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) >> return err; >> } >> >> - if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) { >> + if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev, &idx)) { >> fb_helper->delayed_hotplug = true; >> mutex_unlock(&fb_helper->lock); >> return err; >> } >> >> - drm_master_internal_release(fb_helper->dev); >> + drm_master_internal_release(fb_helper->dev, idx); >> >> drm_dbg_kms(fb_helper->dev, "\n"); >> >> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h >> index 17f3548c8ed2..578fd2769913 100644 >> --- a/drivers/gpu/drm/drm_internal.h >> +++ b/drivers/gpu/drm/drm_internal.h >> @@ -142,8 +142,9 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data, >> struct drm_file *file_priv); >> int drm_master_open(struct drm_file *file_priv); >> void drm_master_release(struct drm_file *file_priv); >> -bool drm_master_internal_acquire(struct drm_device *dev); >> -void drm_master_internal_release(struct drm_device *dev); >> +bool drm_master_internal_acquire(struct drm_device *dev, int *idx); >> +void drm_master_internal_release(struct drm_device *dev, int idx); >> +void drm_master_flush(struct drm_device *dev); >> >> /* drm_sysfs.c */ >> extern struct class *drm_class; >> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c >> index be4a52dc4d6f..eb4ec3fab7d1 100644 >> --- a/drivers/gpu/drm/drm_ioctl.c >> +++ b/drivers/gpu/drm/drm_ioctl.c >> @@ -600,8 +600,10 @@ static const struct drm_ioctl_desc drm_ioctls[] = { >> DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_SET_SAREA_CTX, drm_legacy_setsareactx, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), >> DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_GET_SAREA_CTX, drm_legacy_getsareactx, DRM_AUTH), >> >> - DRM_IOCTL_DEF(DRM_IOCTL_SET_MASTER, drm_setmaster_ioctl, 0), >> - DRM_IOCTL_DEF(DRM_IOCTL_DROP_MASTER, drm_dropmaster_ioctl, 0), >> + DRM_IOCTL_DEF(DRM_IOCTL_SET_MASTER, drm_setmaster_ioctl, >> + DRM_MASTER_FLUSH), >> + DRM_IOCTL_DEF(DRM_IOCTL_DROP_MASTER, drm_dropmaster_ioctl, >> + DRM_MASTER_FLUSH), >> >> DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_ADD_CTX, drm_legacy_addctx, DRM_AUTH|DRM_ROOT_ONLY), >> DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_RM_CTX, drm_legacy_rmctx, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), >> @@ -722,7 +724,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = { >> DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_MASTER), >> DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, DRM_MASTER), >> DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, DRM_MASTER), >> - DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, DRM_MASTER), >> + DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, >> + DRM_MASTER | DRM_MASTER_FLUSH), >> }; >> >> #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls ) >> @@ -781,13 +784,17 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata, >> struct drm_file *file_priv = file->private_data; >> struct drm_device *dev = file_priv->minor->dev; >> int retcode; >> + int idx; >> >> if (drm_dev_is_unplugged(dev)) >> return -ENODEV; >> >> + if (unlikely(flags & DRM_MASTER)) >> + idx = srcu_read_lock(&dev->master_barrier_srcu); >> + >> retcode = drm_ioctl_permit(flags, file_priv); >> if (unlikely(retcode)) >> - return retcode; >> + goto release_master; >> >> /* Enforce sane locking for modern driver ioctls. */ >> if (likely(!drm_core_check_feature(dev, DRIVER_LEGACY)) || >> @@ -798,6 +805,16 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata, >> retcode = func(dev, kdata, file_priv); >> mutex_unlock(&drm_global_mutex); >> } >> + >> +release_master: >> + if (unlikely(flags & DRM_MASTER)) >> + srcu_read_unlock(&dev->master_barrier_srcu, idx); >> + /* After flushing, processes are guaranteed to see the new master/lease >> + * permissions, and any process which might have seen the old >> + * permissions is guaranteed to have finished. >> + */ >> + if (unlikely(flags & DRM_MASTER_FLUSH)) >> + drm_master_flush(dev); >> return retcode; >> } >> EXPORT_SYMBOL(drm_ioctl_kernel); >> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h >> index 604b1d1b2d72..0ac5fdb375f8 100644 >> --- a/include/drm/drm_device.h >> +++ b/include/drm/drm_device.h >> @@ -111,6 +111,17 @@ struct drm_device { >> */ >> struct drm_master *master; >> >> + /** >> + * @master_barrier_srcu: >> + * >> + * Used to synchronize modesetting rights between multiple users. Users >> + * that can change the modeset or display state must hold an >> + * srcu_read_lock() on @master_barrier_srcu, and ioctls that can change >> + * modesetting rights should call synchronize_srcu() before returning >> + * to userspace. >> + */ >> + struct srcu_struct master_barrier_srcu; >> + >> /** >> * @driver_features: per-device driver features >> * >> diff --git a/include/drm/drm_ioctl.h b/include/drm/drm_ioctl.h >> index afb27cb6a7bd..13a68cdcea36 100644 >> --- a/include/drm/drm_ioctl.h >> +++ b/include/drm/drm_ioctl.h >> @@ -130,6 +130,13 @@ enum drm_ioctl_flags { >> * not set DRM_AUTH because they do not require authentication. >> */ >> DRM_RENDER_ALLOW = BIT(5), >> + /** >> + * @DRM_MASTER_FLUSH: >> + * >> + * This must be set for any ioctl which can change the modesetting >> + * permissions for DRM users. >> + */ >> + DRM_MASTER_FLUSH = BIT(6), >> }; >> >> /** >> -- >> 2.25.1 >> >
| |