Re: [PATCH RESEND v13 02/14] coresight: etm4x: prohibit modifying cntr_val while session is enabled

[Yeoreum Yun]

On Sat, Oct 03, 2026 at 03:06:39AM +0000, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] coresight: TOCTOU race in cntr_val_store due to lockless mode check
> --
> 
> commit da77274417a7a9a1e577464873f49041c777a7fc
> Author: Yeoreum Yun <yeoreum.yun@xxxxxxx>
> 
> coresight: etm4x: prohibit modifying cntr_val while session is enabled
> 
> This commit adds a check in cntr_val_store() to prevent modifying the
> cntr_val configuration while a Coresight session is enabled. It returns
> -EBUSY if the session is currently active.
> 
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> > @@ -1615,6 +1615,9 @@ static ssize_t cntr_val_store(struct device *dev,
> >  		return -EINVAL;
> >  	if (val > ETM_CNTR_MAX_VAL)
> >  		return -EINVAL;
> > +	if (IS_ERR_OR_NULL(drvdata->csdev) ||
> > +	    coresight_get_mode(drvdata->csdev))
> > +		return -EBUSY;
> >  
> >  	raw_spin_lock(&drvdata->spinlock);
> 
> [Severity: Medium]
> Could this leave a race window before the state is modified?
> 
> Since the mode check is performed locklessly before acquiring
> drvdata->spinlock, another process could enable the session concurrently. If
> the session is enabled right after this check evaluates to false, but before
> cntr_val_store() acquires the lock, the configuration update would be lost.
> 
> The concurrent enablement would set the mode and call etm4_enable_sysfs()
> to program the hardware. If cntr_val_store() acquires the lock after this,
> it updates the software state which is never flushed to hardware for the
> active session, and will eventually be overwritten by the old hardware state
> during session teardown.
> 
> Should the mode check be moved inside the spinlock critical section to
> properly serialize with session enablement?
> 

The puprpose of this to prevent modification after *session* is fully
enabled since the cntr_val is overwritten after disabled.

IOW, while enabling the cntr_val is not matter and this secnario
couldn't be resolved by the locking -- it's matter who take the lock first
while on concurrent contention between store vs enable also, it's
pre-exist behavior.

Since we don't expect user try to enable before it finishes to config.
Therefore, this is ignorable. 

-- 
Sincerely,
Yeoreum Yun