netdev - Re: [PATCH net 1/2] i40e: fix potential memory leaks in i40e

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6372d2fd-e998-4664-848d-539d592a516c@amd.com>
Date: Tue, 5 Sep 2023 11:29:25 -0700
From: "Nelson, Shannon" <shannon.nelson@....com>
To: Tony Nguyen <anthony.l.nguyen@...el.com>, davem@...emloft.net,
 kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com,
 netdev@...r.kernel.org
Cc: Andrii Staikov <andrii.staikov@...el.com>,
 Aleksandr Loktionov <aleksandr.loktionov@...el.com>,
 Pucha Himasekhar Reddy <himasekharx.reddy.pucha@...el.com>
Subject: Re: [PATCH net 1/2] i40e: fix potential memory leaks in i40e_remove()

On 9/5/2023 11:05 AM, Tony Nguyen wrote:
> 
> From: Andrii Staikov <andrii.staikov@...el.com>
> 
> Instead of freeing memory of a single VSI, make sure
> the memory for all VSIs is cleared before releasing VSIs.
> Add releasing of their resources in a loop with the iteration
> number equal to the number of allocated VSIs.
> 
> Fixes: 41c445ff0f48 ("i40e: main driver core")
> Signed-off-by: Andrii Staikov <andrii.staikov@...el.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
> Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@...el.com> (A Contingent worker at Intel)
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
> ---
>   drivers/net/ethernet/intel/i40e/i40e_main.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index de7fd43dc11c..059db43f8643 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -16323,8 +16323,14 @@ static void i40e_remove(struct pci_dev *pdev)
>          /* Now we can shutdown the PF's VSI, just before we kill

Since we're looping over multiple VSIs, this comment should be updated 
to be plural.

>           * adminq and hmc.
>           */
> -       if (pf->vsi[pf->lan_vsi])
> -               i40e_vsi_release(pf->vsi[pf->lan_vsi]);
> +       for (i = pf->num_alloc_vsi; i--;)
> +               if (pf->vsi[i]) {
> +                       i40e_vsi_close(pf->vsi[i]);
> +                       i40e_vsi_free_q_vectors(pf->vsi[i]);
> +                       i40e_vsi_release(pf->vsi[i]);
> +                       kfree(pf->vsi[i]);
> +                       pf->vsi[i] = NULL;
> +               }

Looping over all the VSIs makes some sense now that there are multiple 
VSIs being used by the PF, but why all the extra calls?  It seems to me 
that i40e_vsi_release() already takes care of all this except for the 
"pf->vsi[i] = NULL".

sln

> 
>          i40e_cloud_filter_exit(pf);
> 
> --
> 2.38.1
>