Re: [PATCH v13 05/14] coresight: etm3x: fix inconsistencies with sysfs configuration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] coresight: etm3x: race condition exposes powered-down hardware reads
- [Medium] coresight: etm3x: data race on cntr_idx in sysfs read
--

commit 6e8889b8f94797194e00dbd7c1a4354b7b49c21d
Author: Yeoreum Yun <yeoreum.yun@xxxxxxx>

coresight: etm3x: fix inconsistencies with sysfs configuration

This commit separates ETM3x configuration into curr_config and config to
resolve inconsistencies when modified via sysfs during an active perf session.
It also removes the spinlock from sysfs enable/disable paths and uses IPIs to
read registers on the proper CPU.

> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> index 862ad0786699c..dcadcc3830123 100644
> --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
[ ... ]
> @@ -640,7 +641,14 @@ static void etm_disable_sysfs(struct coresight_device *csdev)
>  	smp_call_function_single(drvdata->cpu, etm_disable_sysfs_smp_call,
>  				 drvdata, 1);
>  
> -	spin_unlock(&drvdata->spinlock);
> +	/*
> +	 * Userspace may read seq_curr_state and cntr_val through sysfs
> +	 * after the sysfs-session has been disabled.
> +	 */
> +	for (i = 0; i < drvdata->nr_cntr; i++)
> +		config->cntr_val[i] = curr_config->cntr_val[i];
> +
> +	config->seq_curr_state = curr_config->seq_curr_state;
>  
>  	/*
>  	 * we only release trace IDs when resetting sysfs.
> @@ -649,6 +657,8 @@ static void etm_disable_sysfs(struct coresight_device *csdev)
>  	 * prior trace id allocation method
>  	 */
>  
> +	coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED);
> +

[Severity: High]
Can this expose a window where hardware registers are read while the device
is powered down?

The commit removes the drvdata->spinlock that previously serialized sysfs
reads against enable/disable paths. In etm_disable_sysfs(), the hardware is
powered down via smp_call_function_single(), but the mode is not changed
to CS_MODE_DISABLED until much later here.

During this window, could a concurrent sysfs reader like
seq_curr_state_show() observe coresight_get_mode() as CS_MODE_SYSFS and
proceed to call etm_readl_cslocked()? Reading from hardware registers such
as ETMSQR when the ETM power-down bit is set can lead to bus faults (SError)
or return garbage.

>  	dev_dbg(&csdev->dev, "ETM tracing disabled\n");
>  }
>  
> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> index cdb70a4382eb8..284998f180170 100644
> --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
[ ... ]
> @@ -730,7 +765,10 @@ static ssize_t cntr_val_show(struct device *dev,
>  		val = config->cntr_val[config->cntr_idx];
>  		spin_unlock(&drvdata->spinlock);
>  	} else {
> -		val = etm_readl(drvdata, ETMCNTVRn(config->cntr_idx));
> +		ret = etm_readl_cslocked(drvdata, ETMCNTVRn(config->cntr_idx), &val2);

[Severity: Medium]
Is it safe to read config->cntr_idx here without holding drvdata->spinlock?

Concurrent updates from cntr_idx_store() via sysfs could change this value
mid-execution. Since the spinlock was removed from this branch, could this
data race cause the lockless read to fetch an inconsistent index and target
an unintended counter register?

> +		if (ret)
> +			return ret;
> +		val = val2;
>  	}
>  
>  	return sysfs_emit(buf, "%#x\n", val);
[ ... ]
> @@ -946,15 +985,10 @@ static ssize_t seq_curr_state_show(struct device *dev,
>  		goto out;
>  	}
>  
> -	pm_runtime_get_sync(dev->parent);
> -	spin_lock_irqsave(&drvdata->spinlock, flags);
> -
> -	CS_UNLOCK(drvdata->csa.base);
> -	val = (etm_readl(drvdata, ETMSQR) & ETM_SQR_MASK);
> -	CS_LOCK(drvdata->csa.base);
> -
> -	spin_unlock_irqrestore(&drvdata->spinlock, flags);
> -	pm_runtime_put(dev->parent);
> +	ret = etm_readl_cslocked(drvdata, ETMSQR, &val);

[Severity: High]
This is where a concurrent reader could read a powered-down register due to
the window mentioned above in etm_disable_sysfs().

> +	if (ret)
> +		return ret;
> +	val &= ETM_SQR_MASK;
>  out:
>  	return sprintf(buf, "%#lx\n", val);
>  }

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