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

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

[Erwan Dufour]

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)) {
+				slave_warn(bond_dev, real_dev, "%s: failed to add SA\n", __func__);
+				continue;
+			}
 
-		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);
-		ipsec->xs->xso.real_dev = real_dev;
-		spin_unlock_bh(&ipsec->xs->lock);
+			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);
+			ipsec->xs->xso.real_dev = real_dev;
+			spin_unlock_bh(&ipsec->xs->lock);
+		} else {
+			/* XFRM Policy Part */
+			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;
+			}
+			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);
@@ -589,7 +604,7 @@ static void bond_ipsec_del_sa(struct net_device *bond_dev,
 	real_dev->xfrmdev_ops->xdo_dev_state_delete(real_dev, xs);
 }
 
-static void bond_ipsec_del_sa_all(struct bonding *bond)
+static void bond_ipsec_del_sa_sp_all(struct bonding *bond)
 {
 	struct net_device *bond_dev = bond->dev;
 	struct net_device *real_dev;
@@ -603,29 +618,55 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
 
 	mutex_lock(&bond->ipsec_lock);
 	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
-		if (!ipsec->xs->xso.real_dev)
-			continue;
+		if (ipsec->xs) {
+			if (!ipsec->xs->xso.real_dev)
+				continue;
 
-		if (!real_dev->xfrmdev_ops ||
-		    !real_dev->xfrmdev_ops->xdo_dev_state_delete ||
-		    netif_is_bond_master(real_dev)) {
-			slave_warn(bond_dev, real_dev,
-				   "%s: no slave xdo_dev_state_delete\n",
-				   __func__);
-			continue;
-		}
+			if (!real_dev->xfrmdev_ops ||
+				!real_dev->xfrmdev_ops->xdo_dev_state_delete ||
+				netif_is_bond_master(real_dev)) {
+				slave_warn(bond_dev, real_dev,
+					"%s: no slave xdo_dev_state_delete\n",
+					__func__);
+				continue;
+			}
 
-		spin_lock_bh(&ipsec->xs->lock);
-		ipsec->xs->xso.real_dev = NULL;
-		/* Don't double delete states killed by the user. */
-		if (ipsec->xs->km.state != XFRM_STATE_DEAD)
-			real_dev->xfrmdev_ops->xdo_dev_state_delete(real_dev,
-								    ipsec->xs);
-		spin_unlock_bh(&ipsec->xs->lock);
+			spin_lock_bh(&ipsec->xs->lock);
+			ipsec->xs->xso.real_dev = NULL;
+			/* Don't double delete states killed by the user. */
+			if (ipsec->xs->km.state != XFRM_STATE_DEAD)
+				real_dev->xfrmdev_ops->xdo_dev_state_delete(real_dev,
+										ipsec->xs);
+			spin_unlock_bh(&ipsec->xs->lock);
+
+			if (real_dev->xfrmdev_ops->xdo_dev_state_free)
+				real_dev->xfrmdev_ops->xdo_dev_state_free(real_dev,
+									ipsec->xs);
+		} else {
+			/* XFRM Policy part */
+			if (!ipsec->xp->xdo.real_dev)
+				continue;
 
-		if (real_dev->xfrmdev_ops->xdo_dev_state_free)
-			real_dev->xfrmdev_ops->xdo_dev_state_free(real_dev,
-								  ipsec->xs);
+			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__);
+				continue;
+			}
+			/* use write rwlock */
+			write_lock_bh(&ipsec->xp->lock);
+			ipsec->xp->xdo.real_dev = NULL;
+			write_unlock_bh(&ipsec->xp->lock);
+
+			real_dev->xfrmdev_ops->xdo_dev_policy_delete(real_dev,
+										ipsec->xp);
+
+			if (real_dev->xfrmdev_ops->xdo_dev_state_free)
+				real_dev->xfrmdev_ops->xdo_dev_policy_free(real_dev,
+									ipsec->xp);
+		}
 	}
 	mutex_unlock(&bond->ipsec_lock);
 }
@@ -731,6 +772,151 @@ static void bond_xfrm_update_stats(struct xfrm_state *xs)
 	rcu_read_unlock();
 }
 
+/**
+ * bond_ipsec_add_sp - program device with a security policy
+ * @bond_dev: pointer to net device
+ * @xs: pointer to transformer policy struct
+ * @extack: extack point to fill failure reason
+ **/
+static int bond_ipsec_add_sp(struct net_device *bond_dev,
+				 struct xfrm_policy *xp,
+			     struct netlink_ext_ack *extack)
+{
+	struct net_device *real_dev;
+	netdevice_tracker tracker;
+	struct bond_ipsec *ipsec;
+	struct bonding *bond;
+	struct slave *slave;
+	int err;
+
+	if (!bond_dev)
+		return -EINVAL;
+
+	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 (!real_dev) {
+		err = -ENODEV;
+		goto out;
+	}
+
+	if (!real_dev->xfrmdev_ops ||
+	    !real_dev->xfrmdev_ops->xdo_dev_policy_add ||
+	    netif_is_bond_master(real_dev)) {
+		NL_SET_ERR_MSG_MOD(extack, "Slave does not support security policy offload");
+		err = -EINVAL;
+		goto out;
+	}
+
+	ipsec = kmalloc(sizeof(*ipsec), GFP_KERNEL);
+	if (!ipsec) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	err = real_dev->xfrmdev_ops->xdo_dev_policy_add(real_dev, xp, extack);
+	if (!err) {
+		xp->xdo.real_dev = real_dev;
+		ipsec->xp = xp;
+		INIT_LIST_HEAD(&ipsec->list);
+		mutex_lock(&bond->ipsec_lock);
+		list_add(&ipsec->list, &bond->ipsec_list);
+		mutex_unlock(&bond->ipsec_lock);
+	} else {
+		kfree(ipsec);
+	}
+out:
+	netdev_put(real_dev, &tracker);
+	return err;
+}
+
+/**
+ * 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);
+}
+
+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

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

[Hangbin Liu]

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
> 

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

[Hangbin Liu]

Hi Erwan,
On Mon, Jun 30, 2025 at 03:50:46PM +0200, Erwan Dufour wrote:
> Hi Liu,
> Thank's you for your feedback,
> You can find the new patch at the end if this email.
> 
> Please fix the code alignment. And all others in the code.
> 
> Sorry, I'm not really sure to understand. I use indentation in TAB mode
> which has a size of 4.
> I'm not sure to find alignment problem ?  Maybe I don't know the rules for
> this repository.

Tabs are 8 characters.

More coding styles, please check https://www.kernel.org/doc/html/latest/process/coding-style.html

> 
> 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()?
> 
> Good question. I've taken inspiration from version 6.15 for these two
> functions.
> I've just seen that there's been a commit and now the ipsec is only
> released in the free function.
> On my review patch, only the function bond_ipsec_free_sp release ipsec, not
> bond_ipsec_delete_sp.
> The function bond_ipsec_free_sp is always called after the
> bond_ipsec_delete_sp function.
> This is why we now only release in bond_free_sp().

OK, I see your new patch freed the list in bond_free_sp() now.

> 
> 
> BTW, if (ipsec->xp == xp), should we delete the whole ipsec_list? Is it
> > possible ipsec->xs still exist?
> 
> In our case, the struct bond_ipsec *ipsec can have only one xs or one xp
> but not both.
> In functions bond_add_sp or bond_add_sa, we create the struct bond_ipsec
> and put the value.
> This is the only place we create a bond_ipsec, and we never update it
> either. We only read or delete it.

Hmm, I'm not very familiar with IPsec. I thought we can config xfrm state and
policy on the interface at same time. Need others review this part.

Thanks
Hangbin

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

[Hangbin Liu]

Hi Erwan,

On Tue, Jul 01, 2025 at 07:29:48PM +0200, Erwan Dufour wrote:
> Hi Liu,
> Thank you for the link.
> The new patch with the good tab size can be found at the end of this email.
> 
> Hmm, I'm not very familiar with IPsec. I thought we can config xfrm state
> > and
> > policy on the interface at same time. Need others review this part.
> 
> The ip XFRM state and ip XFRM policy that you see when you use iproute with
> 'ip xfrm state' or 'ip xfrm policy' command in cli may be on the same
> interface.
> But in the code here, the struct bond_ipsec is just an element of a list
> used to store all the SAs and SPs linked to this device bond.
> So this structure allows us to remove the SA and SP offloads from the old
> primary slave and add them to the new one during primary current slave
> exchanges. (function bond_change_active_slave() )
> The bond_ipsec struct is an element of a list which stores all the SAs and
> SPs of the device bond.
> So every time we add an SA or SP to our bond, we create a new bond_ipsec
> object and add it to our list. This is why, in our structure, we cannot
> have an SA and an SP in the same bond_ipsec object.

Thanks for your explanation. Unfortunately，the alignment still not works.

e.g.

> 
> -		if (real_dev->xfrmdev_ops->xdo_dev_state_add(real_dev,
> -							     ipsec->xs, NULL)) {

Here the ipsec->xs is aligned with real_dev.

> -			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)) {

But here, ipsec->xs is not aligned with real_dev.
If the code cannot be aligned properly using tabs alone, you can use spaces
to complete the alignment.

Thanks
Hangbin

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

[Hangbin Liu]

On Thu, Jul 03, 2025 at 01:58:36AM +0200, Erwan Dufour wrote:
> Hi Liu,
> 
> Thanks for your explanation. Unfortunately，the alignment still not works.
> 
> With pleasure. Thank you very much for providing an example with an
> explanation.
> Hopefully, there were no mistakes and I managed to correct all the errors
> in the new patch.

Yes, the patch looks good to me now.

Hangbin

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

[Cosmin Ratiu]

+Leon, he cares about IPSec in mlx5.

On Thu, 2025-07-03 at 01:58 +0200, Erwan Dufour wrote:
> Hi Liu,
> 
> > Thanks for your explanation. Unfortunately，the alignment still not
> > works.
> > 
> 
> With pleasure. Thank you very much for providing an example with an
> explanation. 
> Hopefully, there were no mistakes and I managed to correct all the
> errors in the new patch.
> 
> New Patch:
> From 39639cf83712b13271fc3d8bbe3f4d9cd0b38db6 Mon Sep 17 00:00:00
> 2001
> From: Erwan Dufour <erwan.dufour@withings.com>
> Date: Wed, 2 Jul 2025 22:12:10 +0000
> Subject: [PATCH net-next] xfrm: bonding: Add xfrm packet offload for
>  active-backup mode
> 
> 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
Typo here ^. Should be "dev", not "deb".

> 
> 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.
> 
> Signed-off-by: Erwan Dufour <erwan.dufour@withings.com>
> ---
>  drivers/net/bonding/bond_main.c               | 251 ++++++++++++++--
> --
>  .../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, 218 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c
> b/drivers/net/bonding/bond_main.c
> index c4d53e8e7c15..c9505441c863 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)) {
> +				slave_warn(bond_dev, real_dev, "%s:
> failed to add SA\n", __func__);
Lines should be max 80-characters. Please use checkpatch.pl to verify
these things:
scripts/checkpatch.pl -g --max-line-length 80 HEAD-1


[...] I didn't review the bulk of the changes in detail, because:

> 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;
Instead of carrying a mixed list of states and policies, which forces
every piece of code handling this into a model of "if (xs) {} else {}",
prefer two separate lists, one for states and another one for policies.
Then, to make it extra clear that xs and xp are mutually exclusive, an
unnamed union between xs and xp could be used.

Also, in the future, please send new versions of the patch as separate
emails, with subjects such as "[PATCH net-next v2] $title", to interact
better with the kernel patchwork system. Right now, this patch is in a
weird state:
https://patchwork.kernel.org/project/netdevbpf/patch/20250629210623.43497-1-mramonius@gmail.com/


Cosmin.

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

[Cosmin Ratiu]

On Thu, 2025-07-03 at 14:16 +0000, Cosmin Ratiu wrote:
> On Thu, 2025-07-03 at 01:58 +0200, Erwan Dufour wrote:
> > 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
> Typo here ^. Should be "dev", not "deb".
Nit: Also, there's an underscore at the beginning instead of a dash.
Please be consistent.

Cosmin.

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

[Steffen Klassert]

On Thu, Jul 03, 2025 at 01:58:36AM +0200, Erwan Dufour wrote:
> Hi Liu,
> 
> Thanks for your explanation. Unfortunately，the alignment still not works.
> 
> With pleasure. Thank you very much for providing an example with an
> explanation.
> Hopefully, there were no mistakes and I managed to correct all the errors
> in the new patch.
> 
> New Patch:
> 
> >From 39639cf83712b13271fc3d8bbe3f4d9cd0b38db6 Mon Sep 17 00:00:00 2001
> From: Erwan Dufour <erwan.dufour@withings.com>
> Date: Wed, 2 Jul 2025 22:12:10 +0000
> Subject: [PATCH net-next] xfrm: bonding: Add xfrm packet offload for
>  active-backup mode
> 
> 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

We should not add further xfrm offloads to bonding as long
as the security issues are not solved. Moving an already
used SA from one device to another can lead to IV reusage,
as discussed here:

https://lore.kernel.org/all/ZsbkdzvjVf3GiYHa@gauss3.secunet.de/

This should be fixed before we add another offload.