linux-kernel - Re: [PATCHv3 net 1/3] bonding: move IPsec deletion to bond_ipsec_free

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] [day] [month] [year] [list]

Message-ID: <3a5a4d5f943a361d55f1454d2a7c3189eb398c40.camel@nvidia.com>
Date: Tue, 4 Mar 2025 10:25:53 +0000
From: Cosmin Ratiu <cratiu@...dia.com>
To: "liuhangbin@...il.com" <liuhangbin@...il.com>
CC: "shuah@...nel.org" <shuah@...nel.org>, "andrew+netdev@...n.ch"
	<andrew+netdev@...n.ch>, "davem@...emloft.net" <davem@...emloft.net>,
	"jv@...sburgh.net" <jv@...sburgh.net>, "jarod@...hat.com" <jarod@...hat.com>,
	"razor@...ckwall.org" <razor@...ckwall.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "edumazet@...gle.com" <edumazet@...gle.com>,
	Jianbo Liu <jianbol@...dia.com>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"horms@...nel.org" <horms@...nel.org>, "kuba@...nel.org" <kuba@...nel.org>,
	Tariq Toukan <tariqt@...dia.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "steffen.klassert@...unet.com"
	<steffen.klassert@...unet.com>, "linux-kselftest@...r.kernel.org"
	<linux-kselftest@...r.kernel.org>
Subject: Re: [PATCHv3 net 1/3] bonding: move IPsec deletion to
 bond_ipsec_free_sa

On Tue, 2025-03-04 at 09:18 +0000, Hangbin Liu wrote:
> 
> Just to make sure I added the lock in correct place, would you please
> help
> confirm.
> 
> diff --git a/drivers/net/bonding/bond_main.c
> b/drivers/net/bonding/bond_main.c
> index e85878b12376..c59ad3a5cf43 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -537,19 +537,25 @@ static void bond_ipsec_add_sa_all(struct
> bonding *bond)
>  	}
>  
>  	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
> +		spin_lock_bh(&ipsec->xs->lock);
>  		/* Skip dead xfrm states, they'll be freed later. */
> -		if (ipsec->xs->km.state == XFRM_STATE_DEAD)
> +		if (ipsec->xs->km.state == XFRM_STATE_DEAD) {
> +			spin_unlock_bh(&ipsec->xs->lock);

Instead of unlocking on every branch, I recommend adding a "next:" tag
before the unlock at the end of the loop and switching the "continue"
statements with "goto next".

>  			continue;
> +		}
>  
>  		/* If new state is added before ipsec_lock acquired
> */
> -		if (ipsec->xs->xso.real_dev == real_dev)
> +		if (ipsec->xs->xso.real_dev == real_dev) {
> +			spin_unlock_bh(&ipsec->xs->lock);
>  			continue;
> +		}
>  
>  		ipsec->xs->xso.real_dev = real_dev;
>  		if (real_dev->xfrmdev_ops->xdo_dev_state_add(ipsec-
> >xs, NULL)) {
>  			slave_warn(bond_dev, real_dev, "%s: failed
> to add SA\n", __func__);
>  			ipsec->xs->xso.real_dev = NULL;
>  		}

Add the "next:" tag here.

> +		spin_unlock_bh(&ipsec->xs->lock);
>  	}
>  out:
>  	mutex_unlock(&bond->ipsec_lock);
> @@ -614,6 +620,7 @@ static void bond_ipsec_del_sa_all(struct bonding
> *bond)
>  		if (!ipsec->xs->xso.real_dev)
>  			continue;

The above if should be in the critical section as well.

>  
> +		spin_lock_bh(&ipsec->xs->lock);
>  		if (ipsec->xs->km.state == XFRM_STATE_DEAD) {
>  			/* already dead no need to delete again */
>  			if (ipsec->xs->xso.real_dev == real_dev &&
> @@ -621,6 +628,7 @@ static void bond_ipsec_del_sa_all(struct bonding
> *bond)
>  				real_dev->xfrmdev_ops-
> >xdo_dev_state_free(ipsec->xs);
>  			list_del(&ipsec->list);
>  			kfree(ipsec);
> +			spin_unlock_bh(&ipsec->xs->lock);

And I recommend the same thing with "goto next" here, jumping at the
end of the loop, before the unlock.

>  			continue;
>  		}
>  
> @@ -635,6 +643,7 @@ static void bond_ipsec_del_sa_all(struct bonding
> *bond)
>  			if (real_dev->xfrmdev_ops-
> >xdo_dev_state_free)
>  				real_dev->xfrmdev_ops-
> >xdo_dev_state_free(ipsec->xs);
>  		}
> +		spin_unlock_bh(&ipsec->xs->lock);
>  	}
>  	mutex_unlock(&bond->ipsec_lock);
>  }
> 
> Thanks
> Hangbin