Re: [PATCH v13 04/14] coresight: etm4x: fix inconsistencies with sysfs configuration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] coresight: etm4x: data race in etm4_disable_sysfs() due to missing spinlock
- [Medium] coresight: etm4x: resource leak when disabling sysfs tracing on an offline CPU
--

commit 431ca53aed38063c4c77184079a32f5fbbb5b69b
Author: Yeoreum Yun <yeoreum.yun@xxxxxxx>

coresight: etm4x: fix inconsistencies with sysfs configuration

This commit separates the configuration into curr_config and config to prevent
inconsistencies when modifying via sysfs during an active perf session. It
also removes the lock/unlock from the sysfs enable/disable path and
startup/dying_cpu, except when accessing config fields.

> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -1090,8 +1097,9 @@ static void etm4_disable_sysfs_smp_call(void *info)
>  
>  	etm4_disable_hw(drvdata);
>  
> +	cscfg_csdev_disable_active_config(drvdata->csdev);
> +
>  	drvdata->csdev->path = NULL;
> -	coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED);
>  }

[ ... ]

> @@ -1144,9 +1153,19 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
>  	smp_call_function_single(drvdata->cpu, etm4_disable_sysfs_smp_call,
>  				 drvdata, 1);

[Severity: Medium]
If smp_call_function_single() fails (for instance, if the target CPU is
offline), it will return -ENXIO and bypass the callback entirely.

Since cscfg_csdev_disable_active_config() was moved into the SMP callback
etm4_disable_sysfs_smp_call(), does this mean we could permanently leak the
active configuration when the CPU is offline? This could leave the module
reference count (active_cnt) leaked and active_cscfg_ctxt dangling.

>  
> -	raw_spin_unlock(&drvdata->spinlock);
> +	/*
> +	 * Userspace may read ss_status and cntr_val through sysfs after
> +	 * the sysfs-session has been disabled.
> +	 */
> +	for (i = 0; i < drvdata->nr_ss_cmp; i++)
> +		config->ss_status[i] = curr_config->ss_status[i];
>  
> -	cscfg_csdev_disable_active_config(csdev);
> +	for (i = 0; i < drvdata->nr_cntr; i++)
> +		config->cntr_val[i] = curr_config->cntr_val[i];
> +
> +	config->seq_state = curr_config->seq_state;

[Severity: Medium]
Is it safe to copy curr_config to config here without holding the
drvdata->spinlock?

Sysfs writers (like reset_store()) and readers (like cntr_val_show()) do
not check if the device is currently in CS_MODE_SYSFS; they rely solely on
the spinlock for synchronization.

Could this create a data race where concurrent sysfs operations are
overwritten by this lockless loop, corrupting the sysfs-exposed state?

> +
> +	coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED);
>  
>  	/*
>  	 * we only release trace IDs when resetting sysfs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002-separate_etm_cfg_v2-v13-0-fe429d8d96fb@xxxxxxx?part=4