Re: [PATCH v10 3/8] coresight: etm3x: change drvdata->spinlock type to raw_spin_lock_t

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

[...]

> 
> > diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> > index b3c67e96a82a4..0701edc094581 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> > @@ -49,13 +49,13 @@ static ssize_t etmsr_show(struct device *dev,
> >  	struct etm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> >  
> >  	pm_runtime_get_sync(dev->parent);
> > -	spin_lock_irqsave(&drvdata->spinlock, flags);
> > +	raw_spin_lock_irqsave(&drvdata->spinlock, flags);
> >  	CS_UNLOCK(drvdata->csa.base);
> >  
> >  	val = etm_readl(drvdata, ETMSR);
> 
> [Severity: High]
> This is a pre-existing issue, but could this code cause a synchronous
> external abort if the device fails to resume?
> 
> If pm_runtime_get_sync() returns a negative error code (e.g., if the device
> is suspended and its clocks or power domains remain gated), the code proceeds
> unconditionally to dereference drvdata->csa.base in etm_readl(). 
> 
> Accessing unclocked or unpowered CoreSight registers on ARM can result in a
> kernel panic. Should the return value of pm_runtime_get_sync() be checked
> before accessing the hardware?

Valid. might in another patchset.

> 
> [Severity: High]
> This is another pre-existing issue, but can this sysfs read silently break
> concurrent hardware enablement?
> 
> Concurrent sysfs readers like etmsr_show() use drvdata->spinlock to serialize
> their own hardware accesses, issuing CS_UNLOCK and CS_LOCK. However,
> etm_enable_hw() executes via smp_call_function_single() and does not hold
> drvdata->spinlock while issuing its own register writes and CS_LOCK.
> 
> If a sysfs read occurs concurrently with etm_enable_hw():
> 
> Sysfs thread                           etm_enable_hw()
> etmsr_show()
>   raw_spin_lock_irqsave()
>   CS_UNLOCK()
>                                        CS_UNLOCK()
>                                        write to registers...
>   CS_LOCK()                            
>                                        write to more registers... (ignored)
>                                        CS_LOCK()
> 
> Will the sysfs read's CS_LOCK lock the CoreSight Lock Access Register (LAR)
> in the middle of etm_enable_hw()'s configuration sequence, causing
> subsequent register writes in etm_enable_hw() to be silently ignored?
> 
> [ ... ]
> 
> > @@ -76,7 +76,7 @@ static ssize_t reset_store(struct device *dev,
> >  		return ret;
> >  
> >  	if (val) {
> > -		spin_lock(&drvdata->spinlock);
> > +		raw_spin_lock(&drvdata->spinlock);
> >  		memset(config, 0, sizeof(struct etm_config));
> >  		config->mode = ETM_MODE_EXCLUDE;
> >  		config->trigger_event = ETM_DEFAULT_EVENT_VAL;
> > @@ -86,7 +86,7 @@ static ssize_t reset_store(struct device *dev,
> >  
> >  		etm_set_default(config);
> >  		etm_release_trace_id(drvdata);
> > -		spin_unlock(&drvdata->spinlock);
> > +		raw_spin_unlock(&drvdata->spinlock);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but can this cause a trace ID
> use-after-free and corrupt the trace stream?
> 
> This resets the staging configuration and unconditionally calls
> etm_release_trace_id() without checking if the tracer is currently active
> (e.g., via coresight_get_mode()). 
> 
> If writing to this reset attribute happens while a tracing session is running,
> the trace ID is returned to the pool and can be reallocated to another
> CoreSight component while this device continues to emit hardware packets using
> the freed ID.
> 

I think we should allow the reset when coresight_get_mode() == DISABLE
only. @Leo?

> > @@ -722,9 +722,9 @@ static ssize_t cntr_val_show(struct device *dev,
> >  	struct etm_config *config = &drvdata->config;
> >  
> >  	if (!coresight_get_mode(drvdata->csdev)) {
> > -		spin_lock(&drvdata->spinlock);
> > +		raw_spin_lock(&drvdata->spinlock);
> >  		val = config->cntr_val[config->cntr_idx];
> > -		spin_unlock(&drvdata->spinlock);
> > +		raw_spin_unlock(&drvdata->spinlock);
> >  	} else {
> >  		val = etm_readl(drvdata, ETMCNTVRn(config->cntr_idx));
> 
> [Severity: High]
> This is a pre-existing issue, but can this race with a concurrent power down
> and cause a synchronous external abort?
> 
> The check for coresight_get_mode() bypasses both the spinlock and
> pm_runtime_get_sync(). If the device is active during the check, it jumps
> to the else branch to read the hardware register. 
> 
> If another thread disables the device (which calls pm_runtime_put() and
> powers down the hardware) concurrently, this will read from powered-off
> hardware.
> 
> Additionally, is it safe to read config->cntr_idx here without a lock?
> 
> [ ... ]
> 
> > @@ -940,13 +940,13 @@ static ssize_t seq_curr_state_show(struct device *dev,
> >  	}
> >  
> >  	pm_runtime_get_sync(dev->parent);
> > -	spin_lock_irqsave(&drvdata->spinlock, flags);
> > +	raw_spin_lock_irqsave(&drvdata->spinlock, flags);
> >  
> >  	CS_UNLOCK(drvdata->csa.base);
> >  	val = (etm_readl(drvdata, ETMSQR) & ETM_SQR_MASK);
> 
> [Severity: High]
> This is a pre-existing issue, but as with etmsr_show() earlier, could this
> cause a synchronous external abort if the device fails to resume?
> 
> If pm_runtime_get_sync() returns a negative error, accessing the unclocked
> hardware register will cause a kernel panic.

@Leo, I think both of direct read from sysfs interface should be
synchronized via IPI. What do you think?

-- 
Sincerely,
Yeoreum Yun