Re: [PATCH v12 01/14] coresight: etm4x: read-back TRCSEQSTR at disabling and prohibit modifying seq_state while enabling.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] coresight: TOCTOU race in seq_state_store lockless mode check
- [Low] coresight: misplaced IS_ERR_OR_NULL(drvdata) after dereference
--

commit fb240199cfa6db080873efbe2c007beb70c01070
Author: Yeoreum Yun <yeoreum.yun@arm.com>

coresight: etm4x: read-back TRCSEQSTR at disabling and prohibit modifying seq_state while enabling.

TRCSEQSTR can be changed by the ETM while a session is active. Its value
therefore needs to be preserved when a sysfs session is disabled so that
users can read the current sequence state after the session ends.

> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> index cc6cdd3ae29d5..db3cf4ad6806c 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> @@ -1437,6 +1437,8 @@ static ssize_t seq_state_store(struct device *dev,
>  		return -EINVAL;
>  	if (val >= drvdata->nrseqstate)
>  		return -EINVAL;
> +	if (IS_ERR_OR_NULL(drvdata) || coresight_get_mode(drvdata->csdev))
> +		return -EBUSY;

[Severity: Low]
Does this code introduce a regression by checking drvdata for NULL after it
has already been dereferenced?

Looking at seq_state_store(), drvdata is dereferenced during the check for
val >= drvdata->nrseqstate before IS_ERR_OR_NULL(drvdata) is called.

While drvdata is fundamentally guaranteed to be valid in this sysfs context,
making the check technically dead code rather than a reachable crash, should
the validation be moved before the dereference if it is kept?

>  
>  	config->seq_state = val;

[Severity: Medium]
Could this lockless mode check cause a regression?

A concurrent thread could enable the trace session immediately after the
lockless coresight_get_mode() check evaluates to disabled. This would allow
config->seq_state to be modified while the session is actively starting or
already active, completely defeating the patch's stated goal of prohibiting
state modification during an active session.

Should raw_spin_lock(&drvdata->spinlock) be held across both the mode check
and the subsequent state update to prevent this regression?

>  	return size;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-separate_etm_cfg_v2-v12-0-a2b41d2f7c2b@arm.com?part=1