Re: [PATCH net v3] intel/i40e: Fix potential memory leak in i40e_init_recovery_mode()

[Jiri Pirko <jiri@resnulli.us>]

Tue, Dec 06, 2022 at 02:41:46PM CET, yuancan@huawei.com wrote:
>The error handling path of i40e_init_recovery_mode() does not handle the
>vsi->netdev and pf->vsi, and resource leak can happen if error occurs.
>
>In the meantime, the i40e_probe() returns directly without release
>pf->qp_pile when i40e_init_recovery_mode() failed.
>
>Fix by properly releasing vsi->netdev in the error handling path of
>i40e_init_recovery_mode() and relying on the error handling path of
>i40e_probe() to release pf->vsi and pf->qp_pile if anything goes wrong.
>
>Fixes: 4ff0ee1af016 ("i40e: Introduce recovery mode support")
>Signed-off-by: Yuan Can <yuancan@huawei.com>
>---
>Changes in v3:
>- Introduce more error handling path to handle vsi->netdev
>- Rely on error path of i40e_probe() instead of do all cleanup in
>  i40e_init_recovery_mode() to make sure pf->qp_pile is not leaked
>
>Changes in v2:
>- Add net in patch title
>- Add Leon Romanovsky's reviewed by
>
> drivers/net/ethernet/intel/i40e/i40e_main.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
>index b5dcd15ced36..d1aadd298ea7 100644
>--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
>+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
>@@ -15511,13 +15511,13 @@ static int i40e_init_recovery_mode(struct i40e_pf *pf, struct i40e_hw *hw)
> 		goto err_switch_setup;
> 	err = register_netdev(vsi->netdev);
> 	if (err)
>-		goto err_switch_setup;
>+		goto free_netdev;
> 	vsi->netdev_registered = true;
> 	i40e_dbg_pf_init(pf);
> 
> 	err = i40e_setup_misc_vector_for_recovery_mode(pf);
> 	if (err)
>-		goto err_switch_setup;
>+		goto unreg_netdev;
> 
> 	/* tell the firmware that we're starting */
> 	i40e_send_version(pf);
>@@ -15528,15 +15528,15 @@ static int i40e_init_recovery_mode(struct i40e_pf *pf, struct i40e_hw *hw)
> 
> 	return 0;
> 
>+unreg_netdev:
>+	unregister_netdev(vsi->netdev);
>+free_netdev:
>+	free_netdev(vsi->netdev);

[...]

> err_switch_setup:
> 	i40e_reset_interrupt_capability(pf);
> 	del_timer_sync(&pf->service_timer);
> 	i40e_shutdown_adminq(hw);

These are on a error patch in i40e_probe(). Again, you should cleanup
here only what you initialized.


>-	iounmap(hw->hw_addr);
>-	pci_disable_pcie_error_reporting(pf->pdev);
>-	pci_release_mem_regions(pf->pdev);
>-	pci_disable_device(pf->pdev);
>-	kfree(pf);
>+	kfree(pf->vsi);
> 
> 	return err;
> }
>@@ -15789,8 +15789,11 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> 		goto err_sw_init;
> 	}
> 
>-	if (test_bit(__I40E_RECOVERY_MODE, pf->state))
>-		return i40e_init_recovery_mode(pf, hw);
>+	if (test_bit(__I40E_RECOVERY_MODE, pf->state)) {
>+		err = i40e_init_recovery_mode(pf, hw);
>+		if (err)
>+			goto err_init_lan_hmc;

Use a new label here.


Also, you need to return 0 here in case of success. Did you test the
patch?





>+	}
> 
> 	err = i40e_init_lan_hmc(hw, hw->func_caps.num_tx_qp,
> 				hw->func_caps.num_rx_qp, 0, 0);
>-- 
>2.17.1
>