| 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
| ||
|
Date: Fri, 2 Sep 2011 18:06:49 +0200 (CEST)
From: Guennadi Liakhovetski <g.liakhovetski@....de>
To: Florian Tobias Schandinat <FlorianSchandinat@....de>
cc: Bruno Prémont <bonbons@...ux-vserver.org>,
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
Hi Florian
On Thu, 1 Sep 2011, Florian Tobias Schandinat wrote:
> 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.
So, the patch looks simple and correct, when applied on top of
http://marc.info/?l=linux-kernel&m=130833638508657&w=2
But it doesn't apply anymore and after fixing a trivial merge conflict, it
fails to build, which is also trivially fixed. Please, add my
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@....de>
to the set and use this updated version:
diff --git a/drivers/video/sh_mobile_hdmi.c b/drivers/video/sh_mobile_hdmi.c
index 7d54e2c..647ba98 100644
--- a/drivers/video/sh_mobile_hdmi.c
+++ b/drivers/video/sh_mobile_hdmi.c
@@ -1111,6 +1111,7 @@ static long sh_hdmi_clk_configure(struct sh_hdmi *hdmi, unsigned long hdmi_rate,
static void sh_hdmi_edid_work_fn(struct work_struct *work)
{
struct sh_hdmi *hdmi = container_of(work, struct sh_hdmi, edid_work.work);
+ struct fb_info *info;
struct sh_mobile_hdmi_info *pdata = hdmi->dev->platform_data;
struct sh_mobile_lcdc_chan *ch;
int ret;
@@ -1123,8 +1124,9 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
mutex_lock(&hdmi->mutex);
+ info = hdmi->info;
+
if (hdmi->hp_state == HDMI_HOTPLUG_CONNECTED) {
- struct fb_info *info = hdmi->info;
unsigned long parent_rate = 0, hdmi_rate;
ret = sh_hdmi_read_edid(hdmi, &hdmi_rate, &parent_rate);
@@ -1148,42 +1150,45 @@ 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)
+ if (!info)
goto out;
hdmi->monspec.modedb_len = 0;
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(info, 1);
- console_unlock();
+ console_unlock();
+ unlock_fb_info(info);
+ }
}
out:
Thanks
Guennadi
>
>
> 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@...ux-vserver.org> wrote:
> >> 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;
> >>> }
> >
> >
> > 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@...r.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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/
Powered by blists - more mailing lists