| 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
| ||
|
Date: Fri, 07 Aug 2020 11:58:49 +0800
From: tingwei@...eaurora.org
To: Tingwei Zhang <tingweiz@...eaurora.org>
Cc: Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>,
Mathieu Poirier <mathieu.poirier@...aro.org>,
Suzuki K Poulose <suzuki.poulose@....com>,
linux-arm-msm@...r.kernel.org, coresight@...ts.linaro.org,
linux-kernel@...r.kernel.org, Stephen Boyd <swboyd@...omium.org>,
linux-arm-kernel@...ts.infradead.org,
Mike Leach <mike.leach@...aro.org>
Subject: Re: [PATCHv3] coresight: etm4x: Fix etm4_count race by moving cpuhp
callbacks to init
Sorry for the noise. Please ignore previous comment.
The change is in old patch set of my series.
This change is good to go.
On 2020-08-07 11:52, Tingwei Zhang wrote:
> On Wed, Jul 29, 2020 at 01:13:10PM +0800, Sai Prakash Ranjan wrote:
>> etm4_count keeps track of number of ETMv4 registered and on some
>> systems,
>> a race is observed on etm4_count variable which can lead to multiple
>> calls
>> to cpuhp_setup_state_nocalls_cpuslocked(). This function internally
>> calls
>> cpuhp_store_callbacks() which prevents multiple registrations of
>> callbacks
>> for a given state and due to this race, it returns -EBUSY leading to
>> ETM
>> probe failures like below.
>>
>> coresight-etm4x: probe of 7040000.etm failed with error -16
>>
>> This race can easily be triggered with async probe by setting probe
>> type
>> as PROBE_PREFER_ASYNCHRONOUS and with ETM power management property
>> "arm,coresight-loses-context-with-cpu".
>>
>> Prevent this race by moving cpuhp callbacks to etm driver init since
>> the
>> cpuhp callbacks doesn't have to depend on the etm4_count and can be
>> once
>> setup during driver init. Similarly we move cpu_pm notifier
>> registration
>> to driver init and completely remove etm4_count usage. Also now we can
>> use non cpuslocked version of cpuhp callbacks with this movement.
>>
>> Fixes: 9b6a3f3633a5 ("coresight: etmv4: Fix CPU power management setup
>> in
>> probe() function")
>> Fixes: 58eb457be028 ("hwtracing/coresight-etm4x: Convert to hotplug
>> state
>> machine")
>> Suggested-by: Suzuki K Poulose <suzuki.poulose@....com>
>> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>
>> ---
>>
>> Changes in v3:
>> * Minor cleanups from v2 and change to device_initcall (Stephen Boyd)
>> * Move to non cpuslocked cpuhp callbacks and rename to etm_pm_setup()
>> (Mike
>> Leach)
>>
>> Changes in v2:
>> * Rearrange cpuhp callbacks and move them to driver init (Suzuki K
>> Poulose)
>>
>> ---
>> drivers/hwtracing/coresight/coresight-etm4x.c | 65
>> +++++++++----------
>> 1 file changed, 31 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c
>> b/drivers/hwtracing/coresight/coresight-etm4x.c
>> index 6d7d2169bfb2..fddfd93b9a7b 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
>> @@ -48,8 +48,6 @@ module_param(pm_save_enable, int, 0444);
>> MODULE_PARM_DESC(pm_save_enable,
>> "Save/restore state on power down: 1 = never, 2 = self-hosted");
>>
>> -/* The number of ETMv4 currently registered */
>> -static int etm4_count;
>> static struct etmv4_drvdata *etmdrvdata[NR_CPUS];
>> static void etm4_set_default_config(struct etmv4_config *config);
>> static int etm4_set_event_filters(struct etmv4_drvdata *drvdata,
>> @@ -1398,28 +1396,25 @@ static struct notifier_block etm4_cpu_pm_nb =
>> {
>> .notifier_call = etm4_cpu_pm_notify,
>> };
>>
>> -/* Setup PM. Called with cpus locked. Deals with error conditions and
>> counts */
>> -static int etm4_pm_setup_cpuslocked(void)
>> +/* Setup PM. Deals with error conditions and counts */
>> +static int __init etm4_pm_setup(void)
>> {
>> int ret;
>>
>> - if (etm4_count++)
>> - return 0;
>> -
>> ret = cpu_pm_register_notifier(&etm4_cpu_pm_nb);
>> if (ret)
>> - goto reduce_count;
>> + return ret;
>>
>> - ret =
>> cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ARM_CORESIGHT_STARTING,
>> - "arm/coresight4:starting",
>> - etm4_starting_cpu, etm4_dying_cpu);
>> + ret = cpuhp_setup_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING,
>> + "arm/coresight4:starting",
>> + etm4_starting_cpu, etm4_dying_cpu);
>>
>> if (ret)
>> goto unregister_notifier;
>>
>> - ret = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ONLINE_DYN,
>> - "arm/coresight4:online",
>> - etm4_online_cpu, NULL);
>> + ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
>> + "arm/coresight4:online",
>> + etm4_online_cpu, NULL);
>>
>> /* HP dyn state ID returned in ret on success */
>> if (ret > 0) {
>> @@ -1428,21 +1423,15 @@ static int etm4_pm_setup_cpuslocked(void)
>> }
>>
>> /* failed dyn state - remove others */
>> - cpuhp_remove_state_nocalls_cpuslocked(CPUHP_AP_ARM_CORESIGHT_STARTING);
>> + cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
>>
>> unregister_notifier:
>> cpu_pm_unregister_notifier(&etm4_cpu_pm_nb);
>> -
>> -reduce_count:
>> - --etm4_count;
>> return ret;
>> }
>>
>> -static void etm4_pm_clear(void)
>> +static void __init etm4_pm_clear(void)
>> {
>> - if (--etm4_count != 0)
>> - return;
>> -
>> cpu_pm_unregister_notifier(&etm4_cpu_pm_nb);
>> cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
>> if (hp_online) {
>> @@ -1498,22 +1487,12 @@ static int etm4_probe(struct amba_device
>> *adev,
>> const struct amba_id *id)
>> if (!desc.name)
>> return -ENOMEM;
>>
>> - cpus_read_lock();
>> etmdrvdata[drvdata->cpu] = drvdata;
>>
>> if (smp_call_function_single(drvdata->cpu,
>> etm4_init_arch_data, drvdata, 1))
>> dev_err(dev, "ETM arch init failed\n");
>>
>> - ret = etm4_pm_setup_cpuslocked();
>> - cpus_read_unlock();
>> -
>> - /* etm4_pm_setup_cpuslocked() does its own cleanup - exit on error
>> */
>> - if (ret) {
>> - etmdrvdata[drvdata->cpu] = NULL;
>> - return ret;
>> - }
>> -
>> if (etm4_arch_supported(drvdata->arch) == false) {
>> ret = -EINVAL;
>> goto err_arch_supported;
>> @@ -1560,7 +1539,6 @@ static int etm4_probe(struct amba_device *adev,
>> const
>> struct amba_id *id)
>>
>> err_arch_supported:
>> etmdrvdata[drvdata->cpu] = NULL;
>> - etm4_pm_clear();
>> return ret;
>> }
>>
>> @@ -1598,4 +1576,23 @@ static struct amba_driver etm4x_driver = {
>> .probe = etm4_probe,
>> .id_table = etm4_ids,
>> };
>> -builtin_amba_driver(etm4x_driver);
>
>> +
>> +static int __init etm4x_init(void)
>> +{
>> + int ret;
>> +
>> + ret = etm4_pm_setup();
>
> etm4_pm_setup() is called in etm4x_init(), but etm4_pm_clear() is
> called
> in etm4_remove(). If one ETM4x device is removed, etm4_remove() would
> be
> called and we could still have other ETM4x device alive.
>
> Should etm4_pm_clear() be called in etm4x_exit()? It should only be
> called
> when module exits.
>
> Thanks,
> Tingwei
>
>> +
>> + /* etm4_pm_setup() does its own cleanup - exit on error */
>> + if (ret)
>> + return ret;
>> +
>> + ret = amba_driver_register(&etm4x_driver);
>> + if (ret) {
>> + pr_err("Error registering etm4x driver\n");
>> + etm4_pm_clear();
>> + }
>> +
>> + return ret;
>> +}
>> +device_initcall(etm4x_init);
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
>> member
>> of Code Aurora Forum, hosted by The Linux Foundation
>>
>> _______________________________________________
>> CoreSight mailing list
>> CoreSight@...ts.linaro.org
>> https://lists.linaro.org/mailman/listinfo/coresight
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Powered by blists - more mailing lists