[Intel-gfx] [PATCH 13/16] drm/fb-helper: Avoid race with DRM userspace
Noralf Trønnes
noralf at tronnes.org
Sat Mar 30 21:07:58 UTC 2019
Den 28.03.2019 09.17, skrev Daniel Vetter:
> On Tue, Mar 26, 2019 at 06:55:43PM +0100, Noralf Trønnes wrote:
>> drm_fb_helper_is_bound() is used to check if DRM userspace is in control.
>> This is done by looking at the fb on the primary plane. By the time
>> fb-helper gets around to committing, it's possible that the facts have
>> changed.
>>
>> Avoid this race by holding the drm_device->master_mutex lock while
>> committing. When DRM userspace does its first open, it will now wait
>> until fb-helper is done. The helper will stay away if there's a master.
>>
>> Locking rule: Always take the fb-helper lock first.
>>
>> Suggested-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
>
> I think it'd be good to reorder this earlier in the series. And I'm
> wondering why you didn't replace all occurences of the _is_bound()
> function. With the consistent master state check we're doing with this I
> don't think any of the fbdev checking is still needed. Plus looking at
> crtc->primary->fb without any locks is racy, so would be good to ditch
> that hack.
_is_bound() is used in drm_fb_helper_hotplug_event() to decide on
delayed hotplug. The master lock won't help here. I have studied
drm_fb_helper for 2 years now, and I still think it is convoluted and
difficult to grasp in places. So I kept this one call site. I'm vary of
making unrelated changes in this series.
The reason I included this patch is because it's used with the
bootsplash example support code. I can move the patch earlier in the series.
Noralf.
> -Daniel
>
>> ---
>> drivers/gpu/drm/drm_auth.c | 20 ++++++++++++++
>> drivers/gpu/drm/drm_fb_helper.c | 49 ++++++++++++++++++++++++---------
>> drivers/gpu/drm/drm_internal.h | 2 ++
>> 3 files changed, 58 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
>> index 1669c42c40ed..db199807b7dc 100644
>> --- a/drivers/gpu/drm/drm_auth.c
>> +++ b/drivers/gpu/drm/drm_auth.c
>> @@ -368,3 +368,23 @@ void drm_master_put(struct drm_master **master)
>> *master = NULL;
>> }
>> EXPORT_SYMBOL(drm_master_put);
>> +
>> +/* Used by drm_client and drm_fb_helper */
>> +bool drm_master_internal_acquire(struct drm_device *dev)
>> +{
>> + mutex_lock(&dev->master_mutex);
>> + if (dev->master) {
>> + mutex_unlock(&dev->master_mutex);
>> + return false;
>> + }
>> +
>> + 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)
>> +{
>> + mutex_unlock(&dev->master_mutex);
>> +}
>> +EXPORT_SYMBOL(drm_master_internal_release);
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 4a073cd4e423..9f253fcf3f79 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -41,6 +41,8 @@
>> #include <drm/drm_crtc_helper.h>
>> #include <drm/drm_atomic.h>
>>
>> +#include "drm_internal.h"
>> +
>> static bool drm_fbdev_emulation = true;
>> module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
>> MODULE_PARM_DESC(fbdev_emulation,
>> @@ -235,7 +237,12 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
>> return 0;
>>
>> mutex_lock(&fb_helper->lock);
>> - ret = drm_client_modesets_commit(fb_helper->dev, fb_helper->modesets);
>> + if (drm_master_internal_acquire(fb_helper->dev)) {
>> + ret = drm_client_modesets_commit(fb_helper->dev, fb_helper->modesets);
>> + drm_master_internal_release(fb_helper->dev);
>> + } else {
>> + ret = -EBUSY;
>> + }
>>
>> do_delayed = fb_helper->delayed_hotplug;
>> if (do_delayed)
>> @@ -332,13 +339,16 @@ static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { };
>> static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
>> {
>> struct drm_fb_helper *fb_helper = info->par;
>> + struct drm_device *dev = fb_helper->dev;
>>
>> /*
>> * For each CRTC in this fb, turn the connectors on/off.
>> */
>> mutex_lock(&fb_helper->lock);
>> - if (drm_fb_helper_is_bound(fb_helper))
>> - drm_client_modesets_dpms(fb_helper->dev, fb_helper->modesets, dpms_mode);
>> + if (drm_master_internal_acquire(dev)) {
>> + drm_client_modesets_dpms(dev, fb_helper->modesets, dpms_mode);
>> + drm_master_internal_release(dev);
>> + }
>> mutex_unlock(&fb_helper->lock);
>> }
>>
>> @@ -1097,6 +1107,7 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
>> 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;
>>
>> if (oops_in_progress)
>> @@ -1104,9 +1115,9 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
>>
>> mutex_lock(&fb_helper->lock);
>>
>> - if (!drm_fb_helper_is_bound(fb_helper)) {
>> + if (!drm_master_internal_acquire(dev)) {
>> ret = -EBUSY;
>> - goto out;
>> + goto unlock;
>> }
>>
>> if (info->fix.visual == FB_VISUAL_TRUECOLOR)
>> @@ -1116,7 +1127,8 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
>> else
>> ret = setcmap_legacy(cmap, info);
>>
>> -out:
>> + drm_master_internal_release(dev);
>> +unlock:
>> mutex_unlock(&fb_helper->lock);
>>
>> return ret;
>> @@ -1136,11 +1148,13 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
>> unsigned long arg)
>> {
>> struct drm_fb_helper *fb_helper = info->par;
>> + struct drm_device *dev = fb_helper->dev;
>> struct drm_crtc *crtc;
>> int ret = 0;
>>
>> mutex_lock(&fb_helper->lock);
>> - if (!drm_fb_helper_is_bound(fb_helper)) {
>> +
>> + if (!drm_master_internal_acquire(dev)) {
>> ret = -EBUSY;
>> goto unlock;
>> }
>> @@ -1177,13 +1191,15 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
>> }
>>
>> ret = 0;
>> - goto unlock;
>> + break;
>> default:
>> ret = -ENOTTY;
>> }
>>
>> + drm_master_internal_release(dev);
>> unlock:
>> mutex_unlock(&fb_helper->lock);
>> +
>> return ret;
>> }
>> EXPORT_SYMBOL(drm_fb_helper_ioctl);
>> @@ -1426,15 +1442,19 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
>> return -EBUSY;
>>
>> mutex_lock(&fb_helper->lock);
>> - if (!drm_fb_helper_is_bound(fb_helper)) {
>> - mutex_unlock(&fb_helper->lock);
>> - return -EBUSY;
>> +
>> + if (!drm_master_internal_acquire(dev)) {
>> + ret = -EBUSY;
>> + goto unlock;
>> }
>>
>> if (drm_drv_uses_atomic_modeset(dev))
>> ret = pan_display_atomic(var, info);
>> else
>> ret = pan_display_legacy(var, info);
>> +
>> + drm_master_internal_release(dev);
>> +unlock:
>> mutex_unlock(&fb_helper->lock);
>>
>> return ret;
>> @@ -1451,6 +1471,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>> int ret = 0;
>> int crtc_count = 0;
>> struct drm_connector_list_iter conn_iter;
>> + struct drm_device *dev = fb_helper->dev;
>> struct drm_fb_helper_surface_size sizes;
>> struct drm_connector *connector;
>> struct drm_mode_set *mode_set;
>> @@ -1593,8 +1614,10 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>> DRM_INFO("Cannot find any crtc or sizes\n");
>>
>> /* First time: disable all crtc's.. */
>> - if (!fb_helper->deferred_setup && !READ_ONCE(fb_helper->dev->master))
>> - drm_client_modesets_commit(fb_helper->dev, fb_helper->modesets);
>> + if (!fb_helper->deferred_setup && drm_master_internal_acquire(dev)) {
>> + drm_client_modesets_commit(dev, fb_helper->modesets);
>> + drm_master_internal_release(dev);
>> + }
>> return -EAGAIN;
>> }
>>
>> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
>> index 251d67e04c2d..6d5d1184c084 100644
>> --- a/drivers/gpu/drm/drm_internal.h
>> +++ b/drivers/gpu/drm/drm_internal.h
>> @@ -91,6 +91,8 @@ 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);
>>
>> /* drm_sysfs.c */
>> extern struct class *drm_class;
>> --
>> 2.20.1
>>
>
More information about the Intel-gfx
mailing list