Re: [PATCH net 1/2] net: sfc: ef10: fix memory leak in efx_ef10_mtd_probe()

[Martin Habets <habetsm.xilinx@gmail.com>]

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