Re: [PATCH v11 2/9] coresight: etm3x: prohibit modifying cntr_val and reset while session is enabled

[Yeoreum Yun]

On Tue, Sep 15, 2026 at 02:30:06PM +0100, Yeoreum Yun wrote:
> On Tue, Sep 15, 2026 at 11:48:44AM +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 sysfs configuration store functions
> > --
> > 
> > commit 9a331a4cd5a9fd51e7380e92ed6bdd6d8e20d5e4
> > Author: Yeoreum Yun <yeoreum.yun@xxxxxxx>
> > 
> > coresight: etm3x: prohibit modifying cntr_val and reset while session is enabled
> > 
> > This commit attempts to prevent modifications to cntr_val and reset while
> > a CoreSight trace session is enabled. It adds a check for the current
> > mode using coresight_get_mode() before proceeding with the sysfs store
> > operations.
> > 
> > > diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> > > --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> > > +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> > > @@ -74,6 +74,8 @@ static ssize_t reset_store(struct device *dev,
> > >  	ret = kstrtoul(buf, 16, &val);
> > >  	if (ret)
> > >  		return ret;
> > > +	if (coresight_get_mode(drvdata->csdev))
> > > +		return -EBUSY;
> > >  
> > >  	if (val) {
> > >  		spin_lock(&drvdata->spinlock);
> > 
> > [Severity: Medium]
> > Could this lockless check introduce a time-of-check to time-of-use race?
> > 
> > If a concurrent thread starts a trace session via the enable_source sysfs
> > file right after coresight_get_mode() returns false, it might win the race
> > before spin_lock() is acquired here.
> > 
> > This could result in resetting the configuration of an active trace session
> > and erroneously releasing its trace ID, potentially leading to trace ID
> > collisions and hardware trace stream corruption.
> > 
> > [ ... ]
> > 
> > > @@ -744,6 +746,8 @@ static ssize_t cntr_val_store(struct device *dev,
> > >  	ret = kstrtoul(buf, 16, &val);
> > >  	if (ret)
> > >  		return ret;
> > > +	if (coresight_get_mode(drvdata->csdev))
> > > +		return -EBUSY;
> > >  
> > >  	spin_lock(&drvdata->spinlock);
> > 
> > [Severity: Medium]
> > Is this susceptible to a similar race condition as reset_store?
> > 
> > Since coresight_get_mode() is checked outside the spinlock, a concurrent
> > thread could enable the session just before spin_lock() is acquired,
> > allowing the counter value to be modified while the session is active.
> 
> Anyway, the reset interface is nausty. but yeap it would be better to
> check after grabbing *lock*  in here.

But, later patch seperates the config. So it could be ignored.

-- 
Sincerely,
Yeoreum Yun