On Mon, Oct 20, 2025 at 05:06:46PM +0800, Xiaoqi Zhuang wrote:

When ETR is enabled as CS_MODE_SYSFS, if the buffer size is changed and enabled again, currently sysfs_buf will point to the newly allocated memory(buf_new) and free the old memory(buf_old). But the etr_buf that is being used by the ETR remains pointed to buf_old, not updated to buf_new. In this case, it will result in a memory use-after-free issue.

I struggled to understand how to reproduce the issue under the condition "if the buffer size is changed and enabled again."

I don't think the flow below where the trace is re-enabled would cause an issue:

- Step 1: Enable trace path between ETM0 -> ETR0; - Step 2: Change the buffer size for ETR0; - Step 3: Disable trace path between ETM0 -> ETR0; - Step 4: Enable again trace path between ETM0 -> ETR0.

In this case, step3 releases the buffer and update "drvdata->etr_buf" to NULL, and step 4 allocates a new buffer and assign it to "drvdata->etr_buf".

The problem should occur when operating on two trace paths, E.g.,

- Step 1: Enable trace path between ETM0 -> ETR0; - Step 2: Change the buffer size for ETR0; - Step 3: Enable trace path between ETM1 -> ETR0;

In step3, the driver releases the existed buffer and allocate a new one. At the meantime, "drvdata->etr_buf" still holds the buffer allocated in step 1.

Fix this by checking ETR's mode before updating and releasing buf_old, if the mode is CS_MODE_SYSFS, then skip updating and releasing it.

Given that we now have a couple of reported issues related to ETR mode, I'd like to refactor the ETR mode handling and its reference counting thoroughly. I've drafted a large change (it's quite big, but we can split it into small patches if we agree to proceed).

Thanks for reporting the issue!

Leo

---8<---

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index b07fcdb3fe1a..d0fac958c614 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1241,6 +1241,8 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev) struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); struct etr_buf *sysfs_buf = NULL, *new_buf = NULL, *free_buf = NULL;

+ WARN_ON(coresight_get_mode(csdev) != CS_MODE_SYSFS); + /* * If we are enabling the ETR from disabled state, we need to make * sure we have a buffer with the right size. The etr_buf is not reset @@ -1263,7 +1265,7 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev) raw_spin_lock_irqsave(&drvdata->spinlock, flags); }

- if (drvdata->reading || coresight_get_mode(csdev) == CS_MODE_PERF) { + if (drvdata->reading) { ret = -EBUSY; goto out; } @@ -1292,30 +1294,14 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) int ret = 0; unsigned long flags; struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); - struct etr_buf *sysfs_buf = tmc_etr_get_sysfs_buffer(csdev); + struct etr_buf *sysfs_buf;

+ sysfs_buf = tmc_etr_get_sysfs_buffer(csdev); if (IS_ERR(sysfs_buf)) return PTR_ERR(sysfs_buf);

raw_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); - csdev->refcnt++; - } - -out: raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);

if (!ret) @@ -1735,11 +1721,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);

raw_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; @@ -1759,18 +1740,14 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data) * No HW configuration is needed if the sink is already in * use for this session. */ - if (drvdata->pid == pid) { - csdev->refcnt++; + if (drvdata->pid == pid) goto unlock_out; - }

rc = tmc_etr_enable_hw(drvdata, etr_perf->etr_buf); 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++; }

unlock_out: @@ -1778,17 +1755,76 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data) return rc; }

+static int tmc_acquire_mode(struct coresight_device *csdev, enum cs_mode mode) +{ + struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); + + if (mode != CS_MODE_SYSFS && mode != CS_MODE_PERF) + return -EINVAL; + + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock); + + if (coresight_get_mode(csdev) == CS_MODE_DISABLED) { + if (!csdev->refcnt) + coresight_set_mode(csdev, mode); + csdev->refcnt++; + } else if (coresight_get_mode(csdev) != mode) { + ret = -EBUSY; + } + + return csdev->refcnt; +} + +static void tmc_release_mode(struct coresight_device *csdev, enum cs_mode mode) +{ + struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); + + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock); + + if (WARN_ON(coresight_get_mode(csdev) != mode)) + return; + + csdev->refcnt--; + if (!csdev->refcnt) + coresight_set_mode(csdev, CS_MODE_DISABLED); +} + static int tmc_enable_etr_sink(struct coresight_device *csdev, enum cs_mode mode, void *data) { + unsigned long flags; + struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); + int ret; + + ret = tmc_acquire_mode(csdev, mode); + if (ret < 0) + return ret; + + /* + * For sysfs mode, the higher level mutex ensures exclusively + * enabling sink, it is safe to bail out if this is not the + * first time to enable sink. + * + * A perf session can enable the same sink simultaneously, fall + * through to call tmc_enable_etr_sink_perf() to ensure the sink + * has been enabled. + */ + if (mode == CS_MODE_SYSFS && ret > 1) + return 0; + switch (mode) { case CS_MODE_SYSFS: - return tmc_enable_etr_sink_sysfs(csdev); + ret = tmc_enable_etr_sink_sysfs(csdev); case CS_MODE_PERF: - return tmc_enable_etr_sink_perf(csdev, data); + ret = tmc_enable_etr_sink_perf(csdev, data); default: - return -EINVAL; + ret = -EINVAL; } + + if (ret) + tmc_release_mode(csdev, mode); + + return ret; }

static int tmc_disable_etr_sink(struct coresight_device *csdev) @@ -1796,30 +1832,20 @@ static int tmc_disable_etr_sink(struct coresight_device *csdev) unsigned long flags; struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);

- raw_spin_lock_irqsave(&drvdata->spinlock, flags); + tmc_release_mode(csdev, mode);

- if (drvdata->reading) { - raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); - return -EBUSY; - } + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock);

- csdev->refcnt--; - if (csdev->refcnt) { - raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); + if (csdev->refcnt || drvdata->reading) return -EBUSY; - }

- /* Complain if we (somehow) got out of sync */ - WARN_ON_ONCE(coresight_get_mode(csdev) == CS_MODE_DISABLED); + if (drvdata->pid == -1) + return 0; + tmc_etr_disable_hw(drvdata); - /* Dissociate from monitored process. */ - drvdata->pid = -1; - coresight_set_mode(csdev, CS_MODE_DISABLED); /* Reset perf specific data */ drvdata->perf_buf = NULL;

- raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); - dev_dbg(&csdev->dev, "TMC-ETR disabled\n"); return 0; }