netdev - Re: [PATCH net] ice: fix memory leak of aRFS after resuming from suspend

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]

Date:   Thu, 18 Mar 2021 22:20:25 +0000
From:   "Creeley, Brett" <brett.creeley@...el.com>
To:     "yongxin.liu@...driver.com" <yongxin.liu@...driver.com>,
        "jeffrey.t.kirsher@...el.com" <jeffrey.t.kirsher@...el.com>,
        "Chittim, Madhu" <madhu.chittim@...el.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
        "andrewx.bowers@...el.com" <andrewx.bowers@...el.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net] ice: fix memory leak of aRFS after resuming from
 suspend

On Thu, 2021-03-18 at 16:15 +0800, Yongxin Liu wrote:
> In ice_suspend(), ice_clear_interrupt_scheme() is called, and then
> irq_free_descs() will be eventually called to free irq and its
> descriptor.
> 
> In ice_resume(), ice_init_interrupt_scheme() is called to allocate
> new irqs.
> However, in ice_rebuild_arfs(), struct irq_glue and struct cpu_rmap
> maybe
> cannot be freed, if the irqs that released in ice_suspend() were
> reassigned
> to other devices, which makes irq descriptor's affinity_notify lost.
> 
> So move ice_remove_arfs() before ice_clear_interrupt_scheme(), which
> can
> make sure all irq_glue and cpu_rmap can be correctly released before
> corresponding irq and descriptor are released.
> 
> Fix the following memeory leak.

s/memeory/memory

<snip>

> diff --git a/drivers/net/ethernet/intel/ice/ice_arfs.c
> b/drivers/net/ethernet/intel/ice/ice_arfs.c
> index 6560acd76c94..c748d0a5c7d4 100644
> --- a/drivers/net/ethernet/intel/ice/ice_arfs.c
> +++ b/drivers/net/ethernet/intel/ice/ice_arfs.c
> @@ -654,7 +654,6 @@ void ice_rebuild_arfs(struct ice_pf *pf)
>  	if (!pf_vsi)
>  		return;
>  
> -	ice_remove_arfs(pf);

This should not be removed. Removing this would break the
reset flows outside of the suspend/remove case.

>  	if (ice_set_cpu_rx_rmap(pf_vsi)) {
>  		dev_err(ice_pf_to_dev(pf), "Failed to rebuild aRFS\n");
>  		return;
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
> b/drivers/net/ethernet/intel/ice/ice_main.c
> index 2c23c8f468a5..dba901bf2b9b 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -4568,6 +4568,9 @@ static int __maybe_unused ice_suspend(struct
> device *dev)
>  			continue;
>  		ice_vsi_free_q_vectors(pf->vsi[v]);
>  	}
> +	if (test_bit(ICE_FLAG_FD_ENA, pf->flags)) {
> +		ice_remove_arfs(pf);
> +	}

Braces aren't needed around a single if statement like this.

Also, I don't think this is the right solution. I think a better
approach would be to call ice_free_rx_cpu_map() here. With this,
it seems like no other changes are necessary. It also isn't
necessary to check the ICE_FLAG_FD_ENA bit with this change.

>  	ice_clear_interrupt_scheme(pf);
>  
>  	pci_save_state(pdev);