Re: [PATCH v21 7/8] coresight: ctcu: enable byte-cntr for TMC ETR devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] coresight: ctcu: race in tmc_read_unprepare_byte_cntr() during close
- [High] coresight: ctcu: zero-length read in tmc_byte_cntr_get_data() drops buffers and loops
- [High] coresight: ctcu: unbind in ctcu_platform_remove() reads live DMA buffer
- [High] coresight: ctcu: unsynchronized concurrent read() in tmc_byte_cntr_get_data()
- [High] coresight: ctcu: UAF race between byte_cntr_sysfs_ops and CTCU teardown
- [High] coresight: ctcu: unsynchronized state transitions in byte-cntr lifecycle
- [High] coresight: tmc: broken fallback and race in tmc_read_prepare_etr()
- [Medium] coresight: ctcu: ctcu_byte_cntr_start() corrupts active read or trace state
- [Medium] coresight: ctcu: unhandled devm_add_action_or_reset() failure in ctcu_byte_cntr_init()
- [Medium] coresight: tmc: tmc_read() converts negative errors into EOF
--

commit 21dfce27a3a23b8dd103a86f6d7455d0e9197bff
Author: Jie Gan <jie.gan@oss.qualcomm.com>

coresight: ctcu: enable byte-cntr for TMC ETR devices

This commit enables the CTCU byte-cntr functionality to transfer trace data
from TMC ETR buffers to userspace via interrupt-driven buffer switching in
sysfs mode.

> diff --git a/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c b/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
> new file mode 100644
> index 0000000000000..bff87e776a755
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
[ ... ]
> +static struct ctcu_byte_cntr *ctcu_get_byte_cntr(struct coresight_device *ctcu,
> +						 struct coresight_device *etr)
> +{
> +	struct ctcu_byte_cntr *byte_cntr_data;
> +	struct ctcu_drvdata *drvdata;
> +	int port;
> +
> +	/* The ETR may not be connected to a CTCU device */
> +	if (!ctcu)
> +		return NULL;
> +
> +	drvdata = dev_get_drvdata(ctcu->dev.parent);
> +	port = coresight_get_in_port(etr, ctcu);
> +	if (port < 0 || port >= ETR_MAX_NUM)
> +		return NULL;
> +
> +	byte_cntr_data = &drvdata->byte_cntr_data[port];
> +	/* The port is not initialized by ctcu_byte_cntr_init() */
> +	if (!byte_cntr_data->ctcu_drvdata)
> +		return NULL;
> +
> +	return byte_cntr_data;
> +}

[Severity: High]
As reported by Sashiko on v19 [1], can concurrent /dev/tmc_etr* file
operations race with CTCU device removal or module unload and trigger a
use-after-free?

Because CONFIG_CORESIGHT_LINK_AND_SINK_TMC and CONFIG_CORESIGHT_CTCU are
separate modules, opening /dev/tmc_etr* pins coresight-tmc rather than
coresight-ctcu. In coresight-tmc-etr.c, tmc_read_prepare_etr(),
tmc_etr_get_sysfs_trace(), and tmc_read_unprepare_etr() load
READ_ONCE(byte_cntr_sysfs_ops) and invoke its callbacks without holding a
lock, coresight_mutex, or a module reference prior to the call:

1. In tmc_read_prepare_byte_cntr(), try_module_get(THIS_MODULE) and
reinit_completion(&byte_cntr_data->session_done) are only executed after
tmc_read_prepare_etr() has dereferenced byte_cntr_ops->read_prepare and
after tmc_read_prepare_byte_cntr() has called tmc_etr_get_ctcu_device(),
ctcu_get_byte_cntr(), and raw_spin_lock_irqsave(&byte_cntr_data->spin_lock,
flags). Similarly, in tmc_read_unprepare_byte_cntr(),
module_put(THIS_MODULE) and the function return execute after
complete_all(&byte_cntr_data->session_done).

2. For normal non-byte-cntr ETR sysfs read sessions (for instance when
reading /dev/tmc_etr* after stopping the trace path or when irq_enabled is
0), tmc_read_prepare_byte_cntr() returns -EINVAL without pinning
THIS_MODULE or reinitializing session_done, yet every subsequent read() via
tmc_etr_get_sysfs_trace() -> tmc_byte_cntr_get_data() and close() via
tmc_read_unprepare_etr() -> tmc_read_unprepare_byte_cntr() still invokes
byte_cntr_ops and calls tmc_etr_get_ctcu_device() and ctcu_get_byte_cntr()
while session_done is already completed and coresight-ctcu can have a
module refcount of 0.

3. When ctcu_platform_remove() runs concurrently,
wait_for_completion(&byte_cntr_data->session_done) returns immediately if
no byte-cntr session is active. ctcu_platform_remove() then calls
ctcu_remove() -> coresight_unregister(drvdata->csdev) before
devres_release_all() runs ctcu_byte_cntr_put_ops() to clear
byte_cntr_sysfs_ops (and on multi-CTCU systems, byte_cntr_sysfs_ops remains
set when one CTCU device is removed). Inside coresight_unregister(),
coresight_release_platform_data() frees ctcu->pdata->in_conns and
ctcu->pdata, and device_unregister() frees ctcu; devres_release_all() then
frees drvdata and clears dev->driver_data. Concurrent calls to
tmc_etr_get_ctcu_device() and ctcu_get_byte_cntr() can therefore
dereference freed ctcu, freed ctcu->pdata->in_conns in
coresight_get_in_port(), NULL or freed drvdata, or unmapped byte_cntr_ops.

> +
> +/*
> + * ctcu_byte_cntr_get_data() - reads data from the deactivated and filled buffer.
> + * The byte-cntr reading work reads data from the deactivated and filled buffer.
> + * The read operation waits for a buffer to become available, either filled or
> + * upon timeout, and then reads trace data from the synced buffer.
> + */
> +static ssize_t tmc_byte_cntr_get_data(struct tmc_drvdata *etr_drvdata, loff_t pos,
> +				      size_t len, char **bufpp)
> +{
> +	struct coresight_device *ctcu = tmc_etr_get_ctcu_device(etr_drvdata);
> +	struct device *dev = &etr_drvdata->csdev->dev;
> +	struct ctcu_byte_cntr *byte_cntr_data;
> +	struct etr_buf *sysfs_buf;
> +	atomic_t *irq_cnt;
> +	ssize_t actual;
> +	int ret;
> +
> +	/* No byte-cntr read session, let the caller use the generic read */
> +	byte_cntr_data = ctcu_get_byte_cntr(ctcu, etr_drvdata->csdev);
> +	if (!byte_cntr_data || !byte_cntr_data->irq_enabled ||
> +	    !byte_cntr_data->reading)
> +		return -EINVAL;
> +
> +	irq_cnt = &byte_cntr_data->irq_cnt;
> +
> +wait_buffer:
> +	if (!byte_cntr_data->buf_node) {
> +		ret = wait_event_interruptible_timeout(byte_cntr_data->wq,
> +				(atomic_read(irq_cnt) >= MAX_IRQ_CNT - 1) ||
> +				!byte_cntr_data->enable,
> +				BYTE_CNTR_TIMEOUT);
> +		if (ret < 0)
> +			return ret;
> +		/*
> +		 * The current etr_buf is almost full or timeout is triggered,
> +		 * so switch the buffer and mark the switched buffer as reading.
> +		 */
> +		if (byte_cntr_data->enable) {
> +			byte_cntr_data->buf_node =
> +				tmc_etr_switch_buf_node(etr_drvdata);

[Severity: High]
As reported by Sashiko on v19 [2], what prevents concurrent read() calls on
the same open /dev/tmc_etr* file descriptor from racing on
byte_cntr_data->buf_node and nd->pos?

Because /dev/tmc_etr* is a character device opened with nonseekable_open(),
FMODE_ATOMIC_POS is not set and vfs_read() does not serialize concurrent
readers sharing an open file description via file->f_pos_lock. While
tmc_read_prepare_byte_cntr() prevents multiple concurrent open() calls via
byte_cntr_data->reading, multiple threads or processes sharing a single open
file descriptor can enter tmc_read() -> tmc_etr_get_sysfs_trace() ->
tmc_byte_cntr_get_data() and tmc_etr_update_buf_node_pos() simultaneously
without holding any lock:

1. When one thread reaches the end of the current buffer (actual == 0 from
tmc_etr_read_sysfs_buf()), it sets byte_cntr_data->buf_node->reading =
false and byte_cntr_data->buf_node = NULL while a second thread that
already passed the !byte_cntr_data->buf_node check is about to dereference
byte_cntr_data->buf_node->pos and byte_cntr_data->buf_node->sysfs_buf,
causing a NULL pointer dereference.

2. If two threads both see !byte_cntr_data->buf_node and wake from
wait_event_interruptible_timeout(), both can call
tmc_etr_switch_buf_node(). Because tmc_etr_switch_buf_node() sets
curr_node->is_free = true at the same time as curr_node->reading = true,
the second call selects the first thread's curr_node as picked_node and
restarts ETR hardware DMA into that buffer while the first thread is
copying it to userspace.

3. In tmc_read(), tmc_etr_update_buf_node_pos() iterates
drvdata->etr_buf_list and updates nd->pos after copy_to_user() without
synchronization.

[ ... ]
> +	pos = byte_cntr_data->buf_node->pos;
> +	sysfs_buf = byte_cntr_data->buf_node->sysfs_buf;
> +	actual = tmc_etr_read_sysfs_buf(sysfs_buf, pos, len, bufpp);
> +	if (actual <= 0) {
> +		/* Reset buf_node upon reading is finished or failed */
> +		byte_cntr_data->buf_node->reading = false;
> +		byte_cntr_data->buf_node = NULL;
> +
> +		/*
> +		 * Nothing in the buffer, waiting for the next buffer
> +		 * to be filled.
> +		 */
> +		if (actual == 0)
> +			goto wait_buffer;
> +	}

[Severity: High]
What happens here if userspace issues a zero-length read(fd, buf, 0) on
/dev/tmc_etr* during a byte-cntr read session?

vfs_read() invokes file->f_op->read() even when count is 0, passing len ==
0 down to tmc_read() -> tmc_get_sysfs_trace() -> tmc_etr_get_sysfs_trace()
-> tmc_byte_cntr_get_data(). In tmc_etr_read_sysfs_buf(), actual is
initialized to len (0), so the check:

	if (actual <= 0)
		return actual;

immediately returns 0 regardless of how much unread trace data remains in
sysfs_buf. tmc_byte_cntr_get_data() treats actual == 0 as buffer
exhaustion, clears byte_cntr_data->buf_node->reading = false and
byte_cntr_data->buf_node = NULL (discarding all unread data in that
switched-out buffer), and jumps back to wait_buffer, where it loops
waiting for and discarding subsequent buffers until tracing stops or a
signal arrives.

[ ... ]
> +static int tmc_read_unprepare_byte_cntr(struct tmc_drvdata *etr_drvdata)
> +{
> +	struct coresight_device *ctcu = tmc_etr_get_ctcu_device(etr_drvdata);
> +	struct ctcu_byte_cntr *byte_cntr_data;
> +
> +	/*
> +	 * Do the unprepare operation only when the byte_cntr_data->reading
> +	 * is truly set
> +	 */
> +	byte_cntr_data = ctcu_get_byte_cntr(ctcu, etr_drvdata->csdev);
> +	if (!byte_cntr_data || !byte_cntr_data->irq_enabled ||
> +	    !byte_cntr_data->reading)
> +		return -EINVAL;
> +
> +	tmc_clean_etr_buf_list(etr_drvdata);
> +
> +	/* disable_irq_wake() may sleep on slow-bus irqchips, call it unlocked */
> +	disable_irq_wake(byte_cntr_data->irq);
> +
> +	/*
> +	 * The byte-cntr register is not accessed here: the trace path, which
> +	 * keeps the CTCU powered, may have been released already. It is reset
> +	 * when the path is disabled.
> +	 */
> +	scoped_guard(raw_spinlock_irqsave, &byte_cntr_data->spin_lock) {
> +		byte_cntr_data->buf_node = NULL;
> +		byte_cntr_data->reading = false;

[Severity: High]
Can tmc_read_unprepare_byte_cntr() race with a concurrent open() and close()
when the trace path is disabled during teardown?

In tmc_read_unprepare_byte_cntr(), byte_cntr_data->reading is checked
without holding byte_cntr_data->spin_lock and remains true while
tmc_clean_etr_buf_list(etr_drvdata) and the sleeping
disable_irq_wake(byte_cntr_data->irq) run.

If the ETR sink is in CS_MODE_SYSFS when tmc_clean_etr_buf_list() runs,
drvdata->etr_buf_list is emptied while drvdata->sysfs_buf is kept. If the
trace path is then disabled before byte_cntr_data->reading is cleared under
byte_cntr_data->spin_lock, coresight_get_mode(etr_drvdata->csdev) becomes
CS_MODE_DISABLED with an empty drvdata->etr_buf_list, drvdata->reading ==
false, drvdata->sysfs_buf != NULL, and byte_cntr_data->reading == true.

A second process opening /dev/tmc_etr* will call
tmc_read_prepare_byte_cntr(), which sees coresight_get_mode() !=
CS_MODE_SYSFS and returns -EINVAL before checking byte_cntr_data->reading.
tmc_read_prepare_etr() then falls through to generic ETR read preparation
and sets drvdata->reading = true. If that second process closes its file
descriptor before the first process clears byte_cntr_data->reading, the
second close() will also see byte_cntr_data->reading == true in
tmc_read_unprepare_byte_cntr(), calling disable_irq_wake() and
module_put(THIS_MODULE) a second time and returning 0 without clearing
drvdata->reading or freeing drvdata->sysfs_buf.

[ ... ]
> +/* Start the byte-cntr function when the path is enabled. */
> +void ctcu_byte_cntr_start(struct coresight_device *csdev, struct coresight_path *path)
> +{
> +	struct coresight_device *sink = coresight_get_sink(path);
> +	struct tmc_drvdata *etr_drvdata;
> +	struct ctcu_byte_cntr *byte_cntr_data;
> +
> +	byte_cntr_data = ctcu_get_byte_cntr(csdev, sink);
> +	if (!byte_cntr_data)
> +		return;
> +
> +	etr_drvdata = dev_get_drvdata(sink->dev.parent);
> +	guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
> +	/*
> +	 * Don't start byte-cntr function when irq_enabled is not set, or when
> +	 * the CTCU device is being removed.
> +	 */
> +	if (!byte_cntr_data->irq_enabled || byte_cntr_data->removing)
> +		return;
> +
> +	/*
> +	 * A new trace starts with an empty etr_buf. Don't reset the count if
> +	 * the byte-cntr is already started by another path of the same sink.
> +	 */
> +	if (!byte_cntr_data->enable)
> +		atomic_set(&byte_cntr_data->irq_cnt, 0);
> +
> +	/*
> +	 * Configure the byte-cntr register to enable IRQ while the path is
> +	 * enabled, the CTCU is powered then. The configured size is 5% of the
> +	 * buffer_size.
> +	 */
> +	ctcu_cfg_byte_cntr_reg(byte_cntr_data->ctcu_drvdata,
> +			       etr_drvdata->size / MAX_IRQ_CNT,
> +			       byte_cntr_data->irq_ctrl_offset);
> +	byte_cntr_data->enable = true;
> +}

[Severity: Medium]
Can ctcu_byte_cntr_start() corrupt an active read session or an already
running trace session here?

1. In coresight_enable_path_from_to(), helpers on the sink (ctcu_enable()
-> ctcu_byte_cntr_start()) are enabled before coresight_enable_sink() calls
tmc_enable_etr_sink_sysfs(). When a trace stops while /dev/tmc_etr* is still
open in byte-cntr mode, ctcu_byte_cntr_stop() sets byte_cntr_data->enable =
false while byte_cntr_data->reading remains true until the reader drains
the remaining data and closes the file. If another sysfs trace enable is
attempted on the same sink before the file is closed,
ctcu_byte_cntr_start() does not check byte_cntr_data->reading, so it resets
byte_cntr_data->irq_cnt to 0, reprograms IRQCTRL, and sets
byte_cntr_data->enable = true before tmc_enable_etr_sink_sysfs() rejects
the enable with -EBUSY. During that window, a concurrent read() in
tmc_byte_cntr_get_data() sees byte_cntr_data->enable == true and blocks in
wait_event_interruptible_timeout() instead of returning 0 to drain the
final buffer.

2. When a second source path is enabled on an already-running ETR sink
where byte_cntr_data->enable is already true, ctcu_byte_cntr_start() avoids
resetting irq_cnt, but still unconditionally reprograms the IRQCTRL
register using etr_drvdata->size / MAX_IRQ_CNT. Because buffer_size_store()
allows changing etr_drvdata->size while the ETR is running while
tmc_enable_etr_sink_sysfs() continues using the existing sysfs_buf of the
original size, reprogramming IRQCTRL when byte_cntr_data->enable is already
true can overwrite the threshold with a mismatched value (or 0 if
etr_drvdata->size < MAX_IRQ_CNT * 8, disabling interrupts while
byte_cntr_data->enable is true).

[ ... ]
> +void ctcu_byte_cntr_init(struct device *dev, struct ctcu_drvdata *drvdata, int etr_num)
> +{
[ ... ]
> +	scoped_guard(mutex, &byte_cntr_ops_lock) {
> +		tmc_etr_set_byte_cntr_sysfs_ops(&byte_cntr_sysfs_ops);
> +		byte_cntr_ops_users++;
> +	}
> +
> +	/* Drop the ops user when the CTCU device is removed or probe fails */
> +	if (devm_add_action_or_reset(dev, ctcu_byte_cntr_put_ops, NULL))
> +		dev_err(dev, "Failed to add the byte-cntr devm action\n");
> +}

[Severity: Medium]
As reported by Sashiko on v20 [3], if devm_add_action_or_reset() fails in
ctcu_byte_cntr_init(), ctcu_byte_cntr_put_ops() is immediately called to
decrement byte_cntr_ops_users and reset byte_cntr_sysfs_ops to NULL, but
ctcu_byte_cntr_init() returns void without propagating the error to
ctcu_probe() or clearing byte_cntr_data[i].irq.

Doesn't this allow ctcu_probe() to succeed with the IRQs still registered
and the irq_enabled sysfs attributes exposed while byte_cntr_ops_users no
longer accounts for this device?

> diff --git a/drivers/hwtracing/coresight/coresight-ctcu-core.c b/drivers/hwtracing/coresight/coresight-ctcu-core.c
> index 739105acb0a13..cce4e4d76da0d 100644
> --- a/drivers/hwtracing/coresight/coresight-ctcu-core.c
> +++ b/drivers/hwtracing/coresight/coresight-ctcu-core.c
[ ... ]
> +static ssize_t irq_enabled_store(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf,
> +				 size_t size)
> +{
> +	struct ctcu_byte_cntr_irq_attribute *irq_attr =
> +		container_of(attr, struct ctcu_byte_cntr_irq_attribute, attr);
> +	struct ctcu_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	u8 port = irq_attr->port;
> +	unsigned long val;
> +
> +	if (kstrtoul(buf, 0, &val))
> +		return -EINVAL;
> +
> +	guard(raw_spinlock_irqsave)(&drvdata->byte_cntr_data[port].spin_lock);
> +	if (drvdata->byte_cntr_data[port].reading)
> +		return -EBUSY;
> +	else if (drvdata->byte_cntr_data[port].irq_ctrl_offset)
> +		drvdata->byte_cntr_data[port].irq_enabled = !!val;
> +
> +	return size;
> +}

[Severity: High]
Can modifying irq_enabled while a trace path or generic ETR read session is
active desynchronize byte_cntr_data and ETR state?

1. irq_enabled_store() only checks drvdata->byte_cntr_data[port].reading,
not drvdata->byte_cntr_data[port].enable. If a sysfs trace path is enabled
with irq_enabled == 1, ctcu_byte_cntr_start() programs the BYTECNTRVAL
register and sets byte_cntr_data->enable = true while
byte_cntr_data->reading remains false until /dev/tmc_etr* is opened. If
userspace writes 0 to irq_enabled before opening /dev/tmc_etr*,
tmc_read_prepare_byte_cntr() returns -EINVAL and tmc_read_prepare_etr()
falls back to generic ETR read (setting drvdata->reading = true while
byte_cntr_data->enable remains true and byte_cntr_data->reading is false).
If userspace then writes 1 to irq_enabled and a second process opens
/dev/tmc_etr*, tmc_read_prepare_byte_cntr() sets byte_cntr_data->reading =
true before tmc_create_etr_buf_list() fails with -EBUSY on
drvdata->reading. A concurrent close() of the first session during this
window will enter tmc_read_unprepare_byte_cntr() instead of generic
unprepare, calling disable_irq_wake() and module_put(THIS_MODULE) and
leaving drvdata->reading permanently stuck at true.

2. Similarly, if a generic sysfs read session is open (drvdata->reading ==
true) while byte_cntr_data->enable is true and the trace path is disabled,
tmc_disable_etr_sink() returns -EBUSY and leaves the sink mode as
CS_MODE_SYSFS. When ctcu_disable() -> ctcu_byte_cntr_stop() then runs, it
sees coresight_get_mode(sink) == CS_MODE_SYSFS and returns early without
clearing the hardware IRQCTRL register or setting byte_cntr_data->enable =
false, even though the CTCU helper is being disabled.

[ ... ]
> @@ -244,10 +353,52 @@ static int ctcu_platform_probe(struct platform_device *pdev)
>  static void ctcu_platform_remove(struct platform_device *pdev)
>  {
>  	struct ctcu_drvdata *drvdata = platform_get_drvdata(pdev);
> +	struct ctcu_byte_cntr *byte_cntr_data;
> +	unsigned long flags;
> +	int i;
>  
>  	if (WARN_ON(!drvdata))
>  		return;
>  
> +	/*
> +	 * Signal all active byte-cntr readers to exit, then wait for them to
> +	 * finish before resetting the ops pointer and freeing driver data.
> +	 * Without this, a reader blocked in wait_event_interruptible_timeout()
> +	 * would access the freed ctcu_drvdata wait-queue head (use-after-free).
> +	 *
> +	 * The wait ends only when the userspace closes the tmc_etr file, so an
> +	 * unbind with the file left open blocks until then. Unloading the module
> +	 * is rejected with -EBUSY while a read session is open, so only a manual
> +	 * unbind can block here.
> +	 */
> +	for (i = 0; i < ETR_MAX_NUM; i++) {
> +		byte_cntr_data = &drvdata->byte_cntr_data[i];
> +		/* The port was not initialized by ctcu_byte_cntr_init() */
> +		if (!byte_cntr_data->irq)
> +			continue;
> +
> +		raw_spin_lock_irqsave(&byte_cntr_data->spin_lock, flags);
> +		/*
> +		 * Set enable=false to signal teardown to racing readers, and
> +		 * removing=true to keep a new path from enabling byte-cntr again.
> +		 */
> +		byte_cntr_data->enable = false;
> +		byte_cntr_data->removing = true;
> +		raw_spin_unlock_irqrestore(&byte_cntr_data->spin_lock, flags);
> +
> +		/* Wake up the blocked reader, then wait for the session to end */
> +		wake_up_all(&byte_cntr_data->wq);
> +		wait_for_completion(&byte_cntr_data->session_done);

[Severity: High]
When ctcu_platform_remove() clears byte_cntr_data->enable and wakes a
blocked reader while the ETR sink is still actively tracing in
CS_MODE_SYSFS, won't this cause an unsynced read of the live DMA buffer and
leave the hardware IRQCTRL register enabled?

1. When woken in tmc_byte_cntr_get_data(), the reader sees
!byte_cntr_data->enable and returns 0. In tmc_etr_get_sysfs_trace(), a
return value of 0 is treated as meaning the sink has stopped and synced its
final buffer, so it falls through to
tmc_etr_read_sysfs_buf(drvdata->sysfs_buf, pos, len, bufpp). If at least
one buffer switch has already occurred via tmc_etr_switch_buf_node(), the
currently active drvdata->sysfs_buf still holds a non-zero stale
sysfs_buf->len from its previous fill cycle, so tmc_etr_read_sysfs_buf()
reads directly from the live drvdata->sysfs_buf while the ETR hardware is
actively DMA-writing to it (and without drvdata->reading synchronization
against a concurrent tmc_disable_etr_sink() -> tmc_etr_sync_sysfs_buf()).

2. Setting byte_cntr_data->enable = false here without clearing the
hardware BYTECNTRVAL register via ctcu_cfg_byte_cntr_reg() also causes
ctcu_byte_cntr_stop() to skip clearing the register when the path is later
disabled because byte_cntr_data->enable is already false.

> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
> index 4b40b692be4d8..6ad09995ba87c 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
> @@ -293,7 +293,8 @@ static ssize_t tmc_read(struct file *file, char __user *data, size_t len,
>  		return -EFAULT;
>  	}
>  
> -	*ppos += actual;
> +	if (!tmc_etr_update_buf_node_pos(drvdata, actual))
> +		*ppos += actual;
>  	dev_dbg(&drvdata->csdev->dev, "%zu bytes copied\n", actual);
>  
>  	return actual;

[Severity: Medium]
As reported by Sashiko on v19 [4], in tmc_read(), negative error codes
returned by tmc_get_sysfs_trace() -> tmc_etr_get_sysfs_trace() ->
tmc_byte_cntr_get_data() (such as -ERESTARTSYS when
wait_event_interruptible_timeout() is interrupted by a signal, or -ENOMEM
when tmc_etr_switch_buf_node() fails) are caught by:

	actual = tmc_get_sysfs_trace(drvdata, *ppos, len, &bufp);
	if (actual <= 0)
		return 0;

Doesn't this convert -ERESTARTSYS and -ENOMEM into 0 (EOF) and terminate
the trace read session prematurely instead of propagating the error to
userspace?

> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index bf4c1d8f7b2e0..20d6bc60a446e 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
[ ... ]
> @@ -2170,15 +2194,54 @@ int tmc_create_etr_buf_list(struct tmc_drvdata *drvdata, int num_nodes)
[ ... ]
>  int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
>  {
>  	int ret = 0;
>  	unsigned long flags;
> +	const struct tmc_sysfs_ops *byte_cntr_ops;
>  
>  	/* config types are set a boot time and never change */
>  	if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETR))
>  		return -EINVAL;
>  
> +	byte_cntr_ops = READ_ONCE(byte_cntr_sysfs_ops);
> +	if (byte_cntr_ops) {
> +		ret = byte_cntr_ops->read_prepare(drvdata);
> +		if (!ret || ret == -EBUSY)
> +			return ret;
> +
> +		ret = 0;
> +	}

[Severity: High]
As reported by Sashiko on v19 [5], why does tmc_read_prepare_etr() ignore
errors other than -EBUSY (such as -ENOMEM or -ENODEV) from
byte_cntr_ops->read_prepare(drvdata) and fall through to the generic ETR
read path?

When byte-cntr is active in CS_MODE_SYSFS and tmc_read_prepare_byte_cntr()
fails (for example with -ENOMEM in tmc_create_etr_buf_list(), or -EINVAL
when drvdata->etr_mode == ETR_MODE_RESRV), falling back to the generic ETR
read path halts the running ETR hardware via __tmc_etr_disable_hw() and
sets drvdata->reading = true while byte_cntr_data->reading remains false:

1. If a second process then calls open() on /dev/tmc_etr* while the first
process holds that fallback generic session open, tmc_read_prepare_etr()
calls tmc_read_prepare_byte_cntr() before checking drvdata->reading. The
second process sees byte_cntr_data->reading == false, calls
try_module_get(THIS_MODULE), and sets byte_cntr_data->reading = true before
tmc_create_etr_buf_list() checks drvdata->reading under drvdata->spinlock
and fails with -EBUSY. If the first process calls close() during this
window, tmc_read_unprepare_etr() calls tmc_read_unprepare_byte_cntr(),
which sees byte_cntr_data->reading == true and tears down the second
process's half-initialized state: it calls disable_irq_wake() before
enable_irq_wake() was called, clears byte_cntr_data->reading, calls
module_put(THIS_MODULE) (which the second process's err_reading path then
calls a second time, underflowing the module refcount), and returns 0 so
tmc_read_unprepare_etr() skips clearing drvdata->reading, permanently
wedging the ETR device.

2. Furthermore, in tmc_create_etr_buf_list(), if the sink is disabled while
drvdata->spinlock is dropped to allocate the second buffer and that
allocation fails, the error path calls tmc_clean_etr_buf_list(drvdata),
which frees the pre-existing drvdata->sysfs_buf and loses the captured
trace data.

[1] https://lore.kernel.org/r/20260625110924.BA8EA1F000E9@smtp.kernel.org
[2] https://lore.kernel.org/r/20260625110924.BA8EA1F000E9@smtp.kernel.org
[3] https://lore.kernel.org/r/20261030-enable-byte-cntr-for-ctcu-v20-6-bd5c3ac9f916@oss.qualcomm.com
[4] https://lore.kernel.org/r/20260625110924.BA8EA1F000E9@smtp.kernel.org
[5] https://lore.kernel.org/r/20260625110924.BA8EA1F000E9@smtp.kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-enable-byte-cntr-for-ctcu-v21-0-231e069b7853@oss.qualcomm.com?part=7