From patchwork Fri Jun 17 19:02:39 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Tobias Schandinat X-Patchwork-Id: 892252 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by demeter1.kernel.org (8.14.4/8.14.4) with ESMTP id p5HIkCCc016532 for ; Fri, 17 Jun 2011 18:46:13 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932695Ab1FQSqE (ORCPT ); Fri, 17 Jun 2011 14:46:04 -0400 Received: from mailout-de.gmx.net ([213.165.64.22]:56326 "HELO mailout-de.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1757172Ab1FQSqC (ORCPT ); Fri, 17 Jun 2011 14:46:02 -0400 Received: (qmail invoked by alias); 17 Jun 2011 18:45:59 -0000 Received: from dslb-092-075-150-030.pools.arcor-ip.net (EHLO localhost.localdomain) [92.75.150.30] by mail.gmx.net (mp002) with SMTP; 17 Jun 2011 20:45:59 +0200 X-Authenticated: #10250065 X-Provags-ID: V01U2FsdGVkX1/hKEwNMo/hpMmMBU0hb2gWOIShJtRJFkn7Dc9yBL Mqi1HvMm/bfraD From: Florian Tobias Schandinat To: lethal@linux-sh.org, linux-fbdev@vger.kernel.org Cc: francis.moro@gmail.com, torvalds@linux-foundation.org, bonbons@linux-vserver.org, linux-kernel@vger.kernel.org, Herton Ronaldo Krzesinski , Florian Tobias Schandinat , stable@kernel.org Subject: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend Date: Fri, 17 Jun 2011 19:02:39 +0000 Message-Id: <1308337359-3480-1-git-send-email-FlorianSchandinat@gmx.de> X-Mailer: git-send-email 1.6.3.2 In-Reply-To: <4DF7B0B4.4060002@gmx.de> References: <4DF7B0B4.4060002@gmx.de> X-Y-GMX-Trusted: 0 Sender: linux-fbdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fbdev@vger.kernel.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Fri, 17 Jun 2011 18:46:13 +0000 (UTC) From: Herton Ronaldo Krzesinski 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 D 0000000000000000 0 237 228 0x00000000 ffff8800774f3d68 0000000000000082 00000000000135c0 00000000000135c0 ffff880000000000 ffff8800774f3fd8 ffff8800774f3fd8 ffff880076ee4530 00000000000135c0 ffff8800774f3fd8 ffff8800774f2000 00000000000135c0 Call Trace: [] __mutex_lock_slowpath+0x11a/0x1e0 [] ? _raw_spin_lock_irq+0x22/0x40 [] mutex_lock+0x23/0x50 [] lock_fb_info+0x25/0x60 [] fb_set_suspend+0x20/0x80 [] store_fbstate+0x4f/0x70 [] dev_attr_store+0x20/0x30 [] sysfs_write_file+0xd4/0x160 [] vfs_write+0xc6/0x190 [] sys_write+0x51/0x90 [] system_call_fastpath+0x16/0x1b .. modprobe process stalled because has the fb_info lock (got inside unregister_framebuffer) but waiting for the semaphore held by the test process which is waiting to get the fb_info lock: .. modprobe D 0000000000000000 0 230 218 0x00000000 ffff880077a4d618 0000000000000082 0000000000000001 0000000000000001 ffff880000000000 ffff880077a4dfd8 ffff880077a4dfd8 ffff8800775a2e20 00000000000135c0 ffff880077a4dfd8 ffff880077a4c000 00000000000135c0 Call Trace: [] schedule_timeout+0x215/0x310 [] ? get_parent_ip+0x11/0x50 [] __down+0x6d/0xb0 [] down+0x41/0x50 [] acquire_console_sem+0x2c/0x50 [] unbind_con_driver+0xad/0x2d0 [] fbcon_event_notify+0x457/0x890 [] ? _raw_spin_unlock_irqrestore+0x1f/0x50 [] ? get_parent_ip+0x11/0x50 [] notifier_call_chain+0x4d/0x70 [] __blocking_notifier_call_chain+0x58/0x80 [] blocking_notifier_call_chain+0x16/0x20 [] fb_notifier_call_chain+0x1b/0x20 [] unregister_framebuffer+0x7c/0x130 [] remove_conflicting_framebuffers+0x153/0x180 [] register_framebuffer+0x93/0x2c0 [] drm_fb_helper_single_fb_probe+0x252/0x2f0 [drm_kms_helper] [] drm_fb_helper_initial_config+0x2f3/0x6d0 [drm_kms_helper] [] ? drm_fb_helper_single_add_all_connectors+0x5d/0x1c0 [drm_kms_helper] [] intel_fbdev_init+0xa8/0x160 [i915] [] i915_driver_load+0x854/0x12b0 [i915] [] drm_get_pci_dev+0x19e/0x360 [drm] [] ? sub_preempt_count+0x9d/0xd0 [] i915_pci_probe+0x15/0x17 [i915] [] local_pci_probe+0x5f/0xd0 [] pci_device_probe+0x119/0x120 [] ? driver_sysfs_add+0x7a/0xb0 [] driver_probe_device+0xa3/0x290 [] ? __driver_attach+0x0/0xb0 [] __driver_attach+0xab/0xb0 [] ? __driver_attach+0x0/0xb0 [] bus_for_each_dev+0x5e/0x90 [] driver_attach+0x1e/0x20 [] bus_add_driver+0xe2/0x320 [] ? i915_init+0x0/0x96 [i915] [] driver_register+0x76/0x140 [] ? i915_init+0x0/0x96 [i915] [] __pci_register_driver+0x56/0xd0 [] drm_pci_init+0xe4/0xf0 [drm] [] ? i915_init+0x0/0x96 [i915] [] drm_init+0x58/0x70 [drm] [] i915_init+0x94/0x96 [i915] [] do_one_initcall+0x44/0x190 [] sys_init_module+0xcb/0x210 [] system_call_fastpath+0x16/0x1b .. 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. References: https://bugzilla.kernel.org/show_bug.cgi?id=26232 Signed-off-by: Herton Ronaldo Krzesinski Signed-off-by: Florian Tobias Schandinat 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; }