* [patch 1/1] fb: fix potential deadlock between lock_fb_info and console_lock
@ 2011-09-20 21:11 akpm
2011-09-20 21:46 ` [patch 1/1] fb: fix potential deadlock between lock_fb_info and Florian Tobias Schandinat
2011-09-20 22:20 ` Andrea Righi
0 siblings, 2 replies; 3+ messages in thread
From: akpm @ 2011-09-20 21:11 UTC (permalink / raw)
To: linux-fbdev
From: Andrea Righi <arighi@develer.com>
Subject: fb: fix potential deadlock between lock_fb_info and console_lock
fb_set_suspend() must be called with the console semaphore held, which
means the code path coming in here will first take the console_lock() and
then call lock_fb_info().
However several framebuffer ioctl commands acquire these locks in reverse
order (lock_fb_info() and then console_lock()). This gives rise to
potential AB-BA deadlock.
Fix this by changing the order of acquisition in the ioctl commands that
make use of console_lock().
Signed-off-by: Andrea Righi <arighi@develer.com>
Reported-by: Peter Nordström (Palm GBU) <peter.nordstrom@palm.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: <stable@kernel.org>
Signed-off-by: Andrew Morton <akpm@google.com>
---
drivers/video/fbmem.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff -puN drivers/video/fbmem.c~fb-fix-potential-deadlock-between-lock_fb_info-and-console_lock drivers/video/fbmem.c
--- a/drivers/video/fbmem.c~fb-fix-potential-deadlock-between-lock_fb_info-and-console_lock
+++ a/drivers/video/fbmem.c
@@ -1076,14 +1076,16 @@ static long do_fb_ioctl(struct fb_info *
case FBIOPUT_VSCREENINFO:
if (copy_from_user(&var, argp, sizeof(var)))
return -EFAULT;
- if (!lock_fb_info(info))
- return -ENODEV;
console_lock();
+ if (!lock_fb_info(info)) {
+ console_unlock();
+ return -ENODEV;
+ }
info->flags |= FBINFO_MISC_USEREVENT;
ret = fb_set_var(info, &var);
info->flags &= ~FBINFO_MISC_USEREVENT;
- console_unlock();
unlock_fb_info(info);
+ console_unlock();
if (!ret && copy_to_user(argp, &var, sizeof(var)))
ret = -EFAULT;
break;
@@ -1112,12 +1114,14 @@ static long do_fb_ioctl(struct fb_info *
case FBIOPAN_DISPLAY:
if (copy_from_user(&var, argp, sizeof(var)))
return -EFAULT;
- if (!lock_fb_info(info))
- return -ENODEV;
console_lock();
+ if (!lock_fb_info(info)) {
+ console_unlock();
+ return -ENODEV;
+ }
ret = fb_pan_display(info, &var);
- console_unlock();
unlock_fb_info(info);
+ console_unlock();
if (ret = 0 && copy_to_user(argp, &var, sizeof(var)))
return -EFAULT;
break;
@@ -1159,14 +1163,16 @@ static long do_fb_ioctl(struct fb_info *
unlock_fb_info(info);
break;
case FBIOBLANK:
- if (!lock_fb_info(info))
- return -ENODEV;
console_lock();
+ if (!lock_fb_info(info)) {
+ console_unlock();
+ return -ENODEV;
+ }
info->flags |= FBINFO_MISC_USEREVENT;
ret = fb_blank(info, arg);
info->flags &= ~FBINFO_MISC_USEREVENT;
- console_unlock();
unlock_fb_info(info);
+ console_unlock();
break;
default:
if (!lock_fb_info(info))
_
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [patch 1/1] fb: fix potential deadlock between lock_fb_info and
2011-09-20 21:11 [patch 1/1] fb: fix potential deadlock between lock_fb_info and console_lock akpm
@ 2011-09-20 21:46 ` Florian Tobias Schandinat
2011-09-20 22:20 ` Andrea Righi
1 sibling, 0 replies; 3+ messages in thread
From: Florian Tobias Schandinat @ 2011-09-20 21:46 UTC (permalink / raw)
To: linux-fbdev
Hi Andrew,
On 09/20/2011 09:11 PM, akpm@google.com wrote:
> From: Andrea Righi <arighi@develer.com>
> Subject: fb: fix potential deadlock between lock_fb_info and console_lock
>
> fb_set_suspend() must be called with the console semaphore held, which
> means the code path coming in here will first take the console_lock() and
> then call lock_fb_info().
>
> However several framebuffer ioctl commands acquire these locks in reverse
> order (lock_fb_info() and then console_lock()). This gives rise to
> potential AB-BA deadlock.
>
> Fix this by changing the order of acquisition in the ioctl commands that
> make use of console_lock().
I already have another patch [1] that fixes the same issue in a different way.
It looks less risky than yours and got more feedback.
Best regards,
Florian Tobias Schandinat
[1] http://marc.info/?l=linux-kernel&m\x130833638508657&w=2
>
> Signed-off-by: Andrea Righi <arighi@develer.com>
> Reported-by: Peter Nordström (Palm GBU) <peter.nordstrom@palm.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: <stable@kernel.org>
>
> Signed-off-by: Andrew Morton <akpm@google.com>
> ---
>
> drivers/video/fbmem.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff -puN drivers/video/fbmem.c~fb-fix-potential-deadlock-between-lock_fb_info-and-console_lock drivers/video/fbmem.c
> --- a/drivers/video/fbmem.c~fb-fix-potential-deadlock-between-lock_fb_info-and-console_lock
> +++ a/drivers/video/fbmem.c
> @@ -1076,14 +1076,16 @@ static long do_fb_ioctl(struct fb_info *
> case FBIOPUT_VSCREENINFO:
> if (copy_from_user(&var, argp, sizeof(var)))
> return -EFAULT;
> - if (!lock_fb_info(info))
> - return -ENODEV;
> console_lock();
> + if (!lock_fb_info(info)) {
> + console_unlock();
> + return -ENODEV;
> + }
> info->flags |= FBINFO_MISC_USEREVENT;
> ret = fb_set_var(info, &var);
> info->flags &= ~FBINFO_MISC_USEREVENT;
> - console_unlock();
> unlock_fb_info(info);
> + console_unlock();
> if (!ret && copy_to_user(argp, &var, sizeof(var)))
> ret = -EFAULT;
> break;
> @@ -1112,12 +1114,14 @@ static long do_fb_ioctl(struct fb_info *
> case FBIOPAN_DISPLAY:
> if (copy_from_user(&var, argp, sizeof(var)))
> return -EFAULT;
> - if (!lock_fb_info(info))
> - return -ENODEV;
> console_lock();
> + if (!lock_fb_info(info)) {
> + console_unlock();
> + return -ENODEV;
> + }
> ret = fb_pan_display(info, &var);
> - console_unlock();
> unlock_fb_info(info);
> + console_unlock();
> if (ret = 0 && copy_to_user(argp, &var, sizeof(var)))
> return -EFAULT;
> break;
> @@ -1159,14 +1163,16 @@ static long do_fb_ioctl(struct fb_info *
> unlock_fb_info(info);
> break;
> case FBIOBLANK:
> - if (!lock_fb_info(info))
> - return -ENODEV;
> console_lock();
> + if (!lock_fb_info(info)) {
> + console_unlock();
> + return -ENODEV;
> + }
> info->flags |= FBINFO_MISC_USEREVENT;
> ret = fb_blank(info, arg);
> info->flags &= ~FBINFO_MISC_USEREVENT;
> - console_unlock();
> unlock_fb_info(info);
> + console_unlock();
> break;
> default:
> if (!lock_fb_info(info))
> _
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [patch 1/1] fb: fix potential deadlock between lock_fb_info and
2011-09-20 21:11 [patch 1/1] fb: fix potential deadlock between lock_fb_info and console_lock akpm
2011-09-20 21:46 ` [patch 1/1] fb: fix potential deadlock between lock_fb_info and Florian Tobias Schandinat
@ 2011-09-20 22:20 ` Andrea Righi
1 sibling, 0 replies; 3+ messages in thread
From: Andrea Righi @ 2011-09-20 22:20 UTC (permalink / raw)
To: linux-fbdev
On Tue, Sep 20, 2011 at 09:46:08PM +0000, Florian Tobias Schandinat wrote:
> Hi Andrew,
>
> On 09/20/2011 09:11 PM, akpm@google.com wrote:
> > From: Andrea Righi <arighi@develer.com>
> > Subject: fb: fix potential deadlock between lock_fb_info and console_lock
> >
> > fb_set_suspend() must be called with the console semaphore held, which
> > means the code path coming in here will first take the console_lock() and
> > then call lock_fb_info().
> >
> > However several framebuffer ioctl commands acquire these locks in reverse
> > order (lock_fb_info() and then console_lock()). This gives rise to
> > potential AB-BA deadlock.
> >
> > Fix this by changing the order of acquisition in the ioctl commands that
> > make use of console_lock().
>
> I already have another patch [1] that fixes the same issue in a different way.
> It looks less risky than yours and got more feedback.
>
>
> Best regards,
>
> Florian Tobias Schandinat
>
>
> [1] http://marc.info/?l=linux-kernel&m\x130833638508657&w=2
Looks better than my version.
For what it's worth it:
Reviewed-by: Andrea Righi <andrea@betterlinux.com>
Thanks,
-Andrea
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-09-20 22:20 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-20 21:11 [patch 1/1] fb: fix potential deadlock between lock_fb_info and console_lock akpm
2011-09-20 21:46 ` [patch 1/1] fb: fix potential deadlock between lock_fb_info and Florian Tobias Schandinat
2011-09-20 22:20 ` Andrea Righi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox