Re: [PATCH] [PATH xfrm offload] xfrm: bonding: Add xfrm packet offload for active-backup mode

[Hangbin Liu <liuhangbin@gmail.com>]

Hi Erwan,

Cc Cosmin as he updated bond ipsec a lot.

Maybe the subject prefix could be [PATCH net-next].

On Sun, Jun 29, 2025 at 11:06:23PM +0200, Erwan Dufour wrote:
> From: Erwan Dufour <erwan.dufour@withings.com>
> 
> Implement XFRM policy offload functions for bond device in active-backup mode.
>  - xdo_dev_policy_add = bond_ipsec_add_sp
>  - xdo_dev_policy_delete = bond_ipsec_del_sp
>  _ xdo_deb_policy_free = bond_ipsec_free_sp
> 
> Modification of the function signature for copying on SA models.
> Also add netdevice pointer to avoid to use real_dev which is obsolete and deleted for policy.
> 
> Store the bond's xfrm policies in the struct bond_ipsec.
> Also rename these functions:
>  - bond_ipsec_del_sa_all -> bond_ipsec_del_sa_sp_all
>  - bond_ipsec_add_sa_all -> bond_ipsec_add_sa_sp_all
> Now bond_ipsec_{del,add}_sa_sp_all remove/add also the bond's policies stores in same struct as SA.
> 
> Tested on Mellanox ConnectX-6 Dx Crypto Enable Cards.
> ---
>  drivers/net/bonding/bond_main.c               | 279 +++++++++++++++---
>  .../mellanox/mlx5/core/en_accel/ipsec.c       |  10 +-
>  include/linux/netdevice.h                     |   6 +-
>  include/net/bonding.h                         |   1 +
>  include/net/xfrm.h                            |   4 +-
>  net/xfrm/xfrm_device.c                        |   2 +-
>  6 files changed, 246 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index c4d53e8e7c15..85017635f9b5 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -512,7 +512,7 @@ static int bond_ipsec_add_sa(struct net_device *bond_dev,
>  	return err;
>  }
>  
> -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;
> @@ -536,29 +536,44 @@ static void bond_ipsec_add_sa_all(struct bonding *bond)
>  	}
>  
>  	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
> -		/* If new state is added before ipsec_lock acquired */
> -		if (ipsec->xs->xso.real_dev == real_dev)
> -			continue;
> +		if (ipsec->xs) {
> +			/* If new state is added before ipsec_lock 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__);
> -			continue;
> -		}
> +			if (real_dev->xfrmdev_ops->xdo_dev_state_add(real_dev,
> +									ipsec->xs, NULL)) {

Please fix the code alignment. And all others in the code.

> +
> +/**
> + * bond_ipsec_del_sp - clear out this specific SP
> + * @bond_dev: pointer to net device
> + * @xs: pointer to transformer policy struct
> + **/
> +static void bond_ipsec_del_sp(struct net_device *bond_dev, struct xfrm_policy *xp)
> +{
> +	struct net_device *real_dev;
> +	netdevice_tracker tracker;
> +	struct bond_ipsec *ipsec;
> +	struct bonding *bond;
> +	struct slave *slave;
> +
> +	if (!bond_dev)
> +		return;
> +
> +	rcu_read_lock();
> +	bond = netdev_priv(bond_dev);
> +	slave = rcu_dereference(bond->curr_active_slave);
> +	real_dev = slave ? slave->dev : NULL;
> +	netdev_hold(real_dev, &tracker, GFP_ATOMIC);
> +	rcu_read_unlock();
> +
> +	if (!slave)
> +		goto out;
> +
> +	if (!xp->xdo.real_dev)
> +		goto out;
> +
> +	WARN_ON(xp->xdo.real_dev != real_dev);
> +
> +	if (!real_dev->xfrmdev_ops ||
> +	    !real_dev->xfrmdev_ops->xdo_dev_policy_delete ||
> +	    netif_is_bond_master(real_dev)) {
> +		slave_warn(bond_dev, real_dev, "%s: no slave xdo_dev_policy_delete\n", __func__);
> +		goto out;
> +	}
> +
> +	real_dev->xfrmdev_ops->xdo_dev_policy_delete(real_dev, xp);
> +out:
> +	netdev_put(real_dev, &tracker);
> +	mutex_lock(&bond->ipsec_lock);
> +	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
> +		if (ipsec->xp == xp) {
> +			list_del(&ipsec->list);
> +			kfree(ipsec);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&bond->ipsec_lock);
> +}

In xfrm_add_policy() err out, it calls xfrm_dev_policy_delete() first and
then xfrm_dev_policy_free(). So why we free ipsec->list in bond_ipsec_del_sp()
but no bond_ipsec_free_sp()?

BTW, if (ipsec->xp == xp), should we delete the whole ipsec_list? Is it
possible ipsec->xs still exist?

Thanks
Hangbin

> +
> +static void bond_ipsec_free_sp(struct net_device *bond_dev, struct xfrm_policy *xp)
> +{
> +	struct net_device *real_dev;
> +	netdevice_tracker tracker;
> +	struct bonding *bond;
> +	struct slave *slave;
> +
> +	if (!bond_dev)
> +		return;
> +
> +	rcu_read_lock();
> +	bond = netdev_priv(bond_dev);
> +	slave = rcu_dereference(bond->curr_active_slave);
> +	real_dev = slave ? slave->dev : NULL;
> +	netdev_hold(real_dev, &tracker, GFP_ATOMIC);
> +	rcu_read_unlock();
> +
> +	if (!slave)
> +		goto out;
> +
> +	if (!xp->xdo.real_dev)
> +		goto out;
> +
> +	WARN_ON(xp->xdo.real_dev != real_dev);
> +
> +	if (real_dev && real_dev->xfrmdev_ops &&
> +	    real_dev->xfrmdev_ops->xdo_dev_policy_free)
> +		real_dev->xfrmdev_ops->xdo_dev_policy_free(real_dev, xp);
> +out:
> +	netdev_put(real_dev, &tracker);
> +}
> +
>  static const struct xfrmdev_ops bond_xfrmdev_ops = {
>  	.xdo_dev_state_add = bond_ipsec_add_sa,
>  	.xdo_dev_state_delete = bond_ipsec_del_sa,
> @@ -738,6 +924,9 @@ static const struct xfrmdev_ops bond_xfrmdev_ops = {
>  	.xdo_dev_offload_ok = bond_ipsec_offload_ok,
>  	.xdo_dev_state_advance_esn = bond_advance_esn_state,
>  	.xdo_dev_state_update_stats = bond_xfrm_update_stats,
> +	.xdo_dev_policy_add = bond_ipsec_add_sp,
> +	.xdo_dev_policy_delete = bond_ipsec_del_sp,
> +	.xdo_dev_policy_free = bond_ipsec_free_sp,
>  };
>  #endif /* CONFIG_XFRM_OFFLOAD */
>  
> @@ -1277,7 +1466,7 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
>  		return;
>  
>  #ifdef CONFIG_XFRM_OFFLOAD
> -	bond_ipsec_del_sa_all(bond);
> +	bond_ipsec_del_sa_sp_all(bond);
>  #endif /* CONFIG_XFRM_OFFLOAD */
>  
>  	if (new_active) {
> @@ -1352,7 +1541,7 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
>  	}
>  
>  #ifdef CONFIG_XFRM_OFFLOAD
> -	bond_ipsec_add_sa_all(bond);
> +	bond_ipsec_add_sa_sp_all(bond);
>  #endif /* CONFIG_XFRM_OFFLOAD */
>  
>  	/* resend IGMP joins since active slave has changed or
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> index 77f61cd28a79..f5e3fc054f41 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> @@ -1161,15 +1161,15 @@ mlx5e_ipsec_build_accel_pol_attrs(struct mlx5e_ipsec_pol_entry *pol_entry,
>  	attrs->prio = x->priority;
>  }
>  
> -static int mlx5e_xfrm_add_policy(struct xfrm_policy *x,
> +static int mlx5e_xfrm_add_policy(struct net_device *dev,
> +				 struct xfrm_policy *x,
>  				 struct netlink_ext_ack *extack)
>  {
> -	struct net_device *netdev = x->xdo.dev;
>  	struct mlx5e_ipsec_pol_entry *pol_entry;
>  	struct mlx5e_priv *priv;
>  	int err;
>  
> -	priv = netdev_priv(netdev);
> +	priv = netdev_priv(dev);
>  	if (!priv->ipsec) {
>  		NL_SET_ERR_MSG_MOD(extack, "Device doesn't support IPsec packet offload");
>  		return -EOPNOTSUPP;
> @@ -1207,7 +1207,7 @@ static int mlx5e_xfrm_add_policy(struct xfrm_policy *x,
>  	return err;
>  }
>  
> -static void mlx5e_xfrm_del_policy(struct xfrm_policy *x)
> +static void mlx5e_xfrm_del_policy(struct net_device *dev, struct xfrm_policy *x)
>  {
>  	struct mlx5e_ipsec_pol_entry *pol_entry = to_ipsec_pol_entry(x);
>  
> @@ -1215,7 +1215,7 @@ static void mlx5e_xfrm_del_policy(struct xfrm_policy *x)
>  	mlx5_eswitch_unblock_ipsec(pol_entry->ipsec->mdev);
>  }
>  
> -static void mlx5e_xfrm_free_policy(struct xfrm_policy *x)
> +static void mlx5e_xfrm_free_policy(struct net_device *dev, struct xfrm_policy *x)
>  {
>  	struct mlx5e_ipsec_pol_entry *pol_entry = to_ipsec_pol_entry(x);
>  
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index adb14db25798..7c3d74d28ef4 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1024,9 +1024,9 @@ struct xfrmdev_ops {
>  				       struct xfrm_state *x);
>  	void	(*xdo_dev_state_advance_esn) (struct xfrm_state *x);
>  	void	(*xdo_dev_state_update_stats) (struct xfrm_state *x);
> -	int	(*xdo_dev_policy_add) (struct xfrm_policy *x, struct netlink_ext_ack *extack);
> -	void	(*xdo_dev_policy_delete) (struct xfrm_policy *x);
> -	void	(*xdo_dev_policy_free) (struct xfrm_policy *x);
> +	int	(*xdo_dev_policy_add) (struct net_device *dev, struct xfrm_policy *x, struct netlink_ext_ack *extack);
> +	void	(*xdo_dev_policy_delete) (struct net_device *dev, struct xfrm_policy *x);
> +	void	(*xdo_dev_policy_free) (struct net_device *dev, struct xfrm_policy *x);
>  };
>  #endif
>  
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index 95f67b308c19..6ac079673f87 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -207,6 +207,7 @@ struct bond_up_slave {
>  struct bond_ipsec {
>  	struct list_head list;
>  	struct xfrm_state *xs;
> +	struct xfrm_policy *xp;
>  };
>  
>  /*
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index a21e276dbe44..ffae7cc1f989 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -2116,7 +2116,7 @@ static inline void xfrm_dev_policy_delete(struct xfrm_policy *x)
>  	struct net_device *dev = xdo->dev;
>  
>  	if (dev && dev->xfrmdev_ops && dev->xfrmdev_ops->xdo_dev_policy_delete)
> -		dev->xfrmdev_ops->xdo_dev_policy_delete(x);
> +		dev->xfrmdev_ops->xdo_dev_policy_delete(dev, x);
>  }
>  
>  static inline void xfrm_dev_policy_free(struct xfrm_policy *x)
> @@ -2126,7 +2126,7 @@ static inline void xfrm_dev_policy_free(struct xfrm_policy *x)
>  
>  	if (dev && dev->xfrmdev_ops) {
>  		if (dev->xfrmdev_ops->xdo_dev_policy_free)
> -			dev->xfrmdev_ops->xdo_dev_policy_free(x);
> +			dev->xfrmdev_ops->xdo_dev_policy_free(dev, x);
>  		xdo->dev = NULL;
>  		netdev_put(dev, &xdo->dev_tracker);
>  	}
> diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> index 81fd486b5e56..643679b8d13c 100644
> --- a/net/xfrm/xfrm_device.c
> +++ b/net/xfrm/xfrm_device.c
> @@ -394,7 +394,7 @@ int xfrm_dev_policy_add(struct net *net, struct xfrm_policy *xp,
>  		return -EINVAL;
>  	}
>  
> -	err = dev->xfrmdev_ops->xdo_dev_policy_add(xp, extack);
> +	err = dev->xfrmdev_ops->xdo_dev_policy_add(dev, xp, extack);
>  	if (err) {
>  		xdo->dev = NULL;
>  		xdo->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
> -- 
> 2.43.0
>