Re: [PATCH v11 4/9] coresight: etm3x: fix inconsistencies with sysfs configuration

[Yeoreum Yun <yeoreum.yun@arm.com>]

On Tue, Sep 15, 2026 at 11:47:57AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] coresight: TOCTOU race in sysfs read paths exposes powered-down hardware
> - [Low] coresight: benign data race on config->cntr_idx in cntr_val_show
> --
> 
> commit 7fbfa36b293dee9eea85d0d29da9faaa208abb46
> Author: Yeoreum Yun <yeoreum.yun@arm.com>
> 
> coresight: etm3x: fix inconsistencies with sysfs configuration
> 
> This commit splits the sysfs configuration into active and mode
> configurations to fix inconsistencies during active perf sessions. It also
> avoids locking around some sysfs paths and uses IPIs to read registers
> directly on the appropriate CPUs to reduce lock scopes.
> 
> > diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> > index 862ad0786699c..fd76a57e5f861 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> [ ... ]
> > @@ -630,8 +632,9 @@ static void etm_disable_perf(struct coresight_device *csdev)
> >  static void etm_disable_sysfs(struct coresight_device *csdev)
> >  {
> >  	struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> > -
> > -	spin_lock(&drvdata->spinlock);
> > +	struct etm_config *config = &drvdata->config;
> > +	const struct etm_config *active_config = &drvdata->active_config;
> > +	int i;
> >  
> >  	/*
> >  	 * Executing etm_disable_hw on the cpu whose ETM is being disabled
> > @@ -640,7 +643,14 @@ static void etm_disable_sysfs(struct coresight_device *csdev)
> >  	smp_call_function_single(drvdata->cpu, etm_disable_sysfs_smp_call,
> >  				 drvdata, 1);
> >  
> 
> [Severity: Medium]
> When thread A disables tracing via etm_disable_sysfs(), it sends an IPI to
> power down the hardware here. Could a concurrent reader (like Thread B in
> cntr_val_show()) see the mode still set to CS_MODE_SYSFS, and issue its own
> IPI to read the hardware? Since the hardware is already powered down by
> Thread A's IPI, Thread B might read a stale or garbage value instead of the
> correctly saved trace state, breaking post-trace analysis.

In that case, no IPI is shown, and the last Thread B see the
Thread A's saving value of cntr_vals where is saved in the config.

It doesn't break anything.

> 
> > -	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] = active_config->cntr_val[i];
> > +
> > +	config->seq_curr_state = active_config->seq_curr_state;
> >  
> >  	/*
> >  	 * we only release trace IDs when resetting sysfs.
> > @@ -649,6 +659,8 @@ static void etm_disable_sysfs(struct coresight_device *csdev)
> >  	 * prior trace id allocation method
> >  	 */
> >  
> > +	coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED);
> 
> [Severity: Medium]
> The software state is updated here, creating a race window between the
> hardware disable above and the mode update here where the hardware is off but
> the mode is still CS_MODE_SYSFS. A similar window exists during
> etm_enable_sysfs().

Non-sence.

> 
> > +
> >  	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 d4846d6364945..2c8c129228589 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> [ ... ]
> > @@ -719,7 +752,9 @@ static DEVICE_ATTR_RW(cntr_rld_event);
> >  static ssize_t cntr_val_show(struct device *dev,
> >  			     struct device_attribute *attr, char *buf)
> >  {
> > +	int ret;
> >  	u32 val;
> > +	unsigned long val2;
> >  	struct etm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> >  	struct etm_config *config = &drvdata->config;
> >  
> > @@ -728,7 +763,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: Low]
> Does reading config->cntr_idx without locking here create a data race? It
> appears it can race with concurrent updates in cntr_idx_store(). While this
> is likely benign since it is an 8-bit integer and bounded by
> cntr_idx_store(), preventing out-of-bounds access, could it still trigger
> sanitizer warnings when accessed by authorized users?
> 
> [Severity: Medium]
> This reader accesses the hardware assuming it is active, based on a lockless
> mode check in the if condition. If this races with etm_disable_sysfs()
> powering down the hardware before the mode is updated to CS_MODE_DISABLED,
> could this read return garbage values?
> 
> > +		if (ret)
> > +			return ret;
> > +		val = val2;
> >  	}
> >  
> >  	return sysfs_emit(buf, "%#x\n", val);

Ignorable.

-- 
Sincerely,
Yeoreum Yun