Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754833Ab1EFANd (ORCPT ); Thu, 5 May 2011 20:13:33 -0400 Received: from kroah.org ([198.145.64.141]:32901 "EHLO coco.kroah.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753327Ab1EFANb (ORCPT ); Thu, 5 May 2011 20:13:31 -0400 X-Mailbox-Line: From gregkh@clark.kroah.org Thu May 5 17:12:09 2011 Message-Id: <20110506001209.680029807@clark.kroah.org> User-Agent: quilt/0.48-16.4 Date: Thu, 05 May 2011 17:10:56 -0700 From: Greg KH To: linux-kernel@vger.kernel.org, stable@kernel.org Cc: stable-review@kernel.org, torvalds@linux-foundation.org, akpm@linux-foundation.org, alan@lxorguk.ukuu.org.uk, Jarod Wilson , Mauro Carvalho Chehab Subject: [patch 24/38] [media] imon: add conditional locking in change_protocol In-Reply-To: <20110506001225.GA10547@kroah.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7821 Lines: 200 2.6.38-stable review patch. If anyone has any objections, please let us know. ------------------ From: Jarod Wilson commit 23ef710e1a6c4d6b9ef1c2fa19410f7f1479401e upstream. The imon_ir_change_protocol function gets called two different ways, one way is from rc_register_device, for initial protocol selection/setup, and the other is via a userspace-initiated protocol change request, either by direct sysfs prodding or by something like ir-keytable. In the rc_register_device case, the imon context lock is already held, but when initiated from userspace, it is not, so we must acquire it, prior to calling send_packet, which requires that the lock is held. Without this change, there's an easily reproduceable deadlock when another function calls send_packet (such as either of the display write fops) after a userspace-initiated change_protocol. With a lock-debugging-enabled kernel, I was getting this: [ 15.014153] ===================================== [ 15.015048] [ BUG: bad unlock balance detected! ] [ 15.015048] ------------------------------------- [ 15.015048] ir-keytable/773 is trying to release lock (&ictx->lock) at: [ 15.015048] [] mutex_unlock+0xe/0x10 [ 15.015048] but there are no more locks to release! [ 15.015048] [ 15.015048] other info that might help us debug this: [ 15.015048] 2 locks held by ir-keytable/773: [ 15.015048] #0: (&buffer->mutex){+.+.+.}, at: [] sysfs_write_file+0x3c/0x144 [ 15.015048] #1: (s_active#87){.+.+.+}, at: [] sysfs_write_file+0xe7/0x144 [ 15.015048] [ 15.015048] stack backtrace: [ 15.015048] Pid: 773, comm: ir-keytable Not tainted 2.6.38.4-20.fc15.x86_64.debug #1 [ 15.015048] Call Trace: [ 15.015048] [] ? print_unlock_inbalance_bug+0xca/0xd5 [ 15.015048] [] ? lock_release_non_nested+0xc1/0x263 [ 15.015048] [] ? mutex_unlock+0xe/0x10 [ 15.015048] [] ? mutex_unlock+0xe/0x10 [ 15.015048] [] ? lock_release+0x17d/0x1a4 [ 15.015048] [] ? __mutex_unlock_slowpath+0xc5/0x125 [ 15.015048] [] ? mutex_unlock+0xe/0x10 [ 15.015048] [] ? send_packet+0x1c9/0x264 [imon] [ 15.015048] [] ? lock_release_non_nested+0xdb/0x263 [ 15.015048] [] ? imon_ir_change_protocol+0x126/0x15e [imon] [ 15.015048] [] ? store_protocols+0x1c3/0x286 [rc_core] [ 15.015048] [] ? dev_attr_store+0x20/0x22 [ 15.015048] [] ? sysfs_write_file+0x108/0x144 ... The original report that led to the investigation was the following: [ 1679.457305] INFO: task LCDd:8460 blocked for more than 120 seconds. [ 1679.457307] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [ 1679.457309] LCDd D ffff88010fcd89c8 0 8460 1 0x00000000 [ 1679.457312] ffff8800d5a03b48 0000000000000082 0000000000000000 ffff8800d5a03fd8 [ 1679.457314] 00000000012dcd30 fffffffffffffffd ffff8800d5a03fd8 ffff88010fcd86f0 [ 1679.457316] ffff8800d5a03fd8 ffff8800d5a03fd8 ffff88010fcd89d0 ffff8800d5a03fd8 [ 1679.457319] Call Trace: [ 1679.457324] [] ? zone_statistics+0x75/0x90 [ 1679.457327] [] ? get_page_from_freelist+0x3c7/0x820 [ 1679.457330] [] __mutex_lock_slowpath+0x139/0x320 [ 1679.457335] [] mutex_lock+0x11/0x30 [ 1679.457338] [] display_open+0x66/0x130 [imon] [ 1679.457345] [] usb_open+0x180/0x310 [usbcore] [ 1679.457349] [] chrdev_open+0x1bb/0x2d0 [ 1679.457350] [] __dentry_open+0x10d/0x370 [ 1679.457352] [] ? chrdev_open+0x0/0x2d0 ... Bump the driver version here so its easier to tell if people have this locking fix or not, and also make locking during probe easier to follow. Reported-by: Benjamin Hodgetts Signed-off-by: Jarod Wilson Signed-off-by: Mauro Carvalho Chehab Signed-off-by: Greg Kroah-Hartman --- drivers/media/rc/imon.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) --- a/drivers/media/rc/imon.c +++ b/drivers/media/rc/imon.c @@ -46,7 +46,7 @@ #define MOD_AUTHOR "Jarod Wilson " #define MOD_DESC "Driver for SoundGraph iMON MultiMedia IR/Display" #define MOD_NAME "imon" -#define MOD_VERSION "0.9.2" +#define MOD_VERSION "0.9.3" #define DISPLAY_MINOR_BASE 144 #define DEVICE_NAME "lcd%d" @@ -451,8 +451,9 @@ static int display_close(struct inode *i } /** - * Sends a packet to the device -- this function must be called - * with ictx->lock held. + * Sends a packet to the device -- this function must be called with + * ictx->lock held, or its unlock/lock sequence while waiting for tx + * to complete can/will lead to a deadlock. */ static int send_packet(struct imon_context *ictx) { @@ -982,12 +983,21 @@ static void imon_touch_display_timeout(u * the iMON remotes, and those used by the Windows MCE remotes (which is * really just RC-6), but only one or the other at a time, as the signals * are decoded onboard the receiver. + * + * This function gets called two different ways, one way is from + * rc_register_device, for initial protocol selection/setup, and the other is + * via a userspace-initiated protocol change request, either by direct sysfs + * prodding or by something like ir-keytable. In the rc_register_device case, + * the imon context lock is already held, but when initiated from userspace, + * it is not, so we must acquire it prior to calling send_packet, which + * requires that the lock is held. */ static int imon_ir_change_protocol(struct rc_dev *rc, u64 rc_type) { int retval; struct imon_context *ictx = rc->priv; struct device *dev = ictx->dev; + bool unlock = false; unsigned char ir_proto_packet[] = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x86 }; @@ -1020,6 +1030,11 @@ static int imon_ir_change_protocol(struc memcpy(ictx->usb_tx_buf, &ir_proto_packet, sizeof(ir_proto_packet)); + if (!mutex_is_locked(&ictx->lock)) { + unlock = true; + mutex_lock(&ictx->lock); + } + retval = send_packet(ictx); if (retval) goto out; @@ -1028,6 +1043,9 @@ static int imon_ir_change_protocol(struc ictx->pad_mouse = false; out: + if (unlock) + mutex_unlock(&ictx->lock); + return retval; } @@ -2125,6 +2143,7 @@ static struct imon_context *imon_init_in goto rdev_setup_failed; } + mutex_unlock(&ictx->lock); return ictx; rdev_setup_failed: @@ -2196,6 +2215,7 @@ static struct imon_context *imon_init_in goto urb_submit_failed; } + mutex_unlock(&ictx->lock); return ictx; urb_submit_failed: @@ -2290,6 +2310,8 @@ static int __devinit imon_probe(struct u usb_set_intfdata(interface, ictx); if (ifnum == 0) { + mutex_lock(&ictx->lock); + if (product == 0xffdc && ictx->rf_device) { sysfs_err = sysfs_create_group(&interface->dev.kobj, &imon_rf_attr_group); @@ -2300,13 +2322,14 @@ static int __devinit imon_probe(struct u if (ictx->display_supported) imon_init_display(ictx, interface); + + mutex_unlock(&ictx->lock); } dev_info(dev, "iMON device (%04x:%04x, intf%d) on " "usb<%d:%d> initialized\n", vendor, product, ifnum, usbdev->bus->busnum, usbdev->devnum); - mutex_unlock(&ictx->lock); mutex_unlock(&driver_lock); return 0; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/