[PATCH v2 02/12] drm/fb-helper: Avoid race with DRM userspace
Noralf Trønnes
noralf at tronnes.org
Tue Apr 16 18:46:24 UTC 2019
Den 16.04.2019 09.59, skrev Daniel Vetter:
> On Sun, Apr 07, 2019 at 06:52:33PM +0200, 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.
>>
>> v2:
>> - Remove drm_fb_helper_is_bound() (Daniel Vetter)
>> - No need to check fb_helper->dev->master in
>> drm_fb_helper_single_fb_probe(), restore_fbdev_mode() has the check.
>>
>> Suggested-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
>> ---
>> drivers/gpu/drm/drm_auth.c | 20 ++++++++
>> drivers/gpu/drm/drm_fb_helper.c | 90 ++++++++++++++++-----------------
>> drivers/gpu/drm/drm_internal.h | 2 +
>> 3 files changed, 67 insertions(+), 45 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 84791dd4a90d..a6be09ae899b 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -44,6 +44,7 @@
>>
>> #include "drm_crtc_internal.h"
>> #include "drm_crtc_helper_internal.h"
>> +#include "drm_internal.h"
>>
>> static bool drm_fbdev_emulation = true;
>> module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
>> @@ -509,7 +510,7 @@ static int restore_fbdev_mode_legacy(struct drm_fb_helper *fb_helper)
>> return ret;
>> }
>>
>> -static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>> +static int restore_fbdev_mode_force(struct drm_fb_helper *fb_helper)
>
> Bikeshed: usually the function variant that's run with locks already taken
> is called _locked or has a __ prefix. _force feels a bit misplaced.
This isn't a _locked function in the usual sense, it is: apply modeset
even if there is a DRM master. So we are _forcing a modeset on a
possibly unexpecting DRM userspace. To me a _locked function would imply
that the caller _must_ take a lock in order to use it.
But no big deal, I can rename it _locked if that reads better. After a
few years of reading kernel code I've come to appreciate the consistency
in how things are done and named. Every time things are different it
slows down my internal logic/pattern parser.
>> {
>> struct drm_device *dev = fb_helper->dev;
>>
>> @@ -519,6 +520,21 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>> return restore_fbdev_mode_legacy(fb_helper);
>> }
>>
>> +static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>> +{
>> + struct drm_device *dev = fb_helper->dev;
>> + int ret;
>> +
>> + if (!drm_master_internal_acquire(dev))
>> + return -EBUSY;
>> +
>> + ret = restore_fbdev_mode_force(fb_helper);
>> +
>> + drm_master_internal_release(dev);
>> +
>> + return ret;
>> +}
>> +
>> /**
>> * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration
>> * @fb_helper: driver-allocated fbdev helper, can be NULL
>> @@ -556,34 +572,6 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
>> }
>> EXPORT_SYMBOL(drm_fb_helper_restore_fbdev_mode_unlocked);
>>
>> -static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
>> -{
>> - struct drm_device *dev = fb_helper->dev;
>> - struct drm_crtc *crtc;
>> - int bound = 0, crtcs_bound = 0;
>> -
>> - /*
>> - * Sometimes user space wants everything disabled, so don't steal the
>> - * display if there's a master.
>> - */
>> - if (READ_ONCE(dev->master))
>> - return false;
>> -
>> - drm_for_each_crtc(crtc, dev) {
>> - drm_modeset_lock(&crtc->mutex, NULL);
>> - if (crtc->primary->fb)
>> - crtcs_bound++;
>> - if (crtc->primary->fb == fb_helper->fb)
>> - bound++;
>> - drm_modeset_unlock(&crtc->mutex);
>> - }
>> -
>> - if (bound < crtcs_bound)
>> - return false;
>> -
>> - return true;
>> -}
>> -
>> #ifdef CONFIG_MAGIC_SYSRQ
>> /*
>> * restore fbcon display for all kms driver's using this helper, used for sysrq
>> @@ -604,7 +592,7 @@ static bool drm_fb_helper_force_kernel_mode(void)
>> continue;
>>
>> mutex_lock(&helper->lock);
>> - ret = restore_fbdev_mode(helper);
>> + ret = restore_fbdev_mode_force(helper);
>
> I'd leave this as-is, because:
> a) I'm too lazy to review the locking of our open/close calls to convince
> myself that lastclose can't race with the next open
> b) it won't hurt
> c) leaves the door open to easily make our open/close more concurrent in
> the future
>
I'm not actually changing anything here, it's restore_fbdev_mode() that
has changed, it now bails out if there's a DRM master. If we don't
change this to _force/_locked, then sysrq won't work if there's a DRM
master. Which kind of defeats the whole idea of this 'give me fbcon
right now' functionality doesn't it?
>> if (ret)
>> error = true;
>> mutex_unlock(&helper->lock);
>> @@ -663,20 +651,22 @@ static void dpms_legacy(struct drm_fb_helper *fb_helper, int dpms_mode)
>> 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)) {
>> - mutex_unlock(&fb_helper->lock);
>> - return;
>> - }
>> + if (!drm_master_internal_acquire(dev))
>> + goto unlock;
>>
>> - if (drm_drv_uses_atomic_modeset(fb_helper->dev))
>> + if (drm_drv_uses_atomic_modeset(dev))
>> restore_fbdev_mode_atomic(fb_helper, dpms_mode == DRM_MODE_DPMS_ON);
>> else
>> dpms_legacy(fb_helper, dpms_mode);
>> +
>> + drm_master_internal_release(dev);
>> +unlock:
>> mutex_unlock(&fb_helper->lock);
>> }
>>
>> @@ -1509,6 +1499,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)
>> @@ -1516,9 +1507,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)
>> @@ -1528,7 +1519,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;
>> @@ -1548,12 +1540,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_mode_set *mode_set;
>> 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;
>> }
>> @@ -1591,11 +1584,12 @@ 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;
>> @@ -1847,15 +1841,18 @@ 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;
>> @@ -2014,7 +2011,7 @@ 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))
>> + if (!fb_helper->deferred_setup)
>> restore_fbdev_mode(fb_helper);
>
> I think we need to return the errno here, since without that the higher
> levels won't reprobe correctly. Plus we need to remap -EBUSY to -EAGAIN
> (or change the check in __drm_fb_helper_initial_config_and_unlock to also
> retry on -EBUSY).
>
I don't think so, because -EAGAIN is returned unconditionally on the
line below. The restore_fbdev_mode() call is just to disable the outputs
_if_ there's no DRM master, which that function now checks for.
Noralf.
>> return -EAGAIN;
>> }
>> @@ -2842,6 +2839,7 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
>> */
>> int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>> {
>> + struct drm_device *dev = fb_helper->dev;
>> int err = 0;
>>
>> if (!drm_fbdev_emulation || !fb_helper)
>> @@ -2854,12 +2852,14 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>> return err;
>> }
>>
>> - if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
>> + if (!fb_helper->fb || !drm_master_internal_acquire(dev)) {
>> fb_helper->delayed_hotplug = true;
>> mutex_unlock(&fb_helper->lock);
>> return err;
>> }
>>
>> + drm_master_internal_release(dev);
>> +
>> DRM_DEBUG_KMS("\n");
>>
>> drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
>> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
>> index d9a483a5fce0..3ee97c9998a2 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;
>
> With the nits addressed:
>
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>
>> --
>> 2.20.1
>>
>
More information about the dri-devel
mailing list