netdev - Re: [PATCH net-next v2] xfrm: bonding: Add XFRM packet-offload for active-backup

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: <aG3NfmO5wsQWKaoz@fedora>
Date: Wed, 9 Jul 2025 02:01:34 +0000
From: Hangbin Liu <liuhangbin@...il.com>
To: Erwan Dufour <mrarmonius@...il.com>
Cc: netdev@...r.kernel.org, steffen.klassert@...unet.com,
	herbert@...dor.apana.org.au, davem@...emloft.net, jv@...sburgh.net,
	saeedm@...dia.com, tariqt@...dia.com, erwan.dufour@...hings.com,
	cratiu@...dia.com, leon@...nel.org
Subject: Re: [PATCH net-next v2] xfrm: bonding: Add XFRM packet-offload for
 active-backup

Hi Erwan,

On Sun, Jul 06, 2025 at 04:58:04PM +0200, Erwan Dufour wrote:
> From: Erwan Dufour <erwan.dufour@...hings.com>
> 
> New features added:
> - Use of packet offload added for XFRM in active-backup
> - Behaviour modification when changing primary slave to prevent reuse of IV.

...

> 
> -static void bond_ipsec_add_sa_all(struct bonding *bond)
> +static void bond_ipsec_add_sa_sp_all(struct bonding *bond)
>  {
>  	struct net_device *bond_dev = bond->dev;
>  	struct net_device *real_dev;
>  	struct bond_ipsec *ipsec;
>  	struct slave *slave;
> +	int err;
>  
>  	slave = rtnl_dereference(bond->curr_active_slave);
>  	real_dev = slave ? slave->dev : NULL;
>  	if (!real_dev)
>  		return;
>  
> -	mutex_lock(&bond->ipsec_lock);
> +	mutex_lock(&bond->ipsec_lock_sa);
>  	if (!real_dev->xfrmdev_ops ||
>  	    !real_dev->xfrmdev_ops->xdo_dev_state_add ||
>  	    netif_is_bond_master(real_dev)) {
> -		if (!list_empty(&bond->ipsec_list))
> +		if (!list_empty(&bond->ipsec_list_sa))
>  			slave_warn(bond_dev, real_dev,
>  				   "%s: no slave xdo_dev_state_add\n",
>  				   __func__);
> -		goto out;
> +		goto out_sa;
>  	}
>  
> -	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
> -		/* If new state is added before ipsec_lock acquired */
> +	list_for_each_entry(ipsec, &bond->ipsec_list_sa, list) {
> +		/* If new state is added before ipsec_lock_sa acquired */
>  		if (ipsec->xs->xso.real_dev == real_dev)
>  			continue;
>  
> -		if (real_dev->xfrmdev_ops->xdo_dev_state_add(real_dev,
> -							     ipsec->xs, NULL)) {
> -			slave_warn(bond_dev, real_dev, "%s: failed to add SA\n", __func__);
> +		err = __xfrm_state_delete(ipsec->xs);
> +		if (!err)
> +			km_state_expired(ipsec->xs, 1, 0);
> +
> +		xfrm_audit_state_delete(ipsec->xs, err ? 0 : 1, true);

I see you delete ipsec->xs here. Do you mean to prevent reuse of IV?
But I can't find you add the xs back to new slave.

> +	}
> +out_sa:
> +	mutex_unlock(&bond->ipsec_lock_sa);
> +
> +	mutex_lock(&bond->ipsec_lock_sp);
> +	if (!real_dev->xfrmdev_ops ||
> +	    !real_dev->xfrmdev_ops->xdo_dev_policy_add ||
> +	    netif_is_bond_master(real_dev)) {
> +		if (!list_empty(&bond->ipsec_list_sp))
> +			slave_warn(bond_dev, real_dev,
> +				   "%s: no slave xdo_dev_policy_add\n",
> +				   __func__);
> +		goto out_sp;
> +	}
> +	list_for_each_entry(ipsec, &bond->ipsec_list_sp, list) {
> +		if (ipsec->xp->xdo.real_dev == real_dev)
> +			continue;
> +
> +		if (real_dev->xfrmdev_ops->xdo_dev_policy_add(real_dev,
> +							      ipsec->xp,
> +							      NULL)) {
> +			slave_warn(bond_dev, real_dev,
> +				   "%s: failed to add SP\n", __func__);
>  			continue;

Here you do xdo_dev_policy_add(). What about the xdo_dev_state_add()?

>  		}
>  
> -		spin_lock_bh(&ipsec->xs->lock);
> -		/* xs might have been killed by the user during the migration
> -		 * to the new dev, but bond_ipsec_del_sa() should have done
> -		 * nothing, as xso.real_dev is NULL.
> -		 * Delete it from the device we just added it to. The pending
> -		 * bond_ipsec_free_sa() call will do the rest of the cleanup.
> -		 */
> -		if (ipsec->xs->km.state == XFRM_STATE_DEAD &&
> -		    real_dev->xfrmdev_ops->xdo_dev_state_delete)
> -			real_dev->xfrmdev_ops->xdo_dev_state_delete(real_dev,
> -								    ipsec->xs);

Here the xdo_dev_state_delete() is called when km.state == XFRM_STATE_DEAD.
Why we remove this?

> -		ipsec->xs->xso.real_dev = real_dev;
> -		spin_unlock_bh(&ipsec->xs->lock);
> +		write_lock_bh(&ipsec->xp->lock);
> +		ipsec->xp->xdo.real_dev = real_dev;
> +		write_unlock_bh(&ipsec->xp->lock);
>  	}
> -out:
> -	mutex_unlock(&bond->ipsec_lock);
> +
> +out_sp:
> +	mutex_unlock(&bond->ipsec_lock_sp);
>  }

Thanks
Hangbin