Re: [PATCH 1/2] drm: avoid races with modesetting rights

[Desmond Cheong Zhi Xi <desmondcheongzx@xxxxxxxxx>]

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@xxxxxxxx>
> > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@xxxxxxxxx>
>
> 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