mirrors/linux-stable
1
0
Fork 0
mirror of https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git synced 2024-10-04 08:08:54 +00:00
Code Issues Releases Wiki Activity

[media] imon: add conditional locking in change_protocol

Browse source 
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] [<ffffffff814c6297>] 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: [<ffffffff8119d400>] sysfs_write_file+0x3c/0x144
[   15.015048]  #1:  (s_active#87){.+.+.+}, at: [<ffffffff8119d4ab>] 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]  [<ffffffff81089715>] ? print_unlock_inbalance_bug+0xca/0xd5
[   15.015048]  [<ffffffff8108b35c>] ? lock_release_non_nested+0xc1/0x263
[   15.015048]  [<ffffffff814c6297>] ? mutex_unlock+0xe/0x10
[   15.015048]  [<ffffffff814c6297>] ? mutex_unlock+0xe/0x10
[   15.015048]  [<ffffffff8108b67b>] ? lock_release+0x17d/0x1a4
[   15.015048]  [<ffffffff814c6229>] ? __mutex_unlock_slowpath+0xc5/0x125
[   15.015048]  [<ffffffff814c6297>] ? mutex_unlock+0xe/0x10
[   15.015048]  [<ffffffffa02964b6>] ? send_packet+0x1c9/0x264 [imon]
[   15.015048]  [<ffffffff8108b376>] ? lock_release_non_nested+0xdb/0x263
[   15.015048]  [<ffffffffa0296731>] ? imon_ir_change_protocol+0x126/0x15e [imon]
[   15.015048]  [<ffffffffa024a334>] ? store_protocols+0x1c3/0x286 [rc_core]
[   15.015048]  [<ffffffff81326e4e>] ? dev_attr_store+0x20/0x22
[   15.015048]  [<ffffffff8119d4cc>] ? 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]  [<ffffffff810ff1a5>] ? zone_statistics+0x75/0x90
[ 1679.457327]  [<ffffffff810ea907>] ? get_page_from_freelist+0x3c7/0x820
[ 1679.457330]  [<ffffffff813b0a49>] __mutex_lock_slowpath+0x139/0x320
[ 1679.457335]  [<ffffffff813b0c41>] mutex_lock+0x11/0x30
[ 1679.457338]  [<ffffffffa0d54216>] display_open+0x66/0x130 [imon]
[ 1679.457345]  [<ffffffffa01d06c0>] usb_open+0x180/0x310 [usbcore]
[ 1679.457349]  [<ffffffff81143b3b>] chrdev_open+0x1bb/0x2d0
[ 1679.457350]  [<ffffffff8113d93d>] __dentry_open+0x10d/0x370
[ 1679.457352]  [<ffffffff81143980>] ? 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.

CC: stable@kernel.org
Reported-by: Benjamin Hodgetts <ben@xnode.org>
Signed-off-by: Jarod Wilson <jarod@redhat.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
This commit is contained in:
Jarod Wilson 2011-04-27 19:01:44 -03:00 committed by Mauro Carvalho Chehab
parent b30039333a
commit 23ef710e1a
1 changed files with 27 additions and 4 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

31
drivers/media/rc/imon.c
View file

@ -46,7 +46,7 @@
#define MOD_AUTHOR "Jarod Wilson <jarod@wilsonet.com>"
#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"
@ -460,8 +460,9 @@ static int display_close(struct inode *inode, struct file *file)
}
/**
* 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)
{
@ -991,12 +992,21 @@ static void imon_touch_display_timeout(unsigned long data)
* 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 };
@ -1029,6 +1039,11 @@ static int imon_ir_change_protocol(struct rc_dev *rc, u64 rc_type)
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;
@ -1037,6 +1052,9 @@ static int imon_ir_change_protocol(struct rc_dev *rc, u64 rc_type)
ictx->pad_mouse = false;
out:
if (unlock)
mutex_unlock(&ictx->lock);
return retval;
}
@ -2134,6 +2152,7 @@ static struct imon_context *imon_init_intf0(struct usb_interface *intf)
goto rdev_setup_failed;
}
mutex_unlock(&ictx->lock);
return ictx;
rdev_setup_failed:
@ -2205,6 +2224,7 @@ static struct imon_context *imon_init_intf1(struct usb_interface *intf,
goto urb_submit_failed;
}
mutex_unlock(&ictx->lock);
return ictx;
urb_submit_failed:
@ -2299,6 +2319,8 @@ static int __devinit imon_probe(struct usb_interface *interface,
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);
@ -2309,13 +2331,14 @@ static int __devinit imon_probe(struct usb_interface *interface,
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;