From: Sasha Levin <sashal@kernel.org>
To: linux-kernel@vger.kernel.org, stable@vger.kernel.org
Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
Liwei Song <liwei.song@windriver.com>,
Hans de Goede <hdegoede@redhat.com>,
Sasha Levin <sashal@kernel.org>,
markgross@kernel.org, andriy.shevchenko@linux.intel.com,
hpa@redhat.com, platform-driver-x86@vger.kernel.org
Subject: [PATCH AUTOSEL 5.10 02/27] platform/x86: ISST: Fix possible circular locking dependency detected
Date: Wed, 9 Feb 2022 13:40:38 -0500 [thread overview]
Message-ID: <20220209184103.47635-2-sashal@kernel.org> (raw)
In-Reply-To: <20220209184103.47635-1-sashal@kernel.org>
From: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
[ Upstream commit 17da2d5f93692086dd096a975225ffd5622d0bf8 ]
As reported:
[ 256.104522] ======================================================
[ 256.113783] WARNING: possible circular locking dependency detected
[ 256.120093] 5.16.0-rc6-yocto-standard+ #99 Not tainted
[ 256.125362] ------------------------------------------------------
[ 256.131673] intel-speed-sel/844 is trying to acquire lock:
[ 256.137290] ffffffffc036f0d0 (punit_misc_dev_lock){+.+.}-{3:3}, at: isst_if_open+0x18/0x90 [isst_if_common]
[ 256.147171]
[ 256.147171] but task is already holding lock:
[ 256.153135] ffffffff8ee7cb50 (misc_mtx){+.+.}-{3:3}, at: misc_open+0x2a/0x170
[ 256.160407]
[ 256.160407] which lock already depends on the new lock.
[ 256.160407]
[ 256.168712]
[ 256.168712] the existing dependency chain (in reverse order) is:
[ 256.176327]
[ 256.176327] -> #1 (misc_mtx){+.+.}-{3:3}:
[ 256.181946] lock_acquire+0x1e6/0x330
[ 256.186265] __mutex_lock+0x9b/0x9b0
[ 256.190497] mutex_lock_nested+0x1b/0x20
[ 256.195075] misc_register+0x32/0x1a0
[ 256.199390] isst_if_cdev_register+0x65/0x180 [isst_if_common]
[ 256.205878] isst_if_probe+0x144/0x16e [isst_if_mmio]
...
[ 256.241976]
[ 256.241976] -> #0 (punit_misc_dev_lock){+.+.}-{3:3}:
[ 256.248552] validate_chain+0xbc6/0x1750
[ 256.253131] __lock_acquire+0x88c/0xc10
[ 256.257618] lock_acquire+0x1e6/0x330
[ 256.261933] __mutex_lock+0x9b/0x9b0
[ 256.266165] mutex_lock_nested+0x1b/0x20
[ 256.270739] isst_if_open+0x18/0x90 [isst_if_common]
[ 256.276356] misc_open+0x100/0x170
[ 256.280409] chrdev_open+0xa5/0x1e0
...
The call sequence suggested that misc_device /dev file can be opened
before misc device is yet to be registered, which is done only once.
Here punit_misc_dev_lock was used as common lock, to protect the
registration by multiple ISST HW drivers, one time setup, prevent
duplicate registry of misc device and prevent load/unload when device
is open.
We can split into locks:
- One which just prevent duplicate call to misc_register() and one
time setup. Also never call again if the misc_register() failed or
required one time setup is failed. This lock is not shared with
any misc device callbacks.
- The other lock protects registry, load and unload of HW drivers.
Sequence in isst_if_cdev_register()
- Register callbacks under punit_misc_dev_open_lock
- Call isst_misc_reg() which registers misc_device on the first
registry which is under punit_misc_dev_reg_lock, which is not
shared with callbacks.
Sequence in isst_if_cdev_unregister
Just opposite of isst_if_cdev_register
Reported-and-tested-by: Liwei Song <liwei.song@windriver.com>
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Link: https://lore.kernel.org/r/20220112022521.54669-1-srinivas.pandruvada@linux.intel.com
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
.../intel_speed_select_if/isst_if_common.c | 97 ++++++++++++-------
1 file changed, 63 insertions(+), 34 deletions(-)
diff --git a/drivers/platform/x86/intel_speed_select_if/isst_if_common.c b/drivers/platform/x86/intel_speed_select_if/isst_if_common.c
index 0c2aa22c7a12e..407afafc7e83f 100644
--- a/drivers/platform/x86/intel_speed_select_if/isst_if_common.c
+++ b/drivers/platform/x86/intel_speed_select_if/isst_if_common.c
@@ -532,7 +532,10 @@ static long isst_if_def_ioctl(struct file *file, unsigned int cmd,
return ret;
}
-static DEFINE_MUTEX(punit_misc_dev_lock);
+/* Lock to prevent module registration when already opened by user space */
+static DEFINE_MUTEX(punit_misc_dev_open_lock);
+/* Lock to allow one share misc device for all ISST interace */
+static DEFINE_MUTEX(punit_misc_dev_reg_lock);
static int misc_usage_count;
static int misc_device_ret;
static int misc_device_open;
@@ -542,7 +545,7 @@ static int isst_if_open(struct inode *inode, struct file *file)
int i, ret = 0;
/* Fail open, if a module is going away */
- mutex_lock(&punit_misc_dev_lock);
+ mutex_lock(&punit_misc_dev_open_lock);
for (i = 0; i < ISST_IF_DEV_MAX; ++i) {
struct isst_if_cmd_cb *cb = &punit_callbacks[i];
@@ -564,7 +567,7 @@ static int isst_if_open(struct inode *inode, struct file *file)
} else {
misc_device_open++;
}
- mutex_unlock(&punit_misc_dev_lock);
+ mutex_unlock(&punit_misc_dev_open_lock);
return ret;
}
@@ -573,7 +576,7 @@ static int isst_if_relase(struct inode *inode, struct file *f)
{
int i;
- mutex_lock(&punit_misc_dev_lock);
+ mutex_lock(&punit_misc_dev_open_lock);
misc_device_open--;
for (i = 0; i < ISST_IF_DEV_MAX; ++i) {
struct isst_if_cmd_cb *cb = &punit_callbacks[i];
@@ -581,7 +584,7 @@ static int isst_if_relase(struct inode *inode, struct file *f)
if (cb->registered)
module_put(cb->owner);
}
- mutex_unlock(&punit_misc_dev_lock);
+ mutex_unlock(&punit_misc_dev_open_lock);
return 0;
}
@@ -598,6 +601,43 @@ static struct miscdevice isst_if_char_driver = {
.fops = &isst_if_char_driver_ops,
};
+static int isst_misc_reg(void)
+{
+ mutex_lock(&punit_misc_dev_reg_lock);
+ if (misc_device_ret)
+ goto unlock_exit;
+
+ if (!misc_usage_count) {
+ misc_device_ret = isst_if_cpu_info_init();
+ if (misc_device_ret)
+ goto unlock_exit;
+
+ misc_device_ret = misc_register(&isst_if_char_driver);
+ if (misc_device_ret) {
+ isst_if_cpu_info_exit();
+ goto unlock_exit;
+ }
+ }
+ misc_usage_count++;
+
+unlock_exit:
+ mutex_unlock(&punit_misc_dev_reg_lock);
+
+ return misc_device_ret;
+}
+
+static void isst_misc_unreg(void)
+{
+ mutex_lock(&punit_misc_dev_reg_lock);
+ if (misc_usage_count)
+ misc_usage_count--;
+ if (!misc_usage_count && !misc_device_ret) {
+ misc_deregister(&isst_if_char_driver);
+ isst_if_cpu_info_exit();
+ }
+ mutex_unlock(&punit_misc_dev_reg_lock);
+}
+
/**
* isst_if_cdev_register() - Register callback for IOCTL
* @device_type: The device type this callback handling.
@@ -615,38 +655,31 @@ static struct miscdevice isst_if_char_driver = {
*/
int isst_if_cdev_register(int device_type, struct isst_if_cmd_cb *cb)
{
- if (misc_device_ret)
- return misc_device_ret;
+ int ret;
if (device_type >= ISST_IF_DEV_MAX)
return -EINVAL;
- mutex_lock(&punit_misc_dev_lock);
+ mutex_lock(&punit_misc_dev_open_lock);
+ /* Device is already open, we don't want to add new callbacks */
if (misc_device_open) {
- mutex_unlock(&punit_misc_dev_lock);
+ mutex_unlock(&punit_misc_dev_open_lock);
return -EAGAIN;
}
- if (!misc_usage_count) {
- int ret;
-
- misc_device_ret = misc_register(&isst_if_char_driver);
- if (misc_device_ret)
- goto unlock_exit;
-
- ret = isst_if_cpu_info_init();
- if (ret) {
- misc_deregister(&isst_if_char_driver);
- misc_device_ret = ret;
- goto unlock_exit;
- }
- }
memcpy(&punit_callbacks[device_type], cb, sizeof(*cb));
punit_callbacks[device_type].registered = 1;
- misc_usage_count++;
-unlock_exit:
- mutex_unlock(&punit_misc_dev_lock);
+ mutex_unlock(&punit_misc_dev_open_lock);
- return misc_device_ret;
+ ret = isst_misc_reg();
+ if (ret) {
+ /*
+ * No need of mutex as the misc device register failed
+ * as no one can open device yet. Hence no contention.
+ */
+ punit_callbacks[device_type].registered = 0;
+ return ret;
+ }
+ return 0;
}
EXPORT_SYMBOL_GPL(isst_if_cdev_register);
@@ -661,16 +694,12 @@ EXPORT_SYMBOL_GPL(isst_if_cdev_register);
*/
void isst_if_cdev_unregister(int device_type)
{
- mutex_lock(&punit_misc_dev_lock);
- misc_usage_count--;
+ isst_misc_unreg();
+ mutex_lock(&punit_misc_dev_open_lock);
punit_callbacks[device_type].registered = 0;
if (device_type == ISST_IF_DEV_MBOX)
isst_delete_hash();
- if (!misc_usage_count && !misc_device_ret) {
- misc_deregister(&isst_if_char_driver);
- isst_if_cpu_info_exit();
- }
- mutex_unlock(&punit_misc_dev_lock);
+ mutex_unlock(&punit_misc_dev_open_lock);
}
EXPORT_SYMBOL_GPL(isst_if_cdev_unregister);
--
2.34.1
next prev parent reply other threads:[~2022-02-09 18:41 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-09 18:40 [PATCH AUTOSEL 5.10 01/27] platform/x86: touchscreen_dmi: Add info for the RWC NANOTE P8 AY07J 2-in-1 Sasha Levin
2022-02-09 18:40 ` Sasha Levin [this message]
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 03/27] selftests: rtc: Increase test timeout so that all tests run Sasha Levin
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 04/27] kselftest: signal all child processes Sasha Levin
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 05/27] net: ieee802154: at86rf230: Stop leaking skb's Sasha Levin
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 06/27] selftests/zram: Skip max_comp_streams interface on newer kernel Sasha Levin
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 07/27] selftests/zram01.sh: Fix compression ratio calculation Sasha Levin
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 08/27] selftests/zram: Adapt the situation that /dev/zram0 is being used Sasha Levin
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 09/27] selftests: openat2: Print also errno in failure messages Sasha Levin
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 10/27] selftests: openat2: Add missing dependency in Makefile Sasha Levin
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 11/27] selftests: openat2: Skip testcases that fail with EOPNOTSUPP Sasha Levin
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 12/27] selftests: skip mincore.check_file_mmap when fs lacks needed support Sasha Levin
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 13/27] ax25: improve the incomplete fix to avoid UAF and NPD bugs Sasha Levin
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 14/27] vfs: make freeze_super abort when sync_filesystem returns error Sasha Levin
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 15/27] quota: make dquot_quota_sync return errors from ->sync_fs Sasha Levin
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 16/27] btrfs: tree-checker: check item_size for dev_item Sasha Levin
2022-02-18 10:36 ` Greg KH
2022-02-18 11:25 ` Su Yue
2022-02-18 13:04 ` Greg KH
2022-02-23 17:20 ` David Sterba
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 17/27] iommu: Fix potential use-after-free during probe Sasha Levin
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 18/27] scsi: pm8001: Fix use-after-free for aborted TMF sas_task Sasha Levin
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 19/27] scsi: pm8001: Fix use-after-free for aborted SSP/STP sas_task Sasha Levin
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 20/27] nvme: fix a possible use-after-free in controller reset during load Sasha Levin
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 21/27] nvme-tcp: fix possible use-after-free in transport error_recovery work Sasha Levin
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 22/27] nvme-rdma: " Sasha Levin
2022-02-09 18:40 ` [PATCH AUTOSEL 5.10 23/27] drm/amdgpu: fix logic inversion in check Sasha Levin
2022-02-09 18:41 ` [PATCH AUTOSEL 5.10 24/27] x86/Xen: streamline (and fix) PV CPU enumeration Sasha Levin
2022-02-09 18:41 ` [PATCH AUTOSEL 5.10 25/27] Revert "module, async: async_synchronize_full() on module init iff async is used" Sasha Levin
2022-02-09 18:41 ` [PATCH AUTOSEL 5.10 26/27] gcc-plugins/stackleak: Use noinstr in favor of notrace Sasha Levin
2022-02-09 18:41 ` [PATCH AUTOSEL 5.10 27/27] random: wake up /dev/random writers after zap Sasha Levin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220209184103.47635-2-sashal@kernel.org \
--to=sashal@kernel.org \
--cc=andriy.shevchenko@linux.intel.com \
--cc=hdegoede@redhat.com \
--cc=hpa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=liwei.song@windriver.com \
--cc=markgross@kernel.org \
--cc=platform-driver-x86@vger.kernel.org \
--cc=srinivas.pandruvada@linux.intel.com \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox