Re: [PATCH v19 2/7] coresight: tmc: add create/clean functions for etr_buf_list

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Lockless read of `drvdata->sysfs_buf` in `tmc_create_etr_buf_list` leads to a Time-of-Check to Time-of-Use race condition, resulting in a Double-Free or Use-After-Free.
- [Medium] The error path in `tmc_create_etr_buf_list` destructively frees the pre-existing `drvdata->sysfs_buf` if the device becomes inactive, leading to the irreversible loss of previously captured trace data.
- [Medium] `tmc_clean_etr_buf_list` leaks `drvdata->sysfs_buf` if called when the device is active in a non-SYSFS mode (e.g., `CS_MODE_PERF`).
--

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

coresight: tmc: add create/clean functions for etr_buf_list

This commit introduces functions for creating and cleaning a list of
etr_buf_node elements to manage etr_buf entries. It is intended to support
byte-counter functionality which requires multiple trace data buffers.

> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 361a433e6f0c5..9b3ef73e9cf27 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c

[ ... ]

> @@ -1918,6 +1918,132 @@ const struct coresight_ops tmc_etr_cs_ops = {
>  	.panic_ops	= &tmc_etr_sync_ops,
>  };
>  
> +/**
> + * tmc_clean_etr_buf_list - clean the etr_buf_list.
> + * @drvdata:	driver data of the TMC device.
> + *
> + * Remove all nodes from @drvdata->etr_buf_list and free their buffers.
> + * If a node holds the live sysfs_buf and the device is active, the node is
> + * removed but the buffer is not freed; ownership stays with drvdata->sysfs_buf.
> + *
> + * Locking: callers must guarantee exclusive access to @drvdata->etr_buf_list
> + * and must not hold @drvdata->spinlock. The spinlock is taken internally only
> + * to serialise the @drvdata->sysfs_buf accesses against the ETR sink
> + * enable/disable paths. Must be called from process context: buffers are freed
> + * with the lock released.
> + */
> +void tmc_clean_etr_buf_list(struct tmc_drvdata *drvdata)
> +{
> +	struct etr_buf_node *nd, *next;
> +	unsigned long flags;
> +
> +	list_for_each_entry_safe(nd, next, &drvdata->etr_buf_list, link) {
> +		raw_spin_lock_irqsave(&drvdata->spinlock, flags);
> +		if (nd->sysfs_buf == drvdata->sysfs_buf) {
> +			if (coresight_get_mode(drvdata->csdev) != CS_MODE_DISABLED)
> +				/*
> +				 * The device is still active. Keep the live
> +				 * buffer owned by drvdata->sysfs_buf and only
> +				 * drop the list's reference to it.
> +				 */
> +				nd->sysfs_buf = NULL;

[Severity: Medium]
Will this leak drvdata->sysfs_buf if the device mode is active in a non-SYSFS
mode (such as CS_MODE_PERF)?

If the mode changes to CS_MODE_PERF concurrently during the
tmc_create_etr_buf_list() error path, this condition evaluates to true and
abandons the pointer by setting nd->sysfs_buf to NULL.

Since PERF mode tracing exclusively manages drvdata->perf_buf, does its
teardown path ever free drvdata->sysfs_buf?

> +			else
> +				/* Free the buffer below through nd->sysfs_buf */
> +				drvdata->sysfs_buf = NULL;

[Severity: Medium]
If tmc_create_etr_buf_list() encounters an allocation failure and calls
tmc_clean_etr_buf_list() to roll back, and the device happens to be inactive,
will this unconditionally clear and free the pre-existing drvdata->sysfs_buf?

Should an initialization error path only clean up the resources it allocated,
rather than destroying a preserved external resource and losing previously
captured trace data?

> +		}
> +		raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +
> +		/* Free the buffer (NULL is ignored) and the node out of the lock */
> +		tmc_etr_free_sysfs_buf(nd->sysfs_buf);
> +		list_del(&nd->link);
> +		kfree(nd);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(tmc_clean_etr_buf_list);
> +
> +/**
> + * tmc_create_etr_buf_list - create a list to manage the etr_buf_node.
> + * @drvdata:	driver data of the TMC device.
> + * @num_nodes:	number of nodes want to create with the list.
> + *
> + * Locking: callers must guarantee exclusive access to @drvdata->etr_buf_list
> + * and must not hold @drvdata->spinlock. The spinlock is taken internally only
> + * to serialise the @drvdata->sysfs_buf accesses against the ETR sink
> + * enable/disable paths. Must be called from process context: buffers and nodes
> + * are allocated with the lock released.
> + *
> + * Return 0 upon success and return the error number if fail.
> + */
> +int tmc_create_etr_buf_list(struct tmc_drvdata *drvdata, int num_nodes)
> +{
> +	struct etr_buf_node *new_node;
> +	struct etr_buf *sysfs_buf;
> +	unsigned long flags;
> +	int i = 0, ret = 0;
> +
> +	/* We don't need a list if there is only one node */
> +	if (num_nodes < 2)
> +		return -EINVAL;
> +
> +	/*
> +	 * We expect that sysfs_buf in drvdata has already been allocated.
> +	 * Wrap the live sysfs_buf into the first node so the captured trace
> +	 * data is preserved. The list is owned by the caller, so no lock is
> +	 * needed to read sysfs_buf or to add the node here.
> +	 */
> +	if (drvdata->sysfs_buf) {
> +		new_node = kzalloc_obj(*new_node, GFP_KERNEL);
> +		if (!new_node)
> +			return -ENOMEM;
> +
> +		new_node->sysfs_buf = drvdata->sysfs_buf;

[Severity: High]
Does this lockless read of drvdata->sysfs_buf introduce a race condition?

If a concurrent hardware disable/enable path (like tmc_etr_get_sysfs_buffer())
clears and frees drvdata->sysfs_buf immediately after this lockless check,
wouldn't new_node->sysfs_buf be left as a dangling pointer, leading to a
use-after-free or double free?

> +		new_node->is_free = false;
> +		list_add(&new_node->link, &drvdata->etr_buf_list);
> +		i++;
> +	}

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