From patchwork Thu Feb 11 16:03:15 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mauro Carvalho Chehab X-Patchwork-Id: 8282051 Return-Path: X-Original-To: patchwork-linux-media@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id BD5CDBEEE5 for ; Thu, 11 Feb 2016 16:04:46 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id AA3C620380 for ; Thu, 11 Feb 2016 16:04:45 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 88C372034C for ; Thu, 11 Feb 2016 16:04:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751531AbcBKQEl (ORCPT ); Thu, 11 Feb 2016 11:04:41 -0500 Received: from bombadil.infradead.org ([198.137.202.9]:57194 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751487AbcBKQEj (ORCPT ); Thu, 11 Feb 2016 11:04:39 -0500 Received: from [179.181.116.168] (helo=smtp.w2.samsung.com) by bombadil.infradead.org with esmtpsa (Exim 4.80.1 #2 (Red Hat Linux)) id 1aTtjS-0002hJ-2y; Thu, 11 Feb 2016 16:04:38 +0000 Received: from mchehab by smtp.w2.samsung.com with local (Exim 4.86) (envelope-from ) id 1aTti8-0005WY-4Q; Thu, 11 Feb 2016 14:03:16 -0200 From: Mauro Carvalho Chehab Cc: Mauro Carvalho Chehab , Linux Media Mailing List , Mauro Carvalho Chehab , =?UTF-8?q?David=20H=C3=A4rdeman?= , Heiner Kallweit , =?UTF-8?q?Antti=20Sepp=C3=A4l=C3=A4?= , James Hogan , Russell King Subject: [PATCH] [media] rc-core: don't lock device at rc_register_device() Date: Thu, 11 Feb 2016 14:03:15 -0200 Message-Id: <63bc60a7a76f3946508b4958d9156bdfcde6d323.1455206587.git.mchehab@osg.samsung.com> X-Mailer: git-send-email 2.5.0 To: unlisted-recipients:; (no To-header on input) Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP The mutex lock at rc_register_device() was added by commit 08aeb7c9a42a ("[media] rc: add locking to fix register/show race"). It is meant to avoid race issues when trying to open a sysfs file while the RC register didn't complete. Adding a lock there causes troubles, as detected by the Kernel lock debug instrumentation at the Kernel: ====================================================== [ INFO: possible circular locking dependency detected ] 4.5.0-rc3+ #46 Not tainted ------------------------------------------------------- systemd-udevd/2681 is trying to acquire lock: (s_active#171){++++.+}, at: [] kernfs_remove_by_name_ns+0x45/0xa0 but task is already holding lock: (&dev->lock){+.+.+.}, at: [] rc_register_device+0xb2f/0x1450 [rc_core] which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 (&dev->lock){+.+.+.}: [] lock_acquire+0x13d/0x320 [] mutex_lock_nested+0xb6/0x860 [] show_protocols+0x3b/0x3f0 [rc_core] [] dev_attr_show+0x45/0xc0 [] sysfs_kf_seq_show+0x203/0x3c0 [] kernfs_seq_show+0x121/0x1b0 [] seq_read+0x2f1/0x1160 [] kernfs_fop_read+0x321/0x460 [] __vfs_read+0xe0/0x3d0 [] vfs_read+0xde/0x2d0 [] SyS_read+0x111/0x230 [] entry_SYSCALL_64_fastpath+0x16/0x76 -> #0 (s_active#171){++++.+}: [] __lock_acquire+0x4304/0x5990 [] lock_acquire+0x13d/0x320 [] __kernfs_remove+0x58a/0x810 [] kernfs_remove_by_name_ns+0x45/0xa0 [] remove_files.isra.0+0x72/0x190 [] sysfs_remove_group+0x9b/0x150 [] sysfs_remove_groups+0x54/0xa0 [] device_remove_attrs+0xb0/0x140 [] device_del+0x38c/0x6b0 [] rc_register_device+0x8cb/0x1450 [rc_core] [] dvb_usb_remote_init+0x66b/0x14d0 [dvb_usb] [] dvb_usb_device_init+0xf21/0x1860 [dvb_usb] [] dib0700_probe+0x14c/0x410 [dvb_usb_dib0700] [] usb_probe_interface+0x45d/0x940 [] driver_probe_device+0x21a/0xc30 [] __driver_attach+0x121/0x160 [] bus_for_each_dev+0x11f/0x1a0 [] driver_attach+0x3d/0x50 [] bus_add_driver+0x4c9/0x770 [] driver_register+0x18c/0x3b0 [] usb_register_driver+0x1f8/0x440 [] dib0700_driver_init+0x1e/0x1000 [dvb_usb_dib0700] [] do_one_initcall+0x141/0x300 [] do_init_module+0x1d0/0x5ad [] load_module+0x6666/0x9ba0 [] SyS_finit_module+0x108/0x130 [] entry_SYSCALL_64_fastpath+0x16/0x76 other info that might help us debug this: Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(&dev->lock); lock(s_active#171); lock(&dev->lock); lock(s_active#171); *** DEADLOCK *** 3 locks held by systemd-udevd/2681: #0: (&dev->mutex){......}, at: [] __driver_attach+0xa3/0x160 #1: (&dev->mutex){......}, at: [] __driver_attach+0xb1/0x160 #2: (&dev->lock){+.+.+.}, at: [] rc_register_device+0xb2f/0x1450 [rc_core] In this specific case, some error happened during device init, causing IR to be disabled. Let's fix it by adding a var that will tell when the device is initialized. Any calls before that will return a -EINVAL. That should prevent the race issues. Signed-off-by: Mauro Carvalho Chehab --- drivers/media/rc/rc-main.c | 45 ++++++++++++++++++++++++++------------------- include/media/rc-core.h | 2 ++ 2 files changed, 28 insertions(+), 19 deletions(-) diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c index 1042fa331a07..dcf20d9cbe09 100644 --- a/drivers/media/rc/rc-main.c +++ b/drivers/media/rc/rc-main.c @@ -723,12 +723,18 @@ int rc_open(struct rc_dev *rdev) return -EINVAL; mutex_lock(&rdev->lock); + if (!rdev->initialized) { + rval = -EINVAL; + goto unlock; + } + if (!rdev->users++ && rdev->open != NULL) rval = rdev->open(rdev); if (rval) rdev->users--; +unlock: mutex_unlock(&rdev->lock); return rval; @@ -874,6 +880,10 @@ static ssize_t show_protocols(struct device *device, return -EINVAL; mutex_lock(&dev->lock); + if (!dev->initialized) { + mutex_unlock(&dev->lock); + return -EINVAL; + } if (fattr->type == RC_FILTER_NORMAL) { enabled = dev->enabled_protocols; @@ -1074,6 +1084,10 @@ static ssize_t store_protocols(struct device *device, } mutex_lock(&dev->lock); + if (!dev->initialized) { + rc = -EINVAL; + goto out; + } old_protocols = *current_protocols; new_protocols = old_protocols; @@ -1154,12 +1168,17 @@ static ssize_t show_filter(struct device *device, if (!dev) return -EINVAL; + mutex_lock(&dev->lock); + if (!dev->initialized) { + mutex_unlock(&dev->lock); + return -EINVAL; + } + if (fattr->type == RC_FILTER_NORMAL) filter = &dev->scancode_filter; else filter = &dev->scancode_wakeup_filter; - mutex_lock(&dev->lock); if (fattr->mask) val = filter->mask; else @@ -1222,6 +1241,10 @@ static ssize_t store_filter(struct device *device, return -EINVAL; mutex_lock(&dev->lock); + if (!dev->initialized) { + ret = -EINVAL; + goto unlock; + } new_filter = *filter; if (fattr->mask) @@ -1419,14 +1442,6 @@ int rc_register_device(struct rc_dev *dev) dev->sysfs_groups[attr++] = &rc_dev_wakeup_protocol_attr_grp; dev->sysfs_groups[attr++] = NULL; - /* - * Take the lock here, as the device sysfs node will appear - * when device_add() is called, which may trigger an ir-keytable udev - * rule, which will in turn call show_protocols and access - * dev->enabled_protocols before it has been initialized. - */ - mutex_lock(&dev->lock); - rc = device_add(&dev->dev); if (rc) goto out_unlock; @@ -1440,13 +1455,7 @@ int rc_register_device(struct rc_dev *dev) dev->input_dev->phys = dev->input_phys; dev->input_dev->name = dev->input_name; - /* input_register_device can call ir_open, so unlock mutex here */ - mutex_unlock(&dev->lock); - rc = input_register_device(dev->input_dev); - - mutex_lock(&dev->lock); - if (rc) goto out_table; @@ -1475,10 +1484,7 @@ int rc_register_device(struct rc_dev *dev) request_module_nowait("ir-lirc-codec"); raw_init = true; } - /* calls ir_register_device so unlock mutex here*/ - mutex_unlock(&dev->lock); rc = ir_raw_event_register(dev); - mutex_lock(&dev->lock); if (rc < 0) goto out_input; } @@ -1491,6 +1497,8 @@ int rc_register_device(struct rc_dev *dev) dev->enabled_protocols = rc_type; } + mutex_lock(&dev->lock); + dev->initialized = true; mutex_unlock(&dev->lock); IR_dprintk(1, "Registered rc%u (driver: %s, remote: %s, mode %s)\n", @@ -1512,7 +1520,6 @@ out_table: out_dev: device_del(&dev->dev); out_unlock: - mutex_unlock(&dev->lock); ida_simple_remove(&rc_ida, minor); return rc; } diff --git a/include/media/rc-core.h b/include/media/rc-core.h index f6494709e230..c41dd7018fa8 100644 --- a/include/media/rc-core.h +++ b/include/media/rc-core.h @@ -60,6 +60,7 @@ enum rc_filter_type { /** * struct rc_dev - represents a remote control device * @dev: driver model's view of this device + * @initialized: true if the device init has completed * @sysfs_groups: sysfs attribute groups * @input_name: name of the input child device * @input_phys: physical path to the input child device @@ -121,6 +122,7 @@ enum rc_filter_type { */ struct rc_dev { struct device dev; + bool initialized; const struct attribute_group *sysfs_groups[5]; const char *input_name; const char *input_phys;