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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] coresight: data race in etm4_disable_sysfs() due to missing spinlock
--

commit 047fa1646a004922cd9c2e2d801758f38062c7c6
Author: Yeoreum Yun <yeoreum.yun@arm.com>
Subject: coresight: etm4x: fix inconsistencies with sysfs configuration

This commit separates the ETM4x configuration into a session-specific 
curr_config and a sysfs-set config to prevent corruption and inconsistency. 
It also removes drvdata->spinlock coverage from the enable/disable paths 
except when explicitly accessing configuration 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

[ ... ]

> @@ -1134,8 +1142,9 @@ static int etm4_disable_perf(struct coresight_device *csdev,
>  static void etm4_disable_sysfs(struct coresight_device *csdev)
>  {
>  	struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> -
> -	raw_spin_lock(&drvdata->spinlock);
> +	struct etmv4_config *config = &drvdata->config;
> +	const struct etmv4_config *curr_config = &drvdata->curr_config;
> +	int i;
>  
>  	/*
>  	 * Executing etm4_disable_hw on the cpu whose ETM is being 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);
>  
> -	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]
Does this modification to the config fields require drvdata->spinlock
protection?

The commit message notes the intent to remove the lock "except when to
access config fields", which is correctly followed during sysfs enable
via scoped_guard. However, sysfs readers expect this spinlock to be held
when reading these arrays.

For example, a sysfs read legitimately holds the lock here:

drivers/hwtracing/coresight/coresight-etm4x-sysfs.c:sshot_status_show() {
    ...
    raw_spin_lock(&drvdata->spinlock);
    val = config->ss_status[config->ss_idx];
    raw_spin_unlock(&drvdata->spinlock);
    ...
}

Writing to config->ss_status, config->cntr_val, and config->seq_state
concurrently without the lock in etm4_disable_sysfs() could create a
data race resulting in KCSAN warnings and inconsistent reads.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261003-separate_etm_cfg_v2-v13-0-5d8a29aead95@arm.com?part=4