linux-kernel - Re: [PATCH] fb: avoid possible deadlock caused by fb_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 18 Jun 2011 10:43:11 +0200
From:	Bruno Prémont <bonbons@...ux-vserver.org>
To:	Florian Tobias Schandinat <FlorianSchandinat@....de>
Cc:	lethal@...ux-sh.org, linux-fbdev@...r.kernel.org,
	francis.moro@...il.com, torvalds@...ux-foundation.org,
	linux-kernel@...r.kernel.org,
	Herton Ronaldo Krzesinski <herton@...driva.com.br>,
	stable@...nel.org
Subject: Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend

On Fri, 17 June 2011 Florian Tobias Schandinat <FlorianSchandinat@....de> wrote:
> From: Herton Ronaldo Krzesinski <herton@...driva.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=26232
> Signed-off-by: Herton Ronaldo Krzesinski <herton@...driva.com.br>
> Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@....de>
> Cc: stable@...nel.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;
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/