| 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: <4bb59de4-0fab-99f3-41d0-607d5085df05@arm.com>
Date: Wed, 16 Aug 2023 14:58:57 +0100
From: Suzuki K Poulose <suzuki.poulose@....com>
To: hejunhao <hejunhao3@...wei.com>, mike.leach@...aro.org,
leo.yan@...aro.org, anshuman.khandual@....com,
jonathan.cameron@...wei.com, James Clark <james.clark@....com>
Cc: coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, linuxarm@...wei.com,
yangyicong@...wei.com, prime.zeng@...ilicon.com
Subject: Re: [PATCH 2/2] coresight: core: Fix multiple free TRBE platform data
resource
On 16/08/2023 14:13, Suzuki K Poulose wrote:
> On 15/08/2023 12:38, hejunhao wrote:
>> Hi, Suzuki
>>
>>
>> On 2023/8/15 6:47, Suzuki K Poulose wrote:
>>> + James Clark
>>>
>>> On 14/08/2023 10:38, Junhao He wrote:
>>>> Current the TRBE driver supports matching TRBE platform device through
>>>> id_table. The ACPI created a dummy TRBE platform device inside
>>>> drivers/perf/arm_pmu_acpi.c. So the TRBE platform driver will probe
>>>> only
>>>> once and allocate just one TRBE platform data resource.
>>>>
>>>> If the system supports the TRBE feature, Each CPU in the systems can
>>>> have at least one TRBE present, and the coresight_unregister gets
>>>> called
>>>> multiple times, once for each of them.
>>>> Therefore, when unregister TRBE coresight devices, the TRBE platform
>>>> data
>>>> resource will multiple free in function coresight_unregister.
>>>>
>>>> root@...alhost:# insmod coresight-trbe.ko
>>>> root@...alhost:# rmmod coresight-trbe.ko
>>>> [ 423.455932] ------------[ cut here ]------------
>>>> [ 423.461987] WARNING: CPU: 1 PID: 0 at drivers/base/devres.c:1064
>>>> devm_kfree+0x88/0x98
>>>> [ 423.483821] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G O 6.5.0-rc4+ #1
>>>> [ 423.505842] pstate: 614000c9 (nZCv daIF +PAN -UAO -TCO +DIT -SSBS
>>>> BTYPE=--)
>>>> ...
>>>> [ 423.601301] Call trace:
>>>> [ 423.604202] devm_kfree+0x88/0x98
>>>> [ 423.608369] coresight_release_platform_data+0xb8/0xe0 [coresight]
>>>> [ 423.616589] coresight_unregister+0x120/0x170 [coresight]
>>>> [ 423.623533] arm_trbe_remove_coresight_cpu+0x70/0xa0
>>>> [coresight_trbe]
>>>> [ 423.631082] __flush_smp_call_function_queue+0x1e4/0x4e0
>>>> [ 423.637471] generic_smp_call_function_single_interrupt+0x1c/0x30
>>>> [ 423.644796] ipi_handler+0x90/0x278
>>>> [ 423.648992] handle_percpu_devid_irq+0x90/0x250
>>>> [ 423.654636] generic_handle_domain_irq+0x34/0x58
>>>> [ 423.659786] gic_handle_irq+0x12c/0x270
>>>> [ 423.664039] call_on_irq_stack+0x24/0x30
>>>> [ 423.668452] do_interrupt_handler+0x88/0x98
>>>> [ 423.673027] el1_interrupt+0x48/0xe8
>>>> [ 423.677413] el1h_64_irq_handler+0x18/0x28
>>>> [ 423.681781] el1h_64_irq+0x78/0x80
>>>> [ 423.685550] default_idle_call+0x5c/0x180
>>>> [ 423.689855] do_idle+0x25c/0x2c0
>>>> [ 423.694196] cpu_startup_entry+0x2c/0x40
>>>> [ 423.698373] secondary_start_kernel+0x144/0x188
>>>> [ 423.703920] __secondary_switched+0xb8/0xc0
>>>> [ 423.708972] ---[ end trace 0000000000000000 ]---
>>>> [ 423.729209] ------------[ cut here ]------------
>>>> ...
>>>> [ 423.735217] WARNING: CPU: 2 PID: 40 at drivers/base/devres.c:1064
>>>> devm_kfree+0x88/0x98
>>>> ...
>>>> [ 424.012385] WARNING: CPU: 3 PID: 0 at drivers/base/devres.c:1064
>>>> devm_kfree+0x88/0x98
>>>> ...
>>>>
>>>> This patch does the following:
>>>> 1.TRBE coresight devices do not need regular connections
>>>> information, We
>>>> can free connections resource when the nr_conns is valid.
>>>> 2.And we can ignore the free platform data resource, it will be
>>>> automatically free in platform_driver_unregister().
>>>
>>> Do we need a Fixes tag here ?
>>
>> Yes, I will do that.
>>
>>>>
>>>> Signed-off-by: Junhao He <hejunhao3@...wei.com>
>>>> ---
>>>> drivers/hwtracing/coresight/coresight-core.c | 7 ++++---
>>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-core.c
>>>> b/drivers/hwtracing/coresight/coresight-core.c
>>>> index 118fcf27854d..c6f7889d1b4d 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-core.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>>>> @@ -1555,9 +1555,10 @@ void coresight_release_platform_data(struct
>>>> coresight_device *csdev,
>>>> conns[i]->dest_fwnode = NULL;
>>>> devm_kfree(dev, conns[i]);
>>>> }
>>>> - devm_kfree(dev, pdata->out_conns);
>>>> - devm_kfree(dev, pdata->in_conns);
>>>> - devm_kfree(dev, pdata);
>>>> + if (pdata->nr_outconns)
>>>> + devm_kfree(dev, pdata->out_conns);
>>>> + if (pdata->nr_inconns)
>>>> + devm_kfree(dev, pdata->in_conns);
>>>
>>> These allocations are made on the parent device and that
>>> may never get unregistered (e.g., AMBA device, platform device,
>>> stay forever, even when the "coresight" modules are unloaded).
>>> Thus the memory will be left unused, literally leaking.
>>> This specific devm_kfree() was added to fix that. May be we should fix
>>> this in the TRBE driver to use separate pdata for the TRBE device
>>> instances.
>>>
>>> Suzuki
>>
>> If we fix this with minimal changes, I think it is possible to add a
>> check
>> and not free pdata if it is TRBE?
>>
>> if (csdev->subtype.sink_subtype !=
>> CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM)
>> devm_kfree(dev, pdata);
>>
>> Then free pdata in the end of arm_trbe_remove_coresight().
>
> It is much nicer to do something like :
>
>
> --8>--
>
>
> coresight: trbe: Allocate platform data per device
>
> Coresight TRBE driver shares a single platform data (which is empty btw).
> However, with the commit 4e8fe7e5c3a5
> ("coresight: Store pointers to connections rather than an array of them")
> the coresight core would free up the pdata, resulting in multiple attempts
> to free the same pdata for TRBE instances. Fix this by allocating a
> pdata per
> coresight_device.
>
> Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")
> Reported-by: Junhao He <hejunhao3@...wei.com>
> Cc: Anshuman Khandual <anshuman.khandual@....com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@....com>
> ---
> drivers/hwtracing/coresight/coresight-trbe.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c
> b/drivers/hwtracing/coresight/coresight-trbe.c
> index 025f70adee47..fbab2bb4db38 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -1255,10 +1255,17 @@ static void
> arm_trbe_register_coresight_cpu(struct trbe_drvdata *drvdata, int cp
> if (!desc.name)
> goto cpu_clear;
---8>--
>
> + /*
> + * TRBE doesn't have any connections described via firmware. Instead
> + * we register the trbe instance as per-cpu sink.
> + */
Ignore the comments above ^. I have tested this locally and works for
me. Please could you confirm if this solves the problem for you ?
Kind regards
Suzuki
> + desc.pdata = coresight_get_platform_data(dev);
> + if (IS_ERR(desc.pdata))
> + goto cpu_clear;
> +
> desc.type = CORESIGHT_DEV_TYPE_SINK;
> desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM;
> desc.ops = &arm_trbe_cs_ops;
> - desc.pdata = dev_get_platdata(dev);
> desc.groups = arm_trbe_groups;
> desc.dev = dev;
> trbe_csdev = coresight_register(&desc);
> @@ -1482,7 +1489,6 @@ static void arm_trbe_remove_irq(struct
> trbe_drvdata *drvdata)
>
> static int arm_trbe_device_probe(struct platform_device *pdev)
> {
> - struct coresight_platform_data *pdata;
> struct trbe_drvdata *drvdata;
> struct device *dev = &pdev->dev;
> int ret;
> @@ -1497,12 +1503,7 @@ static int arm_trbe_device_probe(struct
> platform_device *pdev)
> if (!drvdata)
> return -ENOMEM;
>
> - pdata = coresight_get_platform_data(dev);
> - if (IS_ERR(pdata))
> - return PTR_ERR(pdata);
> -
> dev_set_drvdata(dev, drvdata);
> - dev->platform_data = pdata;
> drvdata->pdev = pdev;
> ret = arm_trbe_probe_irq(pdev, drvdata);
> if (ret)
Powered by blists - more mailing lists