[Intel-gfx] [PATCH 13/16] drm/fb-helper: Avoid race with DRM userspace

Noralf Trønnes noralf at tronnes.org
Mon Apr 1 13:31:18 UTC 2019



Den 01.04.2019 09.12, skrev Daniel Vetter:
> On Sat, Mar 30, 2019 at 10:07:58PM +0100, Noralf Trønnes wrote:
>>
>>
>> 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.
> 
> Hm, why would it not work for the hotplug case? _is_bound already checks
> dev->master (but with races, unlike this here). And the other checks are
> various levels of cargo-culting. dev->master is (well, has become through
> retrofitting) the "who owns the display" tracking, I think fully
> standardizing on that makes sense. The only case where there's a
> difference is if:
> - some compositor dropped master
> - but didn't disable it's framebuffers
> We actually want that for smooth vt switching, so that the new compositor
> could take over in a transition. Having a special case with fbdev where
> the compositor has to disable all its fb or fbcon won't take over feels a
> bit irky.

You're right ofc.
I read '->deferred_setup = true' when in fact it is '->delayed_hotplug =
true', so I assumed there was something that _is_bound() was protecting
wrt. to setup state.
I'll fix it in the next version.

Noralf.

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