Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
Subject: Re: [PATCH 1/2] drm: avoid races with modesetting rights
To:     Daniel Vetter <daniel@ffwll.ch>
References: <20210813085450.32494-1-desmondcheongzx@gmail.com>
 <20210813085450.32494-2-desmondcheongzx@gmail.com>
 <YRaUetPzT5EKaZkf@phenom.ffwll.local>
Cc:     maarten.lankhorst@linux.intel.com, mripard@kernel.org,
        tzimmermann@suse.de, airlied@linux.ie, sumit.semwal@linaro.org,
        christian.koenig@amd.com, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
        linaro-mm-sig@lists.linaro.org, intel-gfx@lists.freedesktop.org,
        skhan@linuxfoundation.org, gregkh@linuxfoundation.org,
        linux-kernel-mentees@lists.linuxfoundation.org
Message-ID: <2d5290a0-b3ad-1719-c862-c004fe2342de@gmail.com>
Date:   Sun, 15 Aug 2021 15:10:16 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <YRaUetPzT5EKaZkf@phenom.ffwll.local>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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
>>
>