Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend

[Florian Tobias Schandinat <FlorianSchandinat@gmx.de>]

ping

Guennadi, I really want this issue fixed. Please have a look at Bruno's patch
otherwise your driver might remain or get even more broken...

I am scheduling Herton's patch for the next merge window.


Regards,

Florian Tobias Schandinat


On 06/18/2011 09:19 AM, Bruno Prémont wrote:
> Guennadi, could you have a look at (completely untested) patch which avoids
> possible deadlock due to inverted lock taking order on hotplug as well
> as "readding" lock_fb_info() for fb_set_suspend() call after Herton's
> patch to fb_set_suspend().
> 
> Thanks,
> Bruno
> 
> 
> On Sat, 18 June 2011 Bruno Prémont <bonbons@linux-vserver.org> wrote:
>> On Fri, 17 June 2011 Florian Tobias Schandinat <FlorianSchandinat@gmx.de> wrote:
>>> From: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
>>>
>>> A lock ordering issue can cause deadlocks: in framebuffer/console code,
>>> all needed struct fb_info locks are taken before acquire_console_sem(),
>>> in places which need to take console semaphore.
>>>
>>> But fb_set_suspend is always called with console semaphore held, and
>>> inside it we call lock_fb_info which gets the fb_info lock, inverse
>>> locking order of what the rest of the code does. This causes a real
>>> deadlock issue, when we write to state fb sysfs attribute (which calls
>>> fb_set_suspend) while a framebuffer is being unregistered by
>>> remove_conflicting_framebuffers, as can be shown by following show
>>> blocked state trace on a test program which loads i915 and runs another
>>> forked processes writing to state attribute:
>>>
>>> Test process with semaphore held and trying to get fb_info lock:
>>
>> ...
>>
>>> fb-test2 which reproduces above is available on kernel.org bug #26232.
>>> To solve this issue, avoid calling lock_fb_info inside fb_set_suspend,
>>> and move it out to where needed (callers of fb_set_suspend must call
>>> lock_fb_info before if needed). So far, the only place which needs to
>>> call lock_fb_info is store_fbstate, all other places which calls
>>> fb_set_suspend are suspend/resume hooks that should not need the lock as
>>> they should be run only when processes are already frozen in
>>> suspend/resume.
>>
>> From a quick look through FB drivers in 2.6.39 I've found one that would need
>> more work:
>> - drivers/video/sh_mobile_hdmi.c: sh_hdmi_edid_work_fn()
>>   Should get changed to
>>   a) right locking order in case (hdmi->hp_state = HDMI_HOTPLUG_CONNECTED)
>>   b) lock fb_info in the other case
>>   For this one fb_set_suspend() does get call in a hotplug worker,
>>   thus independently on suspend/resume process.
>>
>> The rest does match the suspend/resume hook pattern mentioned.
>>
>> Bruno
>>
>>
>>> References: https://bugzilla.kernel.org/show_bug.cgi?id&232
>>> Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
>>> Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
>>> Cc: stable@kernel.org
>>> ---
>>>  drivers/video/fbmem.c   |    3 ---
>>>  drivers/video/fbsysfs.c |    3 +++
>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
>>> index 5aac00e..ad93629 100644
>>> --- a/drivers/video/fbmem.c
>>> +++ b/drivers/video/fbmem.c
>>> @@ -1738,8 +1738,6 @@ void fb_set_suspend(struct fb_info *info, int state)
>>>  {
>>>  	struct fb_event event;
>>>  
>>> -	if (!lock_fb_info(info))
>>> -		return;
>>>  	event.info = info;
>>>  	if (state) {
>>>  		fb_notifier_call_chain(FB_EVENT_SUSPEND, &event);
>>> @@ -1748,7 +1746,6 @@ void fb_set_suspend(struct fb_info *info, int state)
>>>  		info->state = FBINFO_STATE_RUNNING;
>>>  		fb_notifier_call_chain(FB_EVENT_RESUME, &event);
>>>  	}
>>> -	unlock_fb_info(info);
>>>  }
>>>  
>>>  /**
>>> diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
>>> index 04251ce..67afa9c 100644
>>> --- a/drivers/video/fbsysfs.c
>>> +++ b/drivers/video/fbsysfs.c
>>> @@ -399,9 +399,12 @@ static ssize_t store_fbstate(struct device *device,
>>>  
>>>  	state = simple_strtoul(buf, &last, 0);
>>>  
>>> +	if (!lock_fb_info(fb_info))
>>> +		return -ENODEV;
>>>  	console_lock();
>>>  	fb_set_suspend(fb_info, (int)state);
>>>  	console_unlock();
>>> +	unlock_fb_info(fb_info);
>>>  
>>>  	return count;
>>>  }
> 
> 
> diff --git a/drivers/video/sh_mobile_hdmi.c b/drivers/video/sh_mobile_hdmi.c
> index 2b9e56a..b1a13ab 100644
> --- a/drivers/video/sh_mobile_hdmi.c
> +++ b/drivers/video/sh_mobile_hdmi.c
> @@ -1151,27 +1151,27 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
>  
>  		ch = info->par;
>  
> -		console_lock();
> +		if (lock_fb_info(info)) {
> +			console_lock();
>  
> -		/* HDMI plug in */
> -		if (!sh_hdmi_must_reconfigure(hdmi) &&
> -		    info->state = FBINFO_STATE_RUNNING) {
> -			/*
> -			 * First activation with the default monitor - just turn
> -			 * on, if we run a resume here, the logo disappears
> -			 */
> -			if (lock_fb_info(info)) {
> +			/* HDMI plug in */
> +			if (!sh_hdmi_must_reconfigure(hdmi) &&
> +			    info->state = FBINFO_STATE_RUNNING) {
> +				/*
> +				 * First activation with the default monitor - just turn
> +				 * on, if we run a resume here, the logo disappears
> +				 */
>  				info->var.width = hdmi->var.width;
>  				info->var.height = hdmi->var.height;
>  				sh_hdmi_display_on(hdmi, info);
> -				unlock_fb_info(info);
> +			} else {
> +				/* New monitor or have to wake up */
> +				fb_set_suspend(info, 0);
>  			}
> -		} else {
> -			/* New monitor or have to wake up */
> -			fb_set_suspend(info, 0);
> -		}
>  
> -		console_unlock();
> +			console_unlock();
> +			unlock_fb_info(info);
> +		}
>  	} else {
>  		ret = 0;
>  		if (!hdmi->info)
> @@ -1181,12 +1181,15 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
>  		fb_destroy_modedb(hdmi->monspec.modedb);
>  		hdmi->monspec.modedb = NULL;
>  
> -		console_lock();
> +		if (lock_fb_info(info)) {
> +			console_lock();
>  
> -		/* HDMI disconnect */
> -		fb_set_suspend(hdmi->info, 1);
> +			/* HDMI disconnect */
> +			fb_set_suspend(hdmi->info, 1);
>  
> -		console_unlock();
> +			console_unlock();
> +			unlock_fb_info(info);
> +		}
>  		pm_runtime_put(hdmi->dev);
>  	}
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>