Re: [PATCH v21 2/8] coresight: tmc: add create/clean functions for etr_buf_list

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] coresight: tmc-etr: sink disable race in tmc_create_etr_buf_list()
- [Medium] coresight: tmc-etr: incomplete ETR_MODE_RESRV check in tmc_create_etr_buf_list()
--

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

coresight: tmc: add create/clean functions for etr_buf_list

This commit introduces tmc_create_etr_buf_list() and
tmc_clean_etr_buf_list() to manage a list of etr_buf_node buffers on
TMC ETR devices for byte-cntr trace collection.

> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 361a433e6f0c5..eee98a9269df4 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
[ ... ]
> @@ -1918,6 +1920,167 @@ 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 sink is enabled in sysfs mode, the
> + * node is removed but the buffer is not freed; ownership stays with
> + * drvdata->sysfs_buf.
> + *
> + * Locking: must not be called with @drvdata->spinlock held. The spinlock is
> + * taken internally to serialise the @drvdata->sysfs_buf accesses and the
> + * removal of the nodes against the ETR sink enable/disable paths and the
> + * generic sysfs read. There must be only one owner of the list at a time.
> + * 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_SYSFS)
> +				/*
> +				 * The sink is still enabled in sysfs mode. Keep
> +				 * the live buffer owned by drvdata->sysfs_buf
> +				 * and only drop the list's reference to it.
> +				 */
> +				nd->sysfs_buf = NULL;
> +			else
> +				/* Free the buffer below through nd->sysfs_buf */
> +				drvdata->sysfs_buf = NULL;
> +		}
> +		list_del(&nd->link);
> +		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);
> +		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: must not be called with @drvdata->spinlock held. The spinlock is
> + * taken internally to serialise the @drvdata->sysfs_buf accesses and every
> + * update of @drvdata->etr_buf_list against the ETR sink enable/disable paths
> + * and the generic sysfs read. There must be only one owner of the list at a
> + * time. Must be called from process context: buffers and nodes are allocated
> + * with the lock released.
> + *
> + * Return: 0 upon success, else the error number: -EBUSY if a generic sysfs
> + * read is in progress, -EINVAL if the sink is not enabled in sysfs mode or
> + * the ETR is in the reserved buffer mode, or the error of the allocation.
> + */
> +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;
> +
> +	/*
> +	 * All the etr_buf of the reserved mode are mapped to the same reserved
> +	 * region, so they can't be used as separate buffers of the list.
> +	 */
> +	if (drvdata->etr_mode == ETR_MODE_RESRV)
> +		return -EINVAL;

[Severity: Medium]
As reported by Sashiko on v20 [1], can ETR_MODE_RESRV buffers still end up
in drvdata->etr_buf_list despite this check?

In tmc_create_etr_buf_list(), drvdata->etr_mode == ETR_MODE_RESRV is
only checked once without holding drvdata->spinlock, and
drvdata->sysfs_buf->mode is not checked when wrapping the existing
drvdata->sysfs_buf into the list.

Because buf_mode_preferred_store() allows userspace to change
drvdata->etr_mode at any time via sysfs:
1. If the ETR sink was enabled while drvdata->etr_mode was
   ETR_MODE_RESRV (or reused a prior ETR_MODE_RESRV buffer in
   tmc_etr_get_sysfs_buffer()), drvdata->sysfs_buf->mode is
   ETR_MODE_RESRV. If buf_mode_preferred is then switched to another
   mode before tmc_create_etr_buf_list() runs, this check passes and
   the ETR_MODE_RESRV drvdata->sysfs_buf is wrapped into
   drvdata->etr_buf_list.
2. If buf_mode_preferred_store() -> buf_mode_set_resrv() changes
   drvdata->etr_mode to ETR_MODE_RESRV after this check while
   tmc_create_etr_buf_list() is in the unlocked allocation loop,
   tmc_alloc_etr_buf() re-reads drvdata->etr_mode and calls
   tmc_etr_alloc_resrv_buf(), mapping multiple nodes in
   drvdata->etr_buf_list to the same static drvdata->resrv_buf region.

Should tmc_create_etr_buf_list() also verify that
drvdata->sysfs_buf->mode != ETR_MODE_RESRV and that newly allocated
buffers do not have sysfs_buf->mode == ETR_MODE_RESRV?

> +
> +	/*
> +	 * 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. Take the sysfs_buf and add the node under the
> +	 * lock, so that the ETR sink enable path sees a non-empty list and
> +	 * doesn't replace the buffer.
> +	 */
> +	new_node = kzalloc_obj(*new_node, GFP_KERNEL);
> +	if (!new_node)
> +		return -ENOMEM;
> +
> +	raw_spin_lock_irqsave(&drvdata->spinlock, flags);
> +	/* The sysfs_buf is in use by a sysfs read session */
> +	if (drvdata->reading) {
> +		raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +		kfree(new_node);
> +		return -EBUSY;
> +	}
> +
> +	/*
> +	 * The sink enable path reuses or frees the sysfs_buf of a sink which
> +	 * isn't enabled, so only wrap it while the sink is enabled in sysfs mode.
> +	 */
> +	if (coresight_get_mode(drvdata->csdev) != CS_MODE_SYSFS) {
> +		raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +		kfree(new_node);
> +		return -EINVAL;
> +	}
> +
> +	if (drvdata->sysfs_buf) {
> +		new_node->sysfs_buf = drvdata->sysfs_buf;
> +		new_node->is_free = false;
> +		list_add(&new_node->link, &drvdata->etr_buf_list);
> +		new_node = NULL;
> +		i++;
> +	}
> +	raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +	/* Free the node if there is no live sysfs_buf to wrap */
> +	kfree(new_node);
> +
> +	while (i < num_nodes) {
> +		new_node = kzalloc_obj(*new_node, GFP_KERNEL);
> +		if (!new_node) {
> +			ret = -ENOMEM;
> +			break;
> +		}
> +
> +		/* Allocate the buffer with the lock released */
> +		sysfs_buf = tmc_alloc_etr_buf(drvdata, drvdata->size, 0, cpu_to_node(0), NULL);
> +		if (IS_ERR(sysfs_buf)) {
> +			kfree(new_node);
> +			ret = PTR_ERR(sysfs_buf);
> +			break;
> +		}
> +
> +		new_node->sysfs_buf = sysfs_buf;
> +		/*
> +		 * The drvdata->sysfs_buf write and the list update need the
> +		 * spinlock, to serialise against the ETR sink enable/disable
> +		 * paths.
> +		 */
> +		raw_spin_lock_irqsave(&drvdata->spinlock, flags);
> +		/* We don't have an available sysfs_buf in drvdata, set one up */
> +		if (!drvdata->sysfs_buf) {
> +			drvdata->sysfs_buf = sysfs_buf;
> +			new_node->is_free = false;
> +		} else {
> +			new_node->is_free = true;
> +		}
> +		list_add_tail(&new_node->link, &drvdata->etr_buf_list);
> +		raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +		i++;
> +	}
> +
> +	/* Clean the list if there is an error */
> +	if (ret)
> +		tmc_clean_etr_buf_list(drvdata);
> +
> +	return ret;
> +}

[Severity: High]
As reported by Sashiko on v20 [2], what happens if the ETR sink is
disabled via sysfs while tmc_create_etr_buf_list() has dropped
drvdata->spinlock during the while (i < num_nodes) allocation loop?

Because drvdata->reading is not set and tmc_disable_etr_sink() only
checks drvdata->reading (not list_empty(&drvdata->etr_buf_list)), a
concurrent coresight_disable_sysfs() -> tmc_disable_etr_sink() can flush
and sync the hardware trace data into drvdata->sysfs_buf and set
drvdata->csdev mode to CS_MODE_DISABLED (after which
tmc_enable_etr_sink_perf() could also transition it to CS_MODE_PERF)
while drvdata->spinlock is released for kzalloc_obj() and
tmc_alloc_etr_buf():

1. If a subsequent allocation in the loop fails (ret != 0),
   tmc_create_etr_buf_list() calls tmc_clean_etr_buf_list(drvdata).
   Because coresight_get_mode(drvdata->csdev) is no longer
   CS_MODE_SYSFS, tmc_clean_etr_buf_list() takes the else branch:

	if (nd->sysfs_buf == drvdata->sysfs_buf) {
		if (coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS)
			nd->sysfs_buf = NULL;
		else
			/* Free the buffer below through nd->sysfs_buf */
			drvdata->sysfs_buf = NULL;
	}

   and frees the pre-existing drvdata->sysfs_buf via
   tmc_etr_free_sysfs_buf(nd->sysfs_buf), discarding the synced sysfs
   trace data on an error rollback.

2. If all allocations in the loop succeed after the sink was
   concurrently disabled, the loop re-acquires drvdata->spinlock and
   adds the nodes to drvdata->etr_buf_list without re-checking
   coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS, returning 0
   with a populated etr_buf_list on a sink that is no longer in
   CS_MODE_SYSFS.

Could tmc_create_etr_buf_list() re-check
coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS under
drvdata->spinlock and ensure error cleanup does not free the
pre-existing drvdata->sysfs_buf?

[1] https://lore.kernel.org/r/20261030-enable-byte-cntr-for-ctcu-v20-2-bd5c3ac9f916@oss.qualcomm.com
[2] https://lore.kernel.org/r/20261030-enable-byte-cntr-for-ctcu-v20-2-bd5c3ac9f916@oss.qualcomm.com

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