netdev - Re: [PATCH net v2 2/3] ice: Fix deadlock on the rtnl

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:   Wed, 4 Jan 2023 09:44:35 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Tony Nguyen <anthony.l.nguyen@...el.com>
Cc:     davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
        edumazet@...gle.com,
        Mateusz Palczewski <mateusz.palczewski@...el.com>,
        netdev@...r.kernel.org, Gurucharan G <gurucharanx.g@...el.com>
Subject: Re: [PATCH net v2 2/3] ice: Fix deadlock on the rtnl_mutex

On Tue, Jan 03, 2023 at 03:07:37PM -0800, Tony Nguyen wrote:
> From: Mateusz Palczewski <mateusz.palczewski@...el.com>
> 
> There is a deadlock on rtnl_mutex when attempting to take the lock
> in unregister_netdev() after it has already been taken by
> ethnl_set_channels(). This happened when unregister_netdev() was
> called inside of ice_vsi_rebuild().
> Fix that by removing the unregister_netdev() usage and replace it with
> ice_vsi_clear_rings() that deallocates the tx and rx rings for the VSI.
> 
> Fixes: df0f847915b4 ("ice: Move common functions out of ice_main.c part 6/7")
> Signed-off-by: Mateusz Palczewski <mateusz.palczewski@...el.com>
> Tested-by: Gurucharan G <gurucharanx.g@...el.com> (A Contingent worker at Intel)
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_lib.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index 94aa834cd9a6..22bcb414546a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -3619,12 +3619,10 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, bool init_vsi)
>  err_vectors:
>  	ice_vsi_free_q_vectors(vsi);
>  err_rings:
> -	if (vsi->netdev) {
> -		vsi->current_netdev_flags = 0;
> -		unregister_netdev(vsi->netdev);
> -		free_netdev(vsi->netdev);
> -		vsi->netdev = NULL;
> -	}
> +	ice_vsi_clear_rings(vsi);
> +	set_bit(ICE_RESET_FAILED, pf->state);
> +	kfree(coalesce);
> +	return ret;
>  err_vsi:

This is very odd error unwind idiom only to perform:
if (something)
   ice_vsi_clear_rings(vsi);
else
   ice_vsi_clear(vsi);

Please avoid putting "return ..." and same code in the middle of goto unwind.

Thanks

>  	ice_vsi_clear(vsi);
>  	set_bit(ICE_RESET_FAILED, pf->state);
> -- 
> 2.38.1
>