| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6be400e2-a464-4714-acf4-328dade883a9@linaro.org>
Date: Mon, 17 Mar 2025 15:05:41 +0000
From: James Clark <james.clark@...aro.org>
To: Leo Yan <leo.yan@....com>
Cc: lcherian@...vell.com, coresight@...ts.linaro.org,
Mike Leach <mike.leach@...aro.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-stm32@...md-mailman.stormreply.com
Subject: Re: [PATCH 5/7] coresight: Clear self hosted claim tag on probe
On 13/03/2025 4:04 pm, Leo Yan wrote:
> On Tue, Feb 11, 2025 at 10:39:41AM +0000, James Clark wrote:
>>
>> This can be left behind from a crashed kernel after a kexec so clear it
>> when probing each device. Similarly to
>> coresight_disclaim_device_unlocked(), only clear it if it's already set
>> to avoid races with an external debugger.
>>
>> We need a csdev_access struct in etm_init_arch_data() so just replace
>> the iomem pointer with a full csdev_access struct. This means all usages
>> need to be updated to go through csa->base.
>>
>> Signed-off-by: James Clark <james.clark@...aro.org>
>> ---
>> drivers/hwtracing/coresight/coresight-catu.c | 1 +
>> drivers/hwtracing/coresight/coresight-core.c | 48 +++++++++++++++----
>> .../hwtracing/coresight/coresight-cti-core.c | 2 +
>> drivers/hwtracing/coresight/coresight-etb10.c | 2 +
>> drivers/hwtracing/coresight/coresight-etm.h | 6 +--
>> .../coresight/coresight-etm3x-core.c | 28 +++++------
>> .../coresight/coresight-etm3x-sysfs.c | 8 ++--
>> .../coresight/coresight-etm4x-core.c | 2 +
>> .../hwtracing/coresight/coresight-funnel.c | 2 +
>> .../coresight/coresight-replicator.c | 1 +
>> .../hwtracing/coresight/coresight-tmc-core.c | 1 +
>> include/linux/coresight.h | 3 ++
>> 12 files changed, 73 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
>> index d9259c0b6e64..575c2d247a90 100644
>> --- a/drivers/hwtracing/coresight/coresight-catu.c
>> +++ b/drivers/hwtracing/coresight/coresight-catu.c
>> @@ -558,6 +558,7 @@ static int __catu_probe(struct device *dev, struct resource *res)
>> catu_desc.subtype.helper_subtype = CORESIGHT_DEV_SUBTYPE_HELPER_CATU;
>> catu_desc.ops = &catu_ops;
>>
>> + coresight_reset_claim(&catu_desc.access);
>> drvdata->csdev = coresight_register(&catu_desc);
>> if (IS_ERR(drvdata->csdev))
>> ret = PTR_ERR(drvdata->csdev);
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
>> index 7fe5d5d432c4..97f33ffad05e 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -212,20 +212,48 @@ int coresight_claim_device(struct coresight_device *csdev)
>> EXPORT_SYMBOL_GPL(coresight_claim_device);
>>
>> /*
>> - * coresight_disclaim_device_unlocked : Clear the claim tag for the device.
>> + * Clear the claim tag for the device.
>> + * Returns an error if the device wasn't already claimed.
>> + */
>> +int coresight_reset_claim(struct csdev_access *csa)
>> +{
>> + int ret;
>> +
>> + CS_UNLOCK(csa->base);
>> + ret = coresight_reset_claim_unlocked(csa);
>> + CS_LOCK(csa->base);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(coresight_reset_claim);
>
> Maybe my question is overlapping with Mike's comment.
>
> Callers never check the return values from coresight_reset_claim(). I am
I can remove the return value if it's confusing. The thought process was
probably that it could be useful somewhere in the future, and
coresight_reset_claim_unlocked() returns something anyway so might as
well pass it up.
> wandering if coresight_reset_claim() can directly call
> coresight_clear_self_claim_tag() for _trying_ to clear self-host tag in
> probe phase. Any self claim tag issues will be deferred to detect until
> enable the component.
>
Maybe, the spec does a read before setting which I assumed should be
done for clearing as well. As in to not touch anything if it's in use
externally. It doesn't specifically describe any clearing sequence, but
if we assume it's ok to blindly clear self hosted flag even when it's in
use then yes we can directly call coresight_clear_self_claim_tag().
This doesn't change anything about deferring errors though,
coresight_reset_claim() doesn't warn early either.
> For consistent, we might rename coresight_reset_claim() to
> coresight_reset_self_claim_tag(), which acquires CS lock and clear
> self claim tag.
>
>> +/*
>> + * Clear the claim tag for the device. Called with CS_UNLOCKed for the component.
>> + * Returns an error if the device wasn't already claimed.
>> + */
>> +int coresight_reset_claim_unlocked(struct csdev_access *csa)
>> +{
>> + if (coresight_read_claim_tags(csa) == CORESIGHT_CLAIM_SELF_HOSTED) {
>> + coresight_clear_self_claim_tag(csa);
>> + return 0;
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +EXPORT_SYMBOL_GPL(coresight_reset_claim_unlocked);
>> +
>> +/*
>> + * coresight_disclaim_device_unlocked : Clear the claim tag for the device
>> + * and warn if the device wasn't already claimed.
>> * Called with CS_UNLOCKed for the component.
>> */
>> void coresight_disclaim_device_unlocked(struct csdev_access *csa)
>> {
>> - if (coresight_read_claim_tags(csa) == CORESIGHT_CLAIM_SELF_HOSTED)
>> - coresight_clear_self_claim_tag(csa);
>> - else
>> - /*
>> - * The external agent may have not honoured our claim
>> - * and has manipulated it. Or something else has seriously
>> - * gone wrong in our driver.
>> - */
>> - WARN_ON_ONCE(1);
>> + /*
>> + * Warn if the external agent hasn't honoured our claim
>> + * and has manipulated it. Or something else has seriously
>> + * gone wrong in our driver.
>> + */
>> + WARN_ON_ONCE(coresight_reset_claim_unlocked(csa));
>> }
>> EXPORT_SYMBOL_GPL(coresight_disclaim_device_unlocked);
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c
>> index 073f67a41af9..389a72362f0c 100644
>> --- a/drivers/hwtracing/coresight/coresight-cti-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-cti-core.c
>> @@ -931,6 +931,8 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id)
>> cti_desc.ops = &cti_ops;
>> cti_desc.groups = drvdata->ctidev.con_groups;
>> cti_desc.dev = dev;
>> +
>> + coresight_reset_claim(&cti_desc.access);
>> drvdata->csdev = coresight_register(&cti_desc);
>> if (IS_ERR(drvdata->csdev)) {
>> ret = PTR_ERR(drvdata->csdev);
>> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
>> index d8bc3e776c88..b598b2c0c9bb 100644
>> --- a/drivers/hwtracing/coresight/coresight-etb10.c
>> +++ b/drivers/hwtracing/coresight/coresight-etb10.c
>> @@ -772,6 +772,8 @@ static int etb_probe(struct amba_device *adev, const struct amba_id *id)
>> desc.pdata = pdata;
>> desc.dev = dev;
>> desc.groups = coresight_etb_groups;
>> +
>> + coresight_reset_claim(&desc.access);
>> drvdata->csdev = coresight_register(&desc);
>> if (IS_ERR(drvdata->csdev))
>> return PTR_ERR(drvdata->csdev);
>> diff --git a/drivers/hwtracing/coresight/coresight-etm.h b/drivers/hwtracing/coresight/coresight-etm.h
>> index e02c3ea972c9..a89736309c27 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm.h
>> +++ b/drivers/hwtracing/coresight/coresight-etm.h
>> @@ -229,7 +229,7 @@ struct etm_config {
>> * @config: structure holding configuration parameters.
>> */
>> struct etm_drvdata {
>> - void __iomem *base;
>> + struct csdev_access csa;
>
> I would like to extract the change for using `csdev_access` in the
> ETMv3 driver into a new patch, which is irrelevant to reset self claim
> tags and would significantly reduce the complexity in this patch.
>
Will do, good point.
> Thanks,
> Leo
Powered by blists - more mailing lists