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

[Yeoreum Yun]

On Sat, Oct 03, 2026 at 01:33:46AM +0000, sashiko-bot@xxxxxxxxxx wrote:
> 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.

This is totally wrong. since sysfs seesion is disabled before *offline*
by coresight_dying_cpu().

> >  
> > -	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?

It's okay since the ss_status's purpose is to clear the PENDING and
STATUS bit otherwise, RES and RO field.

> 
> > +
> > +	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

-- 
Sincerely,
Yeoreum Yun