* [PATCH net 0/2] net: sfc: fix memory leak in ->mtd_probe() callback
@ 2022-05-10 15:36 Taehee Yoo
2022-05-10 15:36 ` [PATCH net 1/2] net: sfc: ef10: fix memory leak in efx_ef10_mtd_probe() Taehee Yoo
2022-05-10 15:36 ` [PATCH net 2/2] net: sfc: siena: fix memory leak in siena_mtd_probe() Taehee Yoo
0 siblings, 2 replies; 6+ messages in thread
From: Taehee Yoo @ 2022-05-10 15:36 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, ecree.xilinx, habetsm.xilinx, netdev
Cc: ap420073
This patchset fixes memory leak in ->mtd_probe() callback.
The goal of ->mtd_probe() callback is to allocate and initialize
mtd partition and this is global data of sfc driver.
If NIC has 2 ports, ->mtd_probe() callback will be called twice.
So it allocates mtd partition twice and last allocated mtd partition data
will not be used.
So it should be freed, but it doesn't.
This patchset contains patches for ef10 and siena device.
But I tested only ef10(X2522-25G).
Siena version of ->mtd_probe() callback is similar to ef10 version.
So I added it.
But falcon version of ->mtd_probe() code looks different.
So I didn't add.
Taehee Yoo (2):
net: sfc: ef10: fix memory leak in efx_ef10_mtd_probe()
net: sfc: siena: fix memory leak in siena_mtd_probe()
drivers/net/ethernet/sfc/ef10.c | 5 +++++
drivers/net/ethernet/sfc/siena.c | 5 +++++
2 files changed, 10 insertions(+)
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net 1/2] net: sfc: ef10: fix memory leak in efx_ef10_mtd_probe()
2022-05-10 15:36 [PATCH net 0/2] net: sfc: fix memory leak in ->mtd_probe() callback Taehee Yoo
@ 2022-05-10 15:36 ` Taehee Yoo
2022-05-11 6:09 ` Martin Habets
2022-05-10 15:36 ` [PATCH net 2/2] net: sfc: siena: fix memory leak in siena_mtd_probe() Taehee Yoo
1 sibling, 1 reply; 6+ messages in thread
From: Taehee Yoo @ 2022-05-10 15:36 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, ecree.xilinx, habetsm.xilinx, netdev
Cc: ap420073
In the NIC ->probe() callback, ->mtd_probe() callback is called.
If NIC has 2 ports, ->probe() is called twice and ->mtd_probe() too.
In the ->mtd_probe(), which is efx_ef10_mtd_probe() it allocates and
initializes mtd partiion.
But mtd partition for sfc is shared data.
So that allocated mtd partition data from last called
efx_ef10_mtd_probe() will not be used.
Therefore it must be freed.
But it doesn't free a not used mtd partition data in efx_ef10_mtd_probe().
kmemleak reports:
unreferenced object 0xffff88811ddb0000 (size 63168):
comm "systemd-udevd", pid 265, jiffies 4294681048 (age 348.586s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffffa3767749>] kmalloc_order_trace+0x19/0x120
[<ffffffffa3873f0e>] __kmalloc+0x20e/0x250
[<ffffffffc041389f>] efx_ef10_mtd_probe+0x11f/0x270 [sfc]
[<ffffffffc0484c8a>] efx_pci_probe.cold.17+0x3df/0x53d [sfc]
[<ffffffffa414192c>] local_pci_probe+0xdc/0x170
[<ffffffffa4145df5>] pci_device_probe+0x235/0x680
[<ffffffffa443dd52>] really_probe+0x1c2/0x8f0
[<ffffffffa443e72b>] __driver_probe_device+0x2ab/0x460
[<ffffffffa443e92a>] driver_probe_device+0x4a/0x120
[<ffffffffa443f2ae>] __driver_attach+0x16e/0x320
[<ffffffffa4437a90>] bus_for_each_dev+0x110/0x190
[<ffffffffa443b75e>] bus_add_driver+0x39e/0x560
[<ffffffffa4440b1e>] driver_register+0x18e/0x310
[<ffffffffc02e2055>] 0xffffffffc02e2055
[<ffffffffa3001af3>] do_one_initcall+0xc3/0x450
[<ffffffffa33ca574>] do_init_module+0x1b4/0x700
Fixes: 8127d661e77f ("sfc: Add support for Solarflare SFC9100 family")
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
drivers/net/ethernet/sfc/ef10.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
index 50d535981a35..f8edb3f1b73a 100644
--- a/drivers/net/ethernet/sfc/ef10.c
+++ b/drivers/net/ethernet/sfc/ef10.c
@@ -3579,6 +3579,11 @@ static int efx_ef10_mtd_probe(struct efx_nic *efx)
n_parts++;
}
+ if (!n_parts) {
+ kfree(parts);
+ return 0;
+ }
+
rc = efx_mtd_add(efx, &parts[0].common, n_parts, sizeof(*parts));
fail:
if (rc)
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net 1/2] net: sfc: ef10: fix memory leak in efx_ef10_mtd_probe()
2022-05-10 15:36 ` [PATCH net 1/2] net: sfc: ef10: fix memory leak in efx_ef10_mtd_probe() Taehee Yoo
@ 2022-05-11 6:09 ` Martin Habets
0 siblings, 0 replies; 6+ messages in thread
From: Martin Habets @ 2022-05-11 6:09 UTC (permalink / raw)
To: Taehee Yoo; +Cc: davem, kuba, pabeni, edumazet, ecree.xilinx, netdev
Hi Taehee,
On Tue, May 10, 2022 at 03:36:18PM +0000, Taehee Yoo wrote:
> In the NIC ->probe() callback, ->mtd_probe() callback is called.
> If NIC has 2 ports, ->probe() is called twice and ->mtd_probe() too.
> In the ->mtd_probe(), which is efx_ef10_mtd_probe() it allocates and
> initializes mtd partiion.
> But mtd partition for sfc is shared data.
> So that allocated mtd partition data from last called
> efx_ef10_mtd_probe() will not be used.
> Therefore it must be freed.
> But it doesn't free a not used mtd partition data in efx_ef10_mtd_probe().
Your analysis is correct for X2, where the 2nd port does not have
any MTD partitions.
Note that on 7000 series the 2nd (and possibly 3rd and 4th) port do have
MTD partitions. On those efx_mtd_remove() will free the data.
On X2 efx_mtd_remove() returns too early for the 2nd port due to:
if (list_empty(&efx->mtd_list))
return;
I don't see an easy way to fix that code, since it does not know
the memory address. So your fix is the best we can do.
>
> kmemleak reports:
> unreferenced object 0xffff88811ddb0000 (size 63168):
> comm "systemd-udevd", pid 265, jiffies 4294681048 (age 348.586s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<ffffffffa3767749>] kmalloc_order_trace+0x19/0x120
> [<ffffffffa3873f0e>] __kmalloc+0x20e/0x250
> [<ffffffffc041389f>] efx_ef10_mtd_probe+0x11f/0x270 [sfc]
> [<ffffffffc0484c8a>] efx_pci_probe.cold.17+0x3df/0x53d [sfc]
> [<ffffffffa414192c>] local_pci_probe+0xdc/0x170
> [<ffffffffa4145df5>] pci_device_probe+0x235/0x680
> [<ffffffffa443dd52>] really_probe+0x1c2/0x8f0
> [<ffffffffa443e72b>] __driver_probe_device+0x2ab/0x460
> [<ffffffffa443e92a>] driver_probe_device+0x4a/0x120
> [<ffffffffa443f2ae>] __driver_attach+0x16e/0x320
> [<ffffffffa4437a90>] bus_for_each_dev+0x110/0x190
> [<ffffffffa443b75e>] bus_add_driver+0x39e/0x560
> [<ffffffffa4440b1e>] driver_register+0x18e/0x310
> [<ffffffffc02e2055>] 0xffffffffc02e2055
> [<ffffffffa3001af3>] do_one_initcall+0xc3/0x450
> [<ffffffffa33ca574>] do_init_module+0x1b4/0x700
>
> Fixes: 8127d661e77f ("sfc: Add support for Solarflare SFC9100 family")
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
Acked-by: Martin Habets <habetsm.xilinx@gmail.com>
> ---
> drivers/net/ethernet/sfc/ef10.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
> index 50d535981a35..f8edb3f1b73a 100644
> --- a/drivers/net/ethernet/sfc/ef10.c
> +++ b/drivers/net/ethernet/sfc/ef10.c
> @@ -3579,6 +3579,11 @@ static int efx_ef10_mtd_probe(struct efx_nic *efx)
> n_parts++;
> }
>
> + if (!n_parts) {
> + kfree(parts);
> + return 0;
> + }
> +
> rc = efx_mtd_add(efx, &parts[0].common, n_parts, sizeof(*parts));
> fail:
> if (rc)
> --
> 2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net 2/2] net: sfc: siena: fix memory leak in siena_mtd_probe()
2022-05-10 15:36 [PATCH net 0/2] net: sfc: fix memory leak in ->mtd_probe() callback Taehee Yoo
2022-05-10 15:36 ` [PATCH net 1/2] net: sfc: ef10: fix memory leak in efx_ef10_mtd_probe() Taehee Yoo
@ 2022-05-10 15:36 ` Taehee Yoo
2022-05-11 6:25 ` Martin Habets
1 sibling, 1 reply; 6+ messages in thread
From: Taehee Yoo @ 2022-05-10 15:36 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, ecree.xilinx, habetsm.xilinx, netdev
Cc: ap420073
In the NIC ->probe callback, ->mtd_probe() callback is called.
If NIC has 2 ports, ->probe() is called twice and ->mtd_probe() too.
In the ->mtd_probe(), which is siena_mtd_probe() it allocates and
initializes mtd partiion.
But mtd partition for sfc is shared data.
So that allocated mtd partition data from last called
siena_mtd_probe() will not be used.
Therefore it must be freed.
But it doesn't free a not used mtd partition data in siena_mtd_probe().
Fixes: 8880f4ec21e6 ("sfc: Add support for SFC9000 family (2)")
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
drivers/net/ethernet/sfc/siena.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/ethernet/sfc/siena.c b/drivers/net/ethernet/sfc/siena.c
index ce3060e15b54..8b42951e34d6 100644
--- a/drivers/net/ethernet/sfc/siena.c
+++ b/drivers/net/ethernet/sfc/siena.c
@@ -939,6 +939,11 @@ static int siena_mtd_probe(struct efx_nic *efx)
nvram_types >>= 1;
}
+ if (!n_parts) {
+ kfree(parts);
+ return 0;
+ }
+
rc = siena_mtd_get_fw_subtypes(efx, parts, n_parts);
if (rc)
goto fail;
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net 2/2] net: sfc: siena: fix memory leak in siena_mtd_probe()
2022-05-10 15:36 ` [PATCH net 2/2] net: sfc: siena: fix memory leak in siena_mtd_probe() Taehee Yoo
@ 2022-05-11 6:25 ` Martin Habets
2022-05-11 8:14 ` Taehee Yoo
0 siblings, 1 reply; 6+ messages in thread
From: Martin Habets @ 2022-05-11 6:25 UTC (permalink / raw)
To: Taehee Yoo; +Cc: davem, kuba, pabeni, edumazet, ecree.xilinx, netdev
On Tue, May 10, 2022 at 03:36:19PM +0000, Taehee Yoo wrote:
> In the NIC ->probe callback, ->mtd_probe() callback is called.
> If NIC has 2 ports, ->probe() is called twice and ->mtd_probe() too.
> In the ->mtd_probe(), which is siena_mtd_probe() it allocates and
> initializes mtd partiion.
> But mtd partition for sfc is shared data.
> So that allocated mtd partition data from last called
> siena_mtd_probe() will not be used.
On Siena the 2nd port does have MTD partitions. In the output
from /proc/mtd below eth3 is the 1st port and eth4 is the 2nd
port:
mtd12: 00030000 00010000 "eth3 sfc_mcfw:0b"
mtd13: 00010000 00010000 "eth3 sfc_dynamic_cfg:00"
mtd14: 00030000 00010000 "eth3 sfc_exp_rom:01"
mtd15: 00010000 00010000 "eth3 sfc_exp_rom_cfg:00"
mtd16: 00120000 00010000 "eth3 sfc_fpga:01"
mtd17: 00010000 00010000 "eth4 sfc_dynamic_cfg:00"
mtd18: 00010000 00010000 "eth4 sfc_exp_rom_cfg:00"
So this patch is not needed, and efx_mtd_remove() will free
the memory for both ports.
Martin
> Therefore it must be freed.
> But it doesn't free a not used mtd partition data in siena_mtd_probe().
>
> Fixes: 8880f4ec21e6 ("sfc: Add support for SFC9000 family (2)")
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> ---
> drivers/net/ethernet/sfc/siena.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/ethernet/sfc/siena.c b/drivers/net/ethernet/sfc/siena.c
> index ce3060e15b54..8b42951e34d6 100644
> --- a/drivers/net/ethernet/sfc/siena.c
> +++ b/drivers/net/ethernet/sfc/siena.c
> @@ -939,6 +939,11 @@ static int siena_mtd_probe(struct efx_nic *efx)
> nvram_types >>= 1;
> }
>
> + if (!n_parts) {
> + kfree(parts);
> + return 0;
> + }
> +
> rc = siena_mtd_get_fw_subtypes(efx, parts, n_parts);
> if (rc)
> goto fail;
> --
> 2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net 2/2] net: sfc: siena: fix memory leak in siena_mtd_probe()
2022-05-11 6:25 ` Martin Habets
@ 2022-05-11 8:14 ` Taehee Yoo
0 siblings, 0 replies; 6+ messages in thread
From: Taehee Yoo @ 2022-05-11 8:14 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, ecree.xilinx, netdev
2022. 5. 11. 오후 3:25에 Martin Habets 이(가) 쓴 글:
Hi Martin,
Thanks a lot for your review!
> On Tue, May 10, 2022 at 03:36:19PM +0000, Taehee Yoo wrote:
>> In the NIC ->probe callback, ->mtd_probe() callback is called.
>> If NIC has 2 ports, ->probe() is called twice and ->mtd_probe() too.
>> In the ->mtd_probe(), which is siena_mtd_probe() it allocates and
>> initializes mtd partiion.
>> But mtd partition for sfc is shared data.
>> So that allocated mtd partition data from last called
>> siena_mtd_probe() will not be used.
>
> On Siena the 2nd port does have MTD partitions. In the output
> from /proc/mtd below eth3 is the 1st port and eth4 is the 2nd
> port:
>
> mtd12: 00030000 00010000 "eth3 sfc_mcfw:0b"
> mtd13: 00010000 00010000 "eth3 sfc_dynamic_cfg:00"
> mtd14: 00030000 00010000 "eth3 sfc_exp_rom:01"
> mtd15: 00010000 00010000 "eth3 sfc_exp_rom_cfg:00"
> mtd16: 00120000 00010000 "eth3 sfc_fpga:01"
> mtd17: 00010000 00010000 "eth4 sfc_dynamic_cfg:00"
> mtd18: 00010000 00010000 "eth4 sfc_exp_rom_cfg:00"
>
> So this patch is not needed, and efx_mtd_remove() will free
> the memory for both ports.
>
Okay, I will send a v2 patch tomorrow, that will drop this patch and
unnecessary cover-letter.
Thanks!
Taehee Yoo
> Martin
>
>> Therefore it must be freed.
>> But it doesn't free a not used mtd partition data in siena_mtd_probe().
>>
>> Fixes: 8880f4ec21e6 ("sfc: Add support for SFC9000 family (2)")
>> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
>> ---
>> drivers/net/ethernet/sfc/siena.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/sfc/siena.c
b/drivers/net/ethernet/sfc/siena.c
>> index ce3060e15b54..8b42951e34d6 100644
>> --- a/drivers/net/ethernet/sfc/siena.c
>> +++ b/drivers/net/ethernet/sfc/siena.c
>> @@ -939,6 +939,11 @@ static int siena_mtd_probe(struct efx_nic *efx)
>> nvram_types >>= 1;
>> }
>>
>> + if (!n_parts) {
>> + kfree(parts);
>> + return 0;
>> + }
>> +
>> rc = siena_mtd_get_fw_subtypes(efx, parts, n_parts);
>> if (rc)
>> goto fail;
>> --
>> 2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-05-11 8:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-10 15:36 [PATCH net 0/2] net: sfc: fix memory leak in ->mtd_probe() callback Taehee Yoo
2022-05-10 15:36 ` [PATCH net 1/2] net: sfc: ef10: fix memory leak in efx_ef10_mtd_probe() Taehee Yoo
2022-05-11 6:09 ` Martin Habets
2022-05-10 15:36 ` [PATCH net 2/2] net: sfc: siena: fix memory leak in siena_mtd_probe() Taehee Yoo
2022-05-11 6:25 ` Martin Habets
2022-05-11 8:14 ` Taehee Yoo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).