| Message ID | 20221206092613.122952-1-yuancan@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | intel/i40e: Fix potential memory leak in i40e_init_recovery_mode() | expand |
On Tue, Dec 06, 2022 at 09:26:13AM +0000, Yuan Can wrote: > If i40e_vsi_mem_alloc() failed in i40e_init_recovery_mode(), the pf will be > freed with the pf->vsi leaked. > Fix by free pf->vsi in the error handling path. > > Fixes: 4ff0ee1af016 ("i40e: Introduce recovery mode support") > Signed-off-by: Yuan Can <yuancan@huawei.com> > --- > drivers/net/ethernet/intel/i40e/i40e_main.c | 1 + > 1 file changed, 1 insertion(+) The patch title needs to be "[PATCH net]..." > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c > index b5dcd15ced36..d23081c224d6 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c > @@ -15536,6 +15536,7 @@ static int i40e_init_recovery_mode(struct i40e_pf *pf, struct i40e_hw *hw) > pci_disable_pcie_error_reporting(pf->pdev); > pci_release_mem_regions(pf->pdev); > pci_disable_device(pf->pdev); > + kfree(pf->vsi); > kfree(pf); > > return err; The change is ok, but it is worth to cleanup error flow of i40e_probe and i40e_remove as they are not really in the same order. Thanks, Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
在 2022/12/6 18:02, Leon Romanovsky 写道: > On Tue, Dec 06, 2022 at 09:26:13AM +0000, Yuan Can wrote: >> If i40e_vsi_mem_alloc() failed in i40e_init_recovery_mode(), the pf will be >> freed with the pf->vsi leaked. >> Fix by free pf->vsi in the error handling path. >> >> Fixes: 4ff0ee1af016 ("i40e: Introduce recovery mode support") >> Signed-off-by: Yuan Can <yuancan@huawei.com> >> --- >> drivers/net/ethernet/intel/i40e/i40e_main.c | 1 + >> 1 file changed, 1 insertion(+) > The patch title needs to be "[PATCH net]..." > >> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c >> index b5dcd15ced36..d23081c224d6 100644 >> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c >> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c >> @@ -15536,6 +15536,7 @@ static int i40e_init_recovery_mode(struct i40e_pf *pf, struct i40e_hw *hw) >> pci_disable_pcie_error_reporting(pf->pdev); >> pci_release_mem_regions(pf->pdev); >> pci_disable_device(pf->pdev); >> + kfree(pf->vsi); >> kfree(pf); >> >> return err; > The change is ok, but it is worth to cleanup error flow of i40e_probe and i40e_remove > as they are not really in the same order. > > Thanks, > Reviewed-by: Leon Romanovsky <leonro@nvidia.com> Thanks for the review, the title problem will be fixed in the next version.
Tue, Dec 06, 2022 at 10:26:13AM CET, yuancan@huawei.com wrote: >If i40e_vsi_mem_alloc() failed in i40e_init_recovery_mode(), the pf will be >freed with the pf->vsi leaked. >Fix by free pf->vsi in the error handling path. > >Fixes: 4ff0ee1af016 ("i40e: Introduce recovery mode support") >Signed-off-by: Yuan Can <yuancan@huawei.com> >--- > drivers/net/ethernet/intel/i40e/i40e_main.c | 1 + > 1 file changed, 1 insertion(+) > >diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c >index b5dcd15ced36..d23081c224d6 100644 >--- a/drivers/net/ethernet/intel/i40e/i40e_main.c >+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c >@@ -15536,6 +15536,7 @@ static int i40e_init_recovery_mode(struct i40e_pf *pf, struct i40e_hw *hw) > pci_disable_pcie_error_reporting(pf->pdev); > pci_release_mem_regions(pf->pdev); > pci_disable_device(pf->pdev); >+ kfree(pf->vsi); This is not the only thing which is wrong on this error path. Just quickly looking at the code: - kfree(pf->qp_pile); should be called here as well - if i40e_setup_misc_vector_for_recovery_mode() fails, unregister_netdev() needs to be called. - if register_netdev() fails, netdev needs to be freed at least. Basically the whole error path is completely wrong. I suggest to: 1) rely on error path of i40e_probe() when call of i40e_init_recovery_mode() fails and don't do the cleanup of previously inited things in i40e_probe() locally. 2) implement a proper local error path in i40e_init_recovery_mode() > kfree(pf); > > return err; >-- >2.17.1 >
在 2022/12/6 18:28, Jiri Pirko 写道: > Tue, Dec 06, 2022 at 10:26:13AM CET, yuancan@huawei.com wrote: >> If i40e_vsi_mem_alloc() failed in i40e_init_recovery_mode(), the pf will be >> freed with the pf->vsi leaked. >> Fix by free pf->vsi in the error handling path. >> >> Fixes: 4ff0ee1af016 ("i40e: Introduce recovery mode support") >> Signed-off-by: Yuan Can <yuancan@huawei.com> >> --- >> drivers/net/ethernet/intel/i40e/i40e_main.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c >> index b5dcd15ced36..d23081c224d6 100644 >> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c >> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c >> @@ -15536,6 +15536,7 @@ static int i40e_init_recovery_mode(struct i40e_pf *pf, struct i40e_hw *hw) >> pci_disable_pcie_error_reporting(pf->pdev); >> pci_release_mem_regions(pf->pdev); >> pci_disable_device(pf->pdev); >> + kfree(pf->vsi); > This is not the only thing which is wrong on this error path. Just > quickly looking at the code: > - kfree(pf->qp_pile); should be called here as well > - if i40e_setup_misc_vector_for_recovery_mode() fails, > unregister_netdev() needs to be called. > - if register_netdev() fails, netdev needs to be freed at least. > Basically the whole error path is completely wrong. > I suggest to: > > 1) rely on error path of i40e_probe() when call of > i40e_init_recovery_mode() fails and don't do the cleanup of > previously inited things in i40e_probe() locally. > 2) implement a proper local error path in i40e_init_recovery_mode() Thanks for this, I will try to make everything correct in the next version.
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c index b5dcd15ced36..d23081c224d6 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c @@ -15536,6 +15536,7 @@ static int i40e_init_recovery_mode(struct i40e_pf *pf, struct i40e_hw *hw) pci_disable_pcie_error_reporting(pf->pdev); pci_release_mem_regions(pf->pdev); pci_disable_device(pf->pdev); + kfree(pf->vsi); kfree(pf); return err;
If i40e_vsi_mem_alloc() failed in i40e_init_recovery_mode(), the pf will be freed with the pf->vsi leaked. Fix by free pf->vsi in the error handling path. Fixes: 4ff0ee1af016 ("i40e: Introduce recovery mode support") Signed-off-by: Yuan Can <yuancan@huawei.com> --- drivers/net/ethernet/intel/i40e/i40e_main.c | 1 + 1 file changed, 1 insertion(+)