| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250418055820.3689408-4-hejunhao3@huawei.com>
Date: Fri, 18 Apr 2025 13:58:19 +0800
From: Junhao He <hejunhao3@...wei.com>
To: <suzuki.poulose@....com>, <james.clark@....com>,
<anshuman.khandual@....com>, <mike.leach@...aro.org>, <leo.yan@....com>
CC: <coresight@...ts.linaro.org>, <linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <linuxarm@...wei.com>,
<jonathan.cameron@...wei.com>, <yangyicong@...wei.com>,
<hejunhao3@...wei.com>
Subject: [PATCH 3/4] coresight: tmc: refactor the tmc-etr mode setting
When trying to run perf and sysfs mode simultaneously, the WARN_ON()
in tmc_etr_enable_hw() is triggered sometimes:
WARNING: CPU: 42 PID: 3911571 at drivers/hwtracing/coresight/coresight-tmc-etr.c:1060 tmc_etr_enable_hw+0xc0/0xd8 [coresight_tmc]
[..snip..]
Call trace:
tmc_etr_enable_hw+0xc0/0xd8 [coresight_tmc] (P)
tmc_enable_etr_sink+0x11c/0x250 [coresight_tmc] (L)
tmc_enable_etr_sink+0x11c/0x250 [coresight_tmc]
coresight_enable_path+0x1c8/0x218 [coresight]
coresight_enable_sysfs+0xa4/0x228 [coresight]
enable_source_store+0x58/0xa8 [coresight]
dev_attr_store+0x20/0x40
sysfs_kf_write+0x4c/0x68
kernfs_fop_write_iter+0x120/0x1b8
vfs_write+0x2c8/0x388
ksys_write+0x74/0x108
__arm64_sys_write+0x24/0x38
el0_svc_common.constprop.0+0x64/0x148
do_el0_svc+0x24/0x38
el0_svc+0x3c/0x130
el0t_64_sync_handler+0xc8/0xd0
el0t_64_sync+0x1ac/0x1b0
---[ end trace 0000000000000000 ]---
Since the sysfs buffer allocation and the hardware enablement is not
in the same critical region, it's possible to race with the perf
mode:
[sysfs mode] [perf mode]
tmc_etr_get_sysfs_buffer()
spin_lock(&drvdata->spinlock)
[sysfs buffer allocation]
spin_unlock(&drvdata->spinlock)
spin_lock(&drvdata->spinlock)
tmc_etr_enable_hw()
drvdata->etr_buf = etr_perf->etr_buf
spin_unlock(&drvdata->spinlock)
spin_lock(&drvdata->spinlock)
tmc_etr_enable_hw()
WARN_ON(drvdata->etr_buf) // WARN sicne etr_buf initialized at
the perf side
spin_unlock(&drvdata->spinlock)
To resolve this, configure the tmc-etr mode before invoking
`enable_perf()` or sysfs interfaces. Prior to mode configuration,
explicitly check if the tmc-etr sink is already enabled in a
different mode to prevent race conditions between mode transitions.
Furthermore, enforce spinlock protection around the critical
sections to serialize concurrent accesses from sysfs and perf
subsystems.
Fixes: 296b01fd106e ("coresight: Refactor out buffer allocation function for ETR")
Reported-by: Yicong Yang <yangyicong@...ilicon.com>
Closes: https://lore.kernel.org/linux-arm-kernel/20241202092419.11777-2-yangyicong@huawei.com/
Signed-off-by: Junhao He <hejunhao3@...wei.com>
---
.../hwtracing/coresight/coresight-tmc-etr.c | 77 +++++++++++--------
1 file changed, 47 insertions(+), 30 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index a48bb85d0e7f..3d94d64cacaa 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -1190,11 +1190,6 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev)
spin_lock_irqsave(&drvdata->spinlock, flags);
}
- if (drvdata->reading || coresight_get_mode(csdev) == CS_MODE_PERF) {
- ret = -EBUSY;
- goto out;
- }
-
/*
* If we don't have a buffer or it doesn't match the requested size,
* use the buffer allocated above. Otherwise reuse the existing buffer.
@@ -1205,7 +1200,6 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev)
drvdata->sysfs_buf = new_buf;
}
-out:
spin_unlock_irqrestore(&drvdata->spinlock, flags);
/* Free memory outside the spinlock if need be */
@@ -1216,7 +1210,7 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev)
static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
{
- int ret = 0;
+ int ret;
unsigned long flags;
struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
struct etr_buf *sysfs_buf = tmc_etr_get_sysfs_buffer(csdev);
@@ -1226,23 +1220,10 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
spin_lock_irqsave(&drvdata->spinlock, flags);
- /*
- * In sysFS mode we can have multiple writers per sink. Since this
- * sink is already enabled no memory is needed and the HW need not be
- * touched, even if the buffer size has changed.
- */
- if (coresight_get_mode(csdev) == CS_MODE_SYSFS) {
- csdev->refcnt++;
- goto out;
- }
-
ret = tmc_etr_enable_hw(drvdata, sysfs_buf);
- if (!ret) {
- coresight_set_mode(csdev, CS_MODE_SYSFS);
+ if (!ret)
csdev->refcnt++;
- }
-out:
spin_unlock_irqrestore(&drvdata->spinlock, flags);
if (!ret)
@@ -1652,11 +1633,6 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
struct etr_perf_buffer *etr_perf = etm_perf_sink_config(handle);
spin_lock_irqsave(&drvdata->spinlock, flags);
- /* Don't use this sink if it is already claimed by sysFS */
- if (coresight_get_mode(csdev) == CS_MODE_SYSFS) {
- rc = -EBUSY;
- goto unlock_out;
- }
if (WARN_ON(!etr_perf || !etr_perf->etr_buf)) {
rc = -EINVAL;
@@ -1685,7 +1661,6 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
if (!rc) {
/* Associate with monitored process. */
drvdata->pid = pid;
- coresight_set_mode(csdev, CS_MODE_PERF);
drvdata->perf_buf = etr_perf->etr_buf;
csdev->refcnt++;
}
@@ -1698,14 +1673,56 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
static int tmc_enable_etr_sink(struct coresight_device *csdev,
enum cs_mode mode, void *data)
{
+ struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+ enum cs_mode old_mode;
+ int rc;
+
+ scoped_guard(spinlock_irqsave, &drvdata->spinlock) {
+ old_mode = coresight_get_mode(csdev);
+ if (old_mode != CS_MODE_DISABLED && old_mode != mode)
+ return -EBUSY;
+
+ if (drvdata->reading)
+ return -EBUSY;
+
+ /*
+ * In sysFS mode we can have multiple writers per sink. Since this
+ * sink is already enabled no memory is needed and the HW need not be
+ * touched, even if the buffer size has changed.
+ */
+ if (old_mode == CS_MODE_SYSFS) {
+ csdev->refcnt++;
+ return 0;
+ }
+
+ /*
+ * minor note:
+ * When sysfs-task1 get locked, it setup the mode first. Then
+ * sysfs-task2 gets locked，it will directly return success even
+ * when the tmc-etr is not enabled at this moment. Ultimately,
+ * sysfs-task1 will still successfully enable tmc-etr.
+ * This is a transient state and does not cause an anomaly.
+ */
+ coresight_set_mode(csdev, mode);
+ }
+
switch (mode) {
case CS_MODE_SYSFS:
- return tmc_enable_etr_sink_sysfs(csdev);
+ rc = tmc_enable_etr_sink_sysfs(csdev);
+ break;
case CS_MODE_PERF:
- return tmc_enable_etr_sink_perf(csdev, data);
+ rc = tmc_enable_etr_sink_perf(csdev, data);
+ break;
default:
- return -EINVAL;
+ rc = -EINVAL;
}
+
+ scoped_guard(spinlock_irqsave, &drvdata->spinlock) {
+ if (rc && old_mode != mode)
+ coresight_set_mode(csdev, old_mode);
+ }
+
+ return rc;
}
static int tmc_disable_etr_sink(struct coresight_device *csdev)
--
2.33.0
Powered by blists - more mailing lists