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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [Critical] Global function pointer accessed locklessly leads to Use-After-Free on driver unload
- [Critical] Global sysfs ops pointer triggers NULL pointer dereference on ETR without CTCU
- [High] Driver remove/unbind blocks indefinitely waiting for userspace file closure
- [High] ctcu_byte_cntr_stop() skips setting enable=false for SYSFS mode, causing infinite read hangs
- [High] TOCTOU race re-enables ETR hardware after framework disables it
- [High] Missing wake_up() in error path deadlocks driver remove()
- [High] Unsynchronized checks of irq_enabled lead to resource leaks and driver unbind hangs
- [High] Data race on concurrent read() calls causes NULL pointer dereference
- [Medium] Spurious Interrupt Handling due to missing hardware status check in shared IRQ
- [Medium] Error codes (-ERESTARTSYS) from byte_cntr_ops are swallowed and convert to EOF
- [Medium] Masked error code breaks sysfs reading fallback on memory failure
--

commit 128ffef0065b7b8a8c73e5e3f9b0ce8bebea0593
Author: Jie Gan <jie.gan@oss.qualcomm.com>

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

This patch introduces the byte-cntr function provided by the CTCU device to
transfer data from the ETR buffer to userspace. It sets up an interrupt
handler to track data size thresholds and switches buffers when full or on
timeout. It also integrates with the TMC ETR framework via sysfs ops.

> 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..5ab97a71f02f6
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
[ ... ]
> +static irqreturn_t byte_cntr_handler(int irq, void *data)
> +{
> +	struct ctcu_byte_cntr *byte_cntr_data = data;
> +
> +	atomic_inc(&byte_cntr_data->irq_cnt);
> +	wake_up(&byte_cntr_data->wq);
> +
> +	return IRQ_HANDLED;
> +}

[Severity: Medium]
Is this handler missing a hardware status check?

Since the IRQ is registered with IRQF_SHARED, another device sharing the line
could fire an interrupt. By unconditionally returning IRQ_HANDLED without
verifying the interrupt source, the kernel's spurious interrupt detection
might be suppressed, and the CTCU buffer switch logic could be triggered
prematurely.

[ ... ]
> +static struct ctcu_byte_cntr *ctcu_get_byte_cntr(struct coresight_device *ctcu,
> +						 struct coresight_device *etr)
> +{
> +	struct ctcu_drvdata *drvdata = dev_get_drvdata(ctcu->dev.parent);

[Severity: Critical]
Could this dereference a NULL pointer if an ETR without a connected CTCU
is used?

If the system has multiple ETRs and one lacks a CTCU, the globally registered
byte_cntr_sysfs_ops might pass a NULL ctcu pointer here. This would cause a
panic when ctcu->dev.parent is accessed.

[ ... ]
> +static bool ctcu_byte_cntr_switch_buffer(struct tmc_drvdata *etr_drvdata,
> +					 struct ctcu_byte_cntr *byte_cntr_data)
> +{
[ ... ]
> +	raw_spin_unlock_irqrestore(&etr_drvdata->spinlock, flags);
> +
> +	/* Restart the ETR once a free buffer is available */
> +	if (found_free_buf &&
> +	    coresight_get_mode(etr_drvdata->csdev) != CS_MODE_DISABLED)
> +		tmc_etr_enable_disable_hw(etr_drvdata, true);
> +
> +	return found_free_buf;
> +}

[Severity: High]
Does this lockless check introduce a race condition?

If a concurrent operation disables the trace session and drops the lock before
setting the mode to CS_MODE_DISABLED, this code might incorrectly re-enable
the hardware after the framework has explicitly disabled it.

[ ... ]
> +static ssize_t tmc_byte_cntr_get_data(struct tmc_drvdata *etr_drvdata, loff_t pos,
> +				      size_t len, char **bufpp)
> +{
[ ... ]
> +	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;

[Severity: High]
Can concurrent read operations cause a NULL pointer dereference here?

If two threads read from the same ETR file descriptor, they could concurrently
access and modify byte_cntr_data->buf_node without serialization, causing one
thread to clear the pointer while the other is still using it.

[ ... ]
> +static int tmc_read_prepare_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;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	/* byte-cntr is operating with SYSFS mode being enabled only */
> +	if (coresight_get_mode(etr_drvdata->csdev) != CS_MODE_SYSFS)
> +		return -EINVAL;
> +
> +	byte_cntr_data = ctcu_get_byte_cntr(ctcu, etr_drvdata->csdev);
> +	if (!byte_cntr_data || !byte_cntr_data->irq_enabled)
> +		return -EINVAL;
> +
> +	raw_spin_lock_irqsave(&byte_cntr_data->spin_lock, flags);

[Severity: High]
Does checking irq_enabled outside the spinlock create a race?

A concurrent sysfs write could set irq_enabled to 0 immediately before the
lock is acquired. If reading is then set to true, read_unprepare() will later
abort early upon seeing irq_enabled is 0. This would leak the buffer list and
leave reading permanently true, causing the driver unbind to hang forever.

[ ... ]
> +	byte_cntr_data->reading = true;
> +	raw_spin_unlock_irqrestore(&byte_cntr_data->spin_lock, flags);
> +	/* Setup an available etr_buf_list for byte-cntr */
> +	ret = tmc_create_etr_buf_list(etr_drvdata, 2);
> +	if (ret) {
> +		byte_cntr_data->reading = false;
> +		return ret;
> +	}

[Severity: High]
Is this error path missing a wake_up() call?

If this allocation fails, the reading flag is cleared, but concurrent readers
waiting in the driver's remove function won't be notified, potentially
deadlocking the unbind process.

[ ... ]
> +void ctcu_byte_cntr_stop(struct coresight_device *csdev, struct coresight_path *path)
> +{
> +	struct coresight_device *sink = coresight_get_sink(path);
> +	struct ctcu_byte_cntr *byte_cntr_data;
> +
> +	if (coresight_get_mode(sink) == CS_MODE_SYSFS)
> +		return;

[Severity: High]
Should the enable flag be cleared for SYSFS mode here?

By returning early, enable remains true. Readers waiting in
tmc_byte_cntr_get_data() might never receive the EOF condition when the
trace is stopped, causing them to loop indefinitely.

> diff --git a/drivers/hwtracing/coresight/coresight-ctcu-core.c b/drivers/hwtracing/coresight/coresight-ctcu-core.c
> index e8720026c9e3f..2da1a6f3d29f4 100644
> --- a/drivers/hwtracing/coresight/coresight-ctcu-core.c
> +++ b/drivers/hwtracing/coresight/coresight-ctcu-core.c
[ ... ]
> +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).
> +	 */
> +	for (i = 0; i < ETR_MAX_NUM; i++) {
> +		byte_cntr_data = &drvdata->byte_cntr_data[i];
> +		raw_spin_lock_irqsave(&byte_cntr_data->spin_lock, flags);
> +		/* Set enable=false for all ports to signal teardown to racing readers */
> +		byte_cntr_data->enable = false;
> +		if (!byte_cntr_data->reading) {
> +			raw_spin_unlock_irqrestore(&byte_cntr_data->spin_lock, flags);
> +			continue;
> +		}
> +		raw_spin_unlock_irqrestore(&byte_cntr_data->spin_lock, flags);
> +		wake_up_all(&byte_cntr_data->wq);
> +		wait_event(byte_cntr_data->wq, !byte_cntr_data->reading);
> +	}

[Severity: High]
Could this wait indefinitely during driver remove?

The reading flag is only cleared when the userspace file is closed. A
userspace process leaving the file descriptor open could permanently block
module unload or device unbind.

> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 2b26ce6455a7b..e78f8891f11e0 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
[ ... ]
> +ssize_t tmc_etr_get_sysfs_trace(struct tmc_drvdata *drvdata,
> +				loff_t pos, size_t len, char **bufpp)
> +{
> +	ssize_t ret;
> +	const struct tmc_sysfs_ops *byte_cntr_ops = READ_ONCE(byte_cntr_sysfs_ops);
> +
> +	if (byte_cntr_ops) {
> +		ret = byte_cntr_ops->get_trace_data(drvdata, pos, len, bufpp);
> +		/* Return the filled buffer */
> +		if (ret > 0 || ret == -ENOMEM)
> +			return ret;
> +	}
> +
> +	return tmc_etr_read_sysfs_buf(drvdata->sysfs_buf, pos, len, bufpp);
> +}

[Severity: Medium]
Does this silently swallow -ERESTARTSYS?

If a signal interrupts the wait inside tmc_byte_cntr_get_data(), it returns
-ERESTARTSYS, which falls through here to a normal sysfs buffer read. This
might convert the signal into an EOF or return unsynced data instead of
letting userspace handle the signal cleanly.

[ ... ]
> +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);

[Severity: Critical]
Is it safe to access this global function pointer locklessly?

If the trace file is opened and the CTCU module is concurrently unloaded, the
memory backing byte_cntr_ops could be freed before read_prepare() executes,
leading to a use-after-free regression.

> +		if (!ret || ret == -EBUSY)
> +			return ret;
> +
> +		ret = 0;
> +	}

[Severity: Medium]
Why are errors like -ENOMEM from read_prepare() masked here?

If the memory allocation fails inside tmc_read_prepare_byte_cntr(), hiding the
error breaks the fallback sysfs reading path, leading to delays and hanging
operations instead of propagating the failure properly.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625-enable-byte-cntr-for-ctcu-v19-0-8fbbf22e8381@oss.qualcomm.com?part=6