| Message ID | 20210826020122.1488002-4-desmondcheongzx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: update locking for modesetting | expand |
On Thu, Aug 26, 2021 at 10:01:18AM +0800, Desmond Cheong Zhi Xi wrote: > In a future patch, a read lock on drm_device.master_rwsem is > held in the ioctl handler before the check for ioctl > permissions. However, this inverts the lock hierarchy of > drm_global_mutex --> master_rwsem. > > To avoid this, we do some prep work to grab the drm_global_mutex > before checking for ioctl permissions. > > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> > --- > drivers/gpu/drm/drm_ioctl.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index d25713b09b80..158629d88319 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -772,19 +772,19 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata, > if (drm_dev_is_unplugged(dev)) > return -ENODEV; > > + /* Enforce sane locking for modern driver ioctls. */ > + if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)) && !(flags & DRM_UNLOCKED)) Maybe have a local bool locked_ioctl for this so it's extremely clear it's the same condition in both? Either way: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > + mutex_lock(&drm_global_mutex); > + > retcode = drm_ioctl_permit(flags, file_priv); > if (unlikely(retcode)) > - return retcode; > + goto out; > > - /* Enforce sane locking for modern driver ioctls. */ > - if (likely(!drm_core_check_feature(dev, DRIVER_LEGACY)) || > - (flags & DRM_UNLOCKED)) > - retcode = func(dev, kdata, file_priv); > - else { > - mutex_lock(&drm_global_mutex); > - retcode = func(dev, kdata, file_priv); > + retcode = func(dev, kdata, file_priv); > + > +out: > + if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)) && !(flags & DRM_UNLOCKED)) > mutex_unlock(&drm_global_mutex); > - } > return retcode; > } > EXPORT_SYMBOL(drm_ioctl_kernel); > -- > 2.25.1 >
On 26/8/21 5:58 pm, Daniel Vetter wrote: > On Thu, Aug 26, 2021 at 10:01:18AM +0800, Desmond Cheong Zhi Xi wrote: >> In a future patch, a read lock on drm_device.master_rwsem is >> held in the ioctl handler before the check for ioctl >> permissions. However, this inverts the lock hierarchy of >> drm_global_mutex --> master_rwsem. >> >> To avoid this, we do some prep work to grab the drm_global_mutex >> before checking for ioctl permissions. >> >> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> >> --- >> drivers/gpu/drm/drm_ioctl.c | 18 +++++++++--------- >> 1 file changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c >> index d25713b09b80..158629d88319 100644 >> --- a/drivers/gpu/drm/drm_ioctl.c >> +++ b/drivers/gpu/drm/drm_ioctl.c >> @@ -772,19 +772,19 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata, >> if (drm_dev_is_unplugged(dev)) >> return -ENODEV; >> >> + /* Enforce sane locking for modern driver ioctls. */ >> + if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)) && !(flags & DRM_UNLOCKED)) > > Maybe have a local bool locked_ioctl for this so it's extremely clear it's > the same condition in both? > > Either way: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Thanks for the suggestion and review. Sounds good, I'll update and send out a new version. (Sorry for delays, been busy with moving) >> + mutex_lock(&drm_global_mutex); >> + >> retcode = drm_ioctl_permit(flags, file_priv); >> if (unlikely(retcode)) >> - return retcode; >> + goto out; >> >> - /* Enforce sane locking for modern driver ioctls. */ >> - if (likely(!drm_core_check_feature(dev, DRIVER_LEGACY)) || >> - (flags & DRM_UNLOCKED)) >> - retcode = func(dev, kdata, file_priv); >> - else { >> - mutex_lock(&drm_global_mutex); >> - retcode = func(dev, kdata, file_priv); >> + retcode = func(dev, kdata, file_priv); >> + >> +out: >> + if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)) && !(flags & DRM_UNLOCKED)) >> mutex_unlock(&drm_global_mutex); >> - } >> return retcode; >> } >> EXPORT_SYMBOL(drm_ioctl_kernel); >> -- >> 2.25.1 >> >
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index d25713b09b80..158629d88319 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -772,19 +772,19 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata, if (drm_dev_is_unplugged(dev)) return -ENODEV; + /* Enforce sane locking for modern driver ioctls. */ + if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)) && !(flags & DRM_UNLOCKED)) + mutex_lock(&drm_global_mutex); + retcode = drm_ioctl_permit(flags, file_priv); if (unlikely(retcode)) - return retcode; + goto out; - /* Enforce sane locking for modern driver ioctls. */ - if (likely(!drm_core_check_feature(dev, DRIVER_LEGACY)) || - (flags & DRM_UNLOCKED)) - retcode = func(dev, kdata, file_priv); - else { - mutex_lock(&drm_global_mutex); - retcode = func(dev, kdata, file_priv); + retcode = func(dev, kdata, file_priv); + +out: + if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)) && !(flags & DRM_UNLOCKED)) mutex_unlock(&drm_global_mutex); - } return retcode; } EXPORT_SYMBOL(drm_ioctl_kernel);
In a future patch, a read lock on drm_device.master_rwsem is held in the ioctl handler before the check for ioctl permissions. However, this inverts the lock hierarchy of drm_global_mutex --> master_rwsem. To avoid this, we do some prep work to grab the drm_global_mutex before checking for ioctl permissions. Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> --- drivers/gpu/drm/drm_ioctl.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)