[PATCH net V3 0/3] Fixes for IPsec over bonding

Hi,

This patchset by Jianbo provides bug fixes for IPsec over bonding
driver.

It adds the missing xdo_dev_state_free API, and fixes "scheduling while
atomic" by using mutex lock instead.

Series generated against:
commit 14ab4792ee12 ("net/tcp: Disable TCP-AO static key after RCU grace period")

Regards,
Tariq

V3:
- Add RCU read lock/unlock for bond_ipsec_add_sa, bond_ipsec_del_sa and bond_ipsec_free_sa.

V2:
- Rebased on top of latest net branch.
- Squashed patch #2 into #1 per Hangbin comment.
- Addressed Hangbin's comments.
- Patch #3 (was #4): Addressed comments by Paolo.

Jianbo Liu (3):
  bonding: implement xdo_dev_state_free and call it after deletion
  bonding: extract the use of real_device into local variable
  bonding: change ipsec_lock from spin lock to mutex

 drivers/net/bonding/bond_main.c | 151 ++++++++++++++++++++------------
 include/net/bonding.h           |   2 +-
 2 files changed, 98 insertions(+), 55 deletions(-)

-- 
2.44.0

From: Jianbo Liu <redacted>

Add this implementation for bonding, so hardware resources can be
freed after xfrm state is deleted.

And call it when deleting all SAs from old active real interface.

Fixes: 9a5605505d9c ("bonding: Add struct bond_ipesc to manage SA")
Signed-off-by: Jianbo Liu <redacted>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/bonding/bond_main.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 1cd92c12e782..eb5e43860670 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c

@@ -581,6 +581,8 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
 				   __func__);
 		} else {
 			slave->dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
+			if (slave->dev->xfrmdev_ops->xdo_dev_state_free)
+				slave->dev->xfrmdev_ops->xdo_dev_state_free(ipsec->xs);
 		}
 		ipsec->xs->xso.real_dev = NULL;
 	}

@@ -588,6 +590,35 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
 	rcu_read_unlock();
 }
 
+static void bond_ipsec_free_sa(struct xfrm_state *xs)
+{
+	struct net_device *bond_dev = xs->xso.dev;
+	struct net_device *real_dev;
+	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;
+	rcu_read_unlock();
+
+	if (!slave)
+		return;
+
+	if (!xs->xso.real_dev)
+		return;
+
+	WARN_ON(xs->xso.real_dev != real_dev);
+
+	if (real_dev && real_dev->xfrmdev_ops &&
+	    real_dev->xfrmdev_ops->xdo_dev_state_free)
+		real_dev->xfrmdev_ops->xdo_dev_state_free(xs);
+}
+
 /**
  * bond_ipsec_offload_ok - can this packet use the xfrm hw offload
  * @skb: current data packet

@@ -632,6 +663,7 @@ static bool bond_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs)
 static const struct xfrmdev_ops bond_xfrmdev_ops = {
 	.xdo_dev_state_add = bond_ipsec_add_sa,
 	.xdo_dev_state_delete = bond_ipsec_del_sa,
+	.xdo_dev_state_free = bond_ipsec_free_sa,
 	.xdo_dev_offload_ok = bond_ipsec_offload_ok,
 };
 #endif /* CONFIG_XFRM_OFFLOAD */

-- 
2.44.0

From: Jianbo Liu <redacted>

Add a local variable for slave->dev, to prepare for the lock change in
the next patch. There is no functionality change.

Fixes: 9a5605505d9c ("bonding: Add struct bond_ipesc to manage SA")
Signed-off-by: Jianbo Liu <redacted>
Reviewed-by: Cosmin Ratiu <redacted>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/bonding/bond_main.c | 58 +++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 25 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index eb5e43860670..e550b1c08fdb 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c

@@ -427,6 +427,7 @@ static int bond_ipsec_add_sa(struct xfrm_state *xs,
 			     struct netlink_ext_ack *extack)
 {
 	struct net_device *bond_dev = xs->xso.dev;
+	struct net_device *real_dev;
 	struct bond_ipsec *ipsec;
 	struct bonding *bond;
 	struct slave *slave;

@@ -443,9 +444,10 @@ static int bond_ipsec_add_sa(struct xfrm_state *xs,
 		return -ENODEV;
 	}
 
-	if (!slave->dev->xfrmdev_ops ||
-	    !slave->dev->xfrmdev_ops->xdo_dev_state_add ||
-	    netif_is_bond_master(slave->dev)) {
+	real_dev = slave->dev;
+	if (!real_dev->xfrmdev_ops ||
+	    !real_dev->xfrmdev_ops->xdo_dev_state_add ||
+	    netif_is_bond_master(real_dev)) {
 		NL_SET_ERR_MSG_MOD(extack, "Slave does not support ipsec offload");
 		rcu_read_unlock();
 		return -EINVAL;

@@ -456,9 +458,9 @@ static int bond_ipsec_add_sa(struct xfrm_state *xs,
 		rcu_read_unlock();
 		return -ENOMEM;
 	}
-	xs->xso.real_dev = slave->dev;
 
-	err = slave->dev->xfrmdev_ops->xdo_dev_state_add(xs, extack);
+	xs->xso.real_dev = real_dev;
+	err = real_dev->xfrmdev_ops->xdo_dev_state_add(xs, extack);
 	if (!err) {
 		ipsec->xs = xs;
 		INIT_LIST_HEAD(&ipsec->list);

@@ -475,6 +477,7 @@ static int bond_ipsec_add_sa(struct xfrm_state *xs,
 static void bond_ipsec_add_sa_all(struct bonding *bond)
 {
 	struct net_device *bond_dev = bond->dev;
+	struct net_device *real_dev;
 	struct bond_ipsec *ipsec;
 	struct slave *slave;
 

@@ -483,12 +486,13 @@ static void bond_ipsec_add_sa_all(struct bonding *bond)
 	if (!slave)
 		goto out;
 
-	if (!slave->dev->xfrmdev_ops ||
-	    !slave->dev->xfrmdev_ops->xdo_dev_state_add ||
-	    netif_is_bond_master(slave->dev)) {
+	real_dev = slave->dev;
+	if (!real_dev->xfrmdev_ops ||
+	    !real_dev->xfrmdev_ops->xdo_dev_state_add ||
+	    netif_is_bond_master(real_dev)) {
 		spin_lock_bh(&bond->ipsec_lock);
 		if (!list_empty(&bond->ipsec_list))
-			slave_warn(bond_dev, slave->dev,
+			slave_warn(bond_dev, real_dev,
 				   "%s: no slave xdo_dev_state_add\n",
 				   __func__);
 		spin_unlock_bh(&bond->ipsec_lock);

@@ -497,9 +501,9 @@ static void bond_ipsec_add_sa_all(struct bonding *bond)
 
 	spin_lock_bh(&bond->ipsec_lock);
 	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
-		ipsec->xs->xso.real_dev = slave->dev;
-		if (slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL)) {
-			slave_warn(bond_dev, slave->dev, "%s: failed to add SA\n", __func__);
+		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;
 		}
 	}

@@ -515,6 +519,7 @@ static void bond_ipsec_add_sa_all(struct bonding *bond)
 static void bond_ipsec_del_sa(struct xfrm_state *xs)
 {
 	struct net_device *bond_dev = xs->xso.dev;
+	struct net_device *real_dev;
 	struct bond_ipsec *ipsec;
 	struct bonding *bond;
 	struct slave *slave;

@@ -532,16 +537,17 @@ static void bond_ipsec_del_sa(struct xfrm_state *xs)
 	if (!xs->xso.real_dev)
 		goto out;
 
-	WARN_ON(xs->xso.real_dev != slave->dev);
+	real_dev = slave->dev;
+	WARN_ON(xs->xso.real_dev != real_dev);
 
-	if (!slave->dev->xfrmdev_ops ||
-	    !slave->dev->xfrmdev_ops->xdo_dev_state_delete ||
-	    netif_is_bond_master(slave->dev)) {
-		slave_warn(bond_dev, slave->dev, "%s: no slave xdo_dev_state_delete\n", __func__);
+	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__);
 		goto out;
 	}
 
-	slave->dev->xfrmdev_ops->xdo_dev_state_delete(xs);
+	real_dev->xfrmdev_ops->xdo_dev_state_delete(xs);
 out:
 	spin_lock_bh(&bond->ipsec_lock);
 	list_for_each_entry(ipsec, &bond->ipsec_list, list) {

@@ -558,6 +564,7 @@ static void bond_ipsec_del_sa(struct xfrm_state *xs)
 static void bond_ipsec_del_sa_all(struct bonding *bond)
 {
 	struct net_device *bond_dev = bond->dev;
+	struct net_device *real_dev;
 	struct bond_ipsec *ipsec;
 	struct slave *slave;
 

@@ -568,21 +575,22 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
 		return;
 	}
 
+	real_dev = slave->dev;
 	spin_lock_bh(&bond->ipsec_lock);
 	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
 		if (!ipsec->xs->xso.real_dev)
 			continue;
 
-		if (!slave->dev->xfrmdev_ops ||
-		    !slave->dev->xfrmdev_ops->xdo_dev_state_delete ||
-		    netif_is_bond_master(slave->dev)) {
-			slave_warn(bond_dev, slave->dev,
+		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__);
 		} else {
-			slave->dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
-			if (slave->dev->xfrmdev_ops->xdo_dev_state_free)
-				slave->dev->xfrmdev_ops->xdo_dev_state_free(ipsec->xs);
+			real_dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
+			if (real_dev->xfrmdev_ops->xdo_dev_state_free)
+				real_dev->xfrmdev_ops->xdo_dev_state_free(ipsec->xs);
 		}
 		ipsec->xs->xso.real_dev = NULL;
 	}

-- 
2.44.0

From: Jianbo Liu <redacted>

In the cited commit, bond->ipsec_lock is added to protect ipsec_list,
hence xdo_dev_state_add and xdo_dev_state_delete are called inside
this lock. As ipsec_lock is a spin lock and such xfrmdev ops may sleep,
"scheduling while atomic" will be triggered when changing bond's
active slave.

[  101.055189] BUG: scheduling while atomic: bash/902/0x00000200
[  101.055726] Modules linked in:
[  101.058211] CPU: 3 PID: 902 Comm: bash Not tainted 6.9.0-rc4+ #1
[  101.058760] Hardware name:
[  101.059434] Call Trace:
[  101.059436]  <TASK>
[  101.060873]  dump_stack_lvl+0x51/0x60
[  101.061275]  __schedule_bug+0x4e/0x60
[  101.061682]  __schedule+0x612/0x7c0
[  101.062078]  ? __mod_timer+0x25c/0x370
[  101.062486]  schedule+0x25/0xd0
[  101.062845]  schedule_timeout+0x77/0xf0
[  101.063265]  ? asm_common_interrupt+0x22/0x40
[  101.063724]  ? __bpf_trace_itimer_state+0x10/0x10
[  101.064215]  __wait_for_common+0x87/0x190
[  101.064648]  ? usleep_range_state+0x90/0x90
[  101.065091]  cmd_exec+0x437/0xb20 [mlx5_core]
[  101.065569]  mlx5_cmd_do+0x1e/0x40 [mlx5_core]
[  101.066051]  mlx5_cmd_exec+0x18/0x30 [mlx5_core]
[  101.066552]  mlx5_crypto_create_dek_key+0xea/0x120 [mlx5_core]
[  101.067163]  ? bonding_sysfs_store_option+0x4d/0x80 [bonding]
[  101.067738]  ? kmalloc_trace+0x4d/0x350
[  101.068156]  mlx5_ipsec_create_sa_ctx+0x33/0x100 [mlx5_core]
[  101.068747]  mlx5e_xfrm_add_state+0x47b/0xaa0 [mlx5_core]
[  101.069312]  bond_change_active_slave+0x392/0x900 [bonding]
[  101.069868]  bond_option_active_slave_set+0x1c2/0x240 [bonding]
[  101.070454]  __bond_opt_set+0xa6/0x430 [bonding]
[  101.070935]  __bond_opt_set_notify+0x2f/0x90 [bonding]
[  101.071453]  bond_opt_tryset_rtnl+0x72/0xb0 [bonding]
[  101.071965]  bonding_sysfs_store_option+0x4d/0x80 [bonding]
[  101.072567]  kernfs_fop_write_iter+0x10c/0x1a0
[  101.073033]  vfs_write+0x2d8/0x400
[  101.073416]  ? alloc_fd+0x48/0x180
[  101.073798]  ksys_write+0x5f/0xe0
[  101.074175]  do_syscall_64+0x52/0x110
[  101.074576]  entry_SYSCALL_64_after_hwframe+0x4b/0x53

As bond_ipsec_add_sa_all and bond_ipsec_del_sa_all are only called
from bond_change_active_slave, which requires holding the RTNL lock.
And bond_ipsec_add_sa and bond_ipsec_del_sa are xfrm state
xdo_dev_state_add and xdo_dev_state_delete APIs, which are in user
context. So ipsec_lock doesn't have to be spin lock, change it to
mutex, and thus the above issue can be resolved.

Fixes: 9a5605505d9c ("bonding: Add struct bond_ipesc to manage SA")
Signed-off-by: Jianbo Liu <redacted>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/bonding/bond_main.c | 75 +++++++++++++++++----------------
 include/net/bonding.h           |  2 +-
 2 files changed, 40 insertions(+), 37 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e550b1c08fdb..56764f1c39b8 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c

@@ -439,38 +439,33 @@ static int bond_ipsec_add_sa(struct xfrm_state *xs,
 	rcu_read_lock();
 	bond = netdev_priv(bond_dev);
 	slave = rcu_dereference(bond->curr_active_slave);
-	if (!slave) {
-		rcu_read_unlock();
+	real_dev = slave ? slave->dev : NULL;
+	rcu_read_unlock();
+	if (!real_dev)
 		return -ENODEV;
-	}
 
-	real_dev = slave->dev;
 	if (!real_dev->xfrmdev_ops ||
 	    !real_dev->xfrmdev_ops->xdo_dev_state_add ||
 	    netif_is_bond_master(real_dev)) {
 		NL_SET_ERR_MSG_MOD(extack, "Slave does not support ipsec offload");
-		rcu_read_unlock();
 		return -EINVAL;
 	}
 
-	ipsec = kmalloc(sizeof(*ipsec), GFP_ATOMIC);
-	if (!ipsec) {
-		rcu_read_unlock();
+	ipsec = kmalloc(sizeof(*ipsec), GFP_KERNEL);
+	if (!ipsec)
 		return -ENOMEM;
-	}
 
 	xs->xso.real_dev = real_dev;
 	err = real_dev->xfrmdev_ops->xdo_dev_state_add(xs, extack);
 	if (!err) {
 		ipsec->xs = xs;
 		INIT_LIST_HEAD(&ipsec->list);
-		spin_lock_bh(&bond->ipsec_lock);
+		mutex_lock(&bond->ipsec_lock);
 		list_add(&ipsec->list, &bond->ipsec_list);
-		spin_unlock_bh(&bond->ipsec_lock);
+		mutex_unlock(&bond->ipsec_lock);
 	} else {
 		kfree(ipsec);
 	}
-	rcu_read_unlock();
 	return err;
 }
 

@@ -481,35 +476,43 @@ static void bond_ipsec_add_sa_all(struct bonding *bond)
 	struct bond_ipsec *ipsec;
 	struct slave *slave;
 
-	rcu_read_lock();
-	slave = rcu_dereference(bond->curr_active_slave);
-	if (!slave)
-		goto out;
+	slave = rtnl_dereference(bond->curr_active_slave);
+	real_dev = slave ? slave->dev : NULL;
+	if (!real_dev)
+		return;
 
-	real_dev = slave->dev;
+	mutex_lock(&bond->ipsec_lock);
 	if (!real_dev->xfrmdev_ops ||
 	    !real_dev->xfrmdev_ops->xdo_dev_state_add ||
 	    netif_is_bond_master(real_dev)) {
-		spin_lock_bh(&bond->ipsec_lock);
 		if (!list_empty(&bond->ipsec_list))
 			slave_warn(bond_dev, real_dev,
 				   "%s: no slave xdo_dev_state_add\n",
 				   __func__);
-		spin_unlock_bh(&bond->ipsec_lock);
 		goto out;
 	}
 
-	spin_lock_bh(&bond->ipsec_lock);
 	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
+		struct net_device *dev = ipsec->xs->xso.real_dev;
+
+		/* If new state is added before ipsec_lock acquired */
+		if (dev) {
+			if (dev == real_dev)
+				continue;
+
+			dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
+			if (dev->xfrmdev_ops->xdo_dev_state_free)
+				dev->xfrmdev_ops->xdo_dev_state_free(ipsec->xs);
+		}
+
 		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;
 		}
 	}
-	spin_unlock_bh(&bond->ipsec_lock);
 out:
-	rcu_read_unlock();
+	mutex_unlock(&bond->ipsec_lock);
 }
 
 /**

@@ -530,6 +533,8 @@ static void bond_ipsec_del_sa(struct xfrm_state *xs)
 	rcu_read_lock();
 	bond = netdev_priv(bond_dev);
 	slave = rcu_dereference(bond->curr_active_slave);
+	real_dev = slave ? slave->dev : NULL;
+	rcu_read_unlock();
 
 	if (!slave)
 		goto out;

@@ -537,7 +542,6 @@ static void bond_ipsec_del_sa(struct xfrm_state *xs)
 	if (!xs->xso.real_dev)
 		goto out;
 
-	real_dev = slave->dev;
 	WARN_ON(xs->xso.real_dev != real_dev);
 
 	if (!real_dev->xfrmdev_ops ||

@@ -549,7 +553,7 @@ static void bond_ipsec_del_sa(struct xfrm_state *xs)
 
 	real_dev->xfrmdev_ops->xdo_dev_state_delete(xs);
 out:
-	spin_lock_bh(&bond->ipsec_lock);
+	mutex_lock(&bond->ipsec_lock);
 	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
 		if (ipsec->xs == xs) {
 			list_del(&ipsec->list);

@@ -557,8 +561,7 @@ static void bond_ipsec_del_sa(struct xfrm_state *xs)
 			break;
 		}
 	}
-	spin_unlock_bh(&bond->ipsec_lock);
-	rcu_read_unlock();
+	mutex_unlock(&bond->ipsec_lock);
 }
 
 static void bond_ipsec_del_sa_all(struct bonding *bond)

@@ -568,15 +571,12 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
 	struct bond_ipsec *ipsec;
 	struct slave *slave;
 
-	rcu_read_lock();
-	slave = rcu_dereference(bond->curr_active_slave);
-	if (!slave) {
-		rcu_read_unlock();
+	slave = rtnl_dereference(bond->curr_active_slave);
+	real_dev = slave ? slave->dev : NULL;
+	if (!real_dev)
 		return;
-	}
 
-	real_dev = slave->dev;
-	spin_lock_bh(&bond->ipsec_lock);
+	mutex_lock(&bond->ipsec_lock);
 	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
 		if (!ipsec->xs->xso.real_dev)
 			continue;

@@ -594,8 +594,7 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
 		}
 		ipsec->xs->xso.real_dev = NULL;
 	}
-	spin_unlock_bh(&bond->ipsec_lock);
-	rcu_read_unlock();
+	mutex_unlock(&bond->ipsec_lock);
 }
 
 static void bond_ipsec_free_sa(struct xfrm_state *xs)

@@ -5922,7 +5921,7 @@ void bond_setup(struct net_device *bond_dev)
 	/* set up xfrm device ops (only supported in active-backup right now) */
 	bond_dev->xfrmdev_ops = &bond_xfrmdev_ops;
 	INIT_LIST_HEAD(&bond->ipsec_list);
-	spin_lock_init(&bond->ipsec_lock);
+	mutex_init(&bond->ipsec_lock);
 #endif /* CONFIG_XFRM_OFFLOAD */
 
 	/* don't acquire bond device's netif_tx_lock when transmitting */

@@ -5971,6 +5970,10 @@ static void bond_uninit(struct net_device *bond_dev)
 		__bond_release_one(bond_dev, slave->dev, true, true);
 	netdev_info(bond_dev, "Released all slaves\n");
 
+#ifdef CONFIG_XFRM_OFFLOAD
+	mutex_destroy(&bond->ipsec_lock);
+#endif /* CONFIG_XFRM_OFFLOAD */
+
 	bond_set_slave_arr(bond, NULL, NULL);
 
 	list_del_rcu(&bond->bond_list);

diff --git a/include/net/bonding.h b/include/net/bonding.h
index b61fb1aa3a56..8bb5f016969f 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h

@@ -260,7 +260,7 @@ struct bonding {
 #ifdef CONFIG_XFRM_OFFLOAD
 	struct list_head ipsec_list;
 	/* protecting ipsec_list */
-	spinlock_t ipsec_lock;
+	struct mutex ipsec_lock;
 #endif /* CONFIG_XFRM_OFFLOAD */
 	struct bpf_prog *xdp_prog;
 };

-- 
2.44.0

On Mon, Aug 05, 2024 at 08:03:55AM +0300, Tariq Toukan wrote:

Do we need to check netif_is_bond_master(slave->dev) here?

Thanks
Hangbin

On Tue, 2024-08-06 at 16:45 +0800, Hangbin Liu wrote:

On Mon, Aug 05, 2024 at 08:03:55AM +0300, Tariq Toukan wrote:

quoted

From: Jianbo Liu <redacted>

Add this implementation for bonding, so hardware resources can be
freed after xfrm state is deleted.

And call it when deleting all SAs from old active real interface.

Fixes: 9a5605505d9c ("bonding: Add struct bond_ipesc to manage SA")
Signed-off-by: Jianbo Liu <redacted>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/bonding/bond_main.c | 32
++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c

b/drivers/net/bonding/bond_main.c
index 1cd92c12e782..eb5e43860670 100644

--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c

@@ -581,6 +581,8 @@ static void bond_ipsec_del_sa_all(struct

bonding *bond)
                                   __func__);
                } else {
                        slave->dev->xfrmdev_ops-

quoted

xdo_dev_state_delete(ipsec->xs);

+                       if (slave->dev->xfrmdev_ops-

quoted

xdo_dev_state_free)

+                               slave->dev->xfrmdev_ops-

quoted

xdo_dev_state_free(ipsec->xs);

                }
                ipsec->xs->xso.real_dev = NULL;
        }

@@ -588,6 +590,35 @@ static void bond_ipsec_del_sa_all(struct

bonding *bond)
        rcu_read_unlock();
 }
 
+static void bond_ipsec_free_sa(struct xfrm_state *xs)
+{
+       struct net_device *bond_dev = xs->xso.dev;
+       struct net_device *real_dev;
+       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;
+       rcu_read_unlock();
+
+       if (!slave)
+               return;
+
+       if (!xs->xso.real_dev)
+               return;
+
+       WARN_ON(xs->xso.real_dev != real_dev);
+
+       if (real_dev && real_dev->xfrmdev_ops &&
+           real_dev->xfrmdev_ops->xdo_dev_state_free)
+               real_dev->xfrmdev_ops->xdo_dev_state_free(xs);

Do we need to check netif_is_bond_master(slave->dev) here?

Probably no need, because we will not call xdo_dev_state_free only. It
is called after xdo_dev_state_delete.

Thanks
Hangbin

On Mon, Aug 05, 2024 at 08:03:55AM +0300, Tariq Toukan wrote:

Reviewed-by: Hangbin Liu <redacted>

On Mon, Aug 05, 2024 at 08:03:56AM +0300, Tariq Toukan wrote:

Reviewed-by: Hangbin Liu <redacted>

On Mon, Aug 05, 2024 at 08:03:57AM +0300, Tariq Toukan wrote:

Hi Jianbo,

Why we skip the deleting here if dev == real_dev? What if the state
is added again on the same slave? From the previous logic it looks we
don't check and do over write for the same device.

Thanks
Hangbin

+			dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
+			if (dev->xfrmdev_ops->xdo_dev_state_free)
+				dev->xfrmdev_ops->xdo_dev_state_free(ipsec->xs);
+		}
+
 		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;
 		}
 	}
-	spin_unlock_bh(&bond->ipsec_lock);
 out:
-	rcu_read_unlock();
+	mutex_unlock(&bond->ipsec_lock);
 }

On Thu, 2024-08-08 at 17:34 +0800, Hangbin Liu wrote:

On Mon, Aug 05, 2024 at 08:03:57AM +0300, Tariq Toukan wrote:

quoted

From: Jianbo Liu <redacted>

In the cited commit, bond->ipsec_lock is added to protect
ipsec_list,
hence xdo_dev_state_add and xdo_dev_state_delete are called inside
this lock. As ipsec_lock is a spin lock and such xfrmdev ops may
sleep,
"scheduling while atomic" will be triggered when changing bond's
active slave.

[  101.055189] BUG: scheduling while atomic: bash/902/0x00000200
[  101.055726] Modules linked in:
[  101.058211] CPU: 3 PID: 902 Comm: bash Not tainted 6.9.0-rc4+ #1
[  101.058760] Hardware name:
[  101.059434] Call Trace:
[  101.059436]  <TASK>
[  101.060873]  dump_stack_lvl+0x51/0x60
[  101.061275]  __schedule_bug+0x4e/0x60
[  101.061682]  __schedule+0x612/0x7c0
[  101.062078]  ? __mod_timer+0x25c/0x370
[  101.062486]  schedule+0x25/0xd0
[  101.062845]  schedule_timeout+0x77/0xf0
[  101.063265]  ? asm_common_interrupt+0x22/0x40
[  101.063724]  ? __bpf_trace_itimer_state+0x10/0x10
[  101.064215]  __wait_for_common+0x87/0x190
[  101.064648]  ? usleep_range_state+0x90/0x90
[  101.065091]  cmd_exec+0x437/0xb20 [mlx5_core]
[  101.065569]  mlx5_cmd_do+0x1e/0x40 [mlx5_core]
[  101.066051]  mlx5_cmd_exec+0x18/0x30 [mlx5_core]
[  101.066552]  mlx5_crypto_create_dek_key+0xea/0x120 [mlx5_core]
[  101.067163]  ? bonding_sysfs_store_option+0x4d/0x80 [bonding]
[  101.067738]  ? kmalloc_trace+0x4d/0x350
[  101.068156]  mlx5_ipsec_create_sa_ctx+0x33/0x100 [mlx5_core]
[  101.068747]  mlx5e_xfrm_add_state+0x47b/0xaa0 [mlx5_core]
[  101.069312]  bond_change_active_slave+0x392/0x900 [bonding]
[  101.069868]  bond_option_active_slave_set+0x1c2/0x240 [bonding]
[  101.070454]  __bond_opt_set+0xa6/0x430 [bonding]
[  101.070935]  __bond_opt_set_notify+0x2f/0x90 [bonding]
[  101.071453]  bond_opt_tryset_rtnl+0x72/0xb0 [bonding]
[  101.071965]  bonding_sysfs_store_option+0x4d/0x80 [bonding]
[  101.072567]  kernfs_fop_write_iter+0x10c/0x1a0
[  101.073033]  vfs_write+0x2d8/0x400
[  101.073416]  ? alloc_fd+0x48/0x180
[  101.073798]  ksys_write+0x5f/0xe0
[  101.074175]  do_syscall_64+0x52/0x110
[  101.074576]  entry_SYSCALL_64_after_hwframe+0x4b/0x53

As bond_ipsec_add_sa_all and bond_ipsec_del_sa_all are only called
from bond_change_active_slave, which requires holding the RTNL
lock.
And bond_ipsec_add_sa and bond_ipsec_del_sa are xfrm state
xdo_dev_state_add and xdo_dev_state_delete APIs, which are in user
context. So ipsec_lock doesn't have to be spin lock, change it to
mutex, and thus the above issue can be resolved.

Fixes: 9a5605505d9c ("bonding: Add struct bond_ipesc to manage SA")
Signed-off-by: Jianbo Liu <redacted>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/bonding/bond_main.c | 75 +++++++++++++++++------------
----
 include/net/bonding.h           |  2 +-
 2 files changed, 40 insertions(+), 37 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c

b/drivers/net/bonding/bond_main.c
index e550b1c08fdb..56764f1c39b8 100644

--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c

@@ -481,35 +476,43 @@ static void bond_ipsec_add_sa_all(struct

bonding *bond)
        struct bond_ipsec *ipsec;
        struct slave *slave;
 
-       rcu_read_lock();
-       slave = rcu_dereference(bond->curr_active_slave);
-       if (!slave)
-               goto out;
+       slave = rtnl_dereference(bond->curr_active_slave);
+       real_dev = slave ? slave->dev : NULL;
+       if (!real_dev)
+               return;
 
-       real_dev = slave->dev;
+       mutex_lock(&bond->ipsec_lock);
        if (!real_dev->xfrmdev_ops ||
            !real_dev->xfrmdev_ops->xdo_dev_state_add ||
            netif_is_bond_master(real_dev)) {
-               spin_lock_bh(&bond->ipsec_lock);
                if (!list_empty(&bond->ipsec_list))
                        slave_warn(bond_dev, real_dev,
                                   "%s: no slave
xdo_dev_state_add\n",
                                   __func__);
-               spin_unlock_bh(&bond->ipsec_lock);
                goto out;
        }
 
-       spin_lock_bh(&bond->ipsec_lock);
        list_for_each_entry(ipsec, &bond->ipsec_list, list) {
+               struct net_device *dev = ipsec->xs->xso.real_dev;
+
+               /* If new state is added before ipsec_lock acquired
*/
+               if (dev) {
+                       if (dev == real_dev)
+                               continue;

Hi Jianbo,

Why we skip the deleting here if dev == real_dev? What if the state

Here the bond active slave is updated. If dev == real_dev, the state
(should be newly added) is offloaded to new active, so no need to
delete and add back again.  

is added again on the same slave? From the previous logic it looks we

Why is it added to the same slave? It's not the active one.

don't check and do over write for the same device.

Thanks
Hangbin

quoted

+                       dev->xfrmdev_ops-

quoted

xdo_dev_state_delete(ipsec->xs);

+                       if (dev->xfrmdev_ops->xdo_dev_state_free)
+                               dev->xfrmdev_ops-

quoted

xdo_dev_state_free(ipsec->xs);

+               }
+
                ipsec->xs->xso.real_dev = real_dev;
                if (real_dev->xfrmdev_ops->xdo_dev_state_add(ipsec-

quoted

xs, NULL)) {

                        slave_warn(bond_dev, real_dev, "%s: failed
to add SA\n", __func__);
                        ipsec->xs->xso.real_dev = NULL;
                }
        }
-       spin_unlock_bh(&bond->ipsec_lock);
 out:
-       rcu_read_unlock();
+       mutex_unlock(&bond->ipsec_lock);
 }

On Thu, Aug 08, 2024 at 10:05:26AM +0000, Jianbo Liu wrote:

On Thu, 2024-08-08 at 17:34 +0800, Hangbin Liu wrote:

quoted

On Mon, Aug 05, 2024 at 08:03:57AM +0300, Tariq Toukan wrote:

quoted

 drivers/net/bonding/bond_main.c | 75 +++++++++++++++++------------
----
 include/net/bonding.h           |  2 +-
 2 files changed, 40 insertions(+), 37 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c

b/drivers/net/bonding/bond_main.c
index e550b1c08fdb..56764f1c39b8 100644

--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c

@@ -481,35 +476,43 @@ static void bond_ipsec_add_sa_all(struct

bonding *bond)
        struct bond_ipsec *ipsec;
        struct slave *slave;
 
-       rcu_read_lock();
-       slave = rcu_dereference(bond->curr_active_slave);
-       if (!slave)
-               goto out;
+       slave = rtnl_dereference(bond->curr_active_slave);
+       real_dev = slave ? slave->dev : NULL;
+       if (!real_dev)
+               return;
 
-       real_dev = slave->dev;
+       mutex_lock(&bond->ipsec_lock);
        if (!real_dev->xfrmdev_ops ||
            !real_dev->xfrmdev_ops->xdo_dev_state_add ||
            netif_is_bond_master(real_dev)) {
-               spin_lock_bh(&bond->ipsec_lock);
                if (!list_empty(&bond->ipsec_list))
                        slave_warn(bond_dev, real_dev,
                                   "%s: no slave
xdo_dev_state_add\n",
                                   __func__);
-               spin_unlock_bh(&bond->ipsec_lock);
                goto out;
        }
 
-       spin_lock_bh(&bond->ipsec_lock);
        list_for_each_entry(ipsec, &bond->ipsec_list, list) {
+               struct net_device *dev = ipsec->xs->xso.real_dev;
+
+               /* If new state is added before ipsec_lock acquired
*/
+               if (dev) {
+                       if (dev == real_dev)
+                               continue;

Hi Jianbo,

Why we skip the deleting here if dev == real_dev? What if the state

Here the bond active slave is updated. If dev == real_dev, the state
(should be newly added) is offloaded to new active, so no need to
delete and add back again.  

quoted

is added again on the same slave? From the previous logic it looks we

Why is it added to the same slave? It's not the active one.

OK, I got what you mean now. Thanks for the explaination.

Reviewed-by: Hangbin Liu <redacted>

On Mon, 5 Aug 2024 08:03:55 +0300 Tariq Toukan wrote:

+static void bond_ipsec_free_sa(struct xfrm_state *xs)
+{
+	struct net_device *bond_dev = xs->xso.dev;
+	struct net_device *real_dev;
+	struct bonding *bond;
+	struct slave *slave;
+
+	if (!bond_dev)
+		return;

can xs->xso.dev be NULL during the dev_free_state callback?

+	rcu_read_lock();
+	bond = netdev_priv(bond_dev);
+	slave = rcu_dereference(bond->curr_active_slave);
+	real_dev = slave ? slave->dev : NULL;
+	rcu_read_unlock();

What's holding onto real_dev once you drop the rcu lock here?

On Mon, 2024-08-12 at 17:48 -0700, Jakub Kicinski wrote:

On Mon, 5 Aug 2024 08:03:55 +0300 Tariq Toukan wrote:

quoted

+static void bond_ipsec_free_sa(struct xfrm_state *xs)
+{
+       struct net_device *bond_dev = xs->xso.dev;
+       struct net_device *real_dev;
+       struct bonding *bond;
+       struct slave *slave;
+
+       if (!bond_dev)
+               return;

can xs->xso.dev be NULL during the dev_free_state callback?

Shouldn't be NULL because bond_ipsec_del_sa is called before and xs is
removed from bond->ipsec_list, so xs->xso.dev is kept unless it's
cleared in dev's xdo_dev_state_delete callback.

quoted

+       rcu_read_lock();
+       bond = netdev_priv(bond_dev);
+       slave = rcu_dereference(bond->curr_active_slave);
+       real_dev = slave ? slave->dev : NULL;
+       rcu_read_unlock();

What's holding onto real_dev once you drop the rcu lock here?

I think it should be xfrm state (and bond device).

Thanks!
Jianbo

On Tue, 13 Aug 2024 02:58:12 +0000 Jianbo Liu wrote:

quoted

quoted

+       rcu_read_lock();
+       bond = netdev_priv(bond_dev);
+       slave = rcu_dereference(bond->curr_active_slave);
+       real_dev = slave ? slave->dev : NULL;
+       rcu_read_unlock();  

What's holding onto real_dev once you drop the rcu lock here?  

I think it should be xfrm state (and bond device).

Please explain it in the commit message in more certain terms.

On Tue, 2024-08-13 at 07:14 -0700, Jakub Kicinski wrote:

On Tue, 13 Aug 2024 02:58:12 +0000 Jianbo Liu wrote:

quoted

quoted

quoted

+       rcu_read_lock();
+       bond = netdev_priv(bond_dev);
+       slave = rcu_dereference(bond->curr_active_slave);
+       real_dev = slave ? slave->dev : NULL;
+       rcu_read_unlock();  

What's holding onto real_dev once you drop the rcu lock here?  

I think it should be xfrm state (and bond device).

Please explain it in the commit message in more certain terms.

Sorry, I don't understand. The real_dev is saved in xs->xso.real_dev,
and also bond's slave. It's straightforward. What else do I need to
explain?

Thanks!
Jianbo

On Wed, Aug 14, 2024 at 02:03:58AM +0000, Jianbo Liu wrote:

On Tue, 2024-08-13 at 07:14 -0700, Jakub Kicinski wrote:

quoted

On Tue, 13 Aug 2024 02:58:12 +0000 Jianbo Liu wrote:

quoted

quoted

quoted

+       rcu_read_lock();
+       bond = netdev_priv(bond_dev);
+       slave = rcu_dereference(bond->curr_active_slave);
+       real_dev = slave ? slave->dev : NULL;
+       rcu_read_unlock();  

What's holding onto real_dev once you drop the rcu lock here?  

I think it should be xfrm state (and bond device).

Please explain it in the commit message in more certain terms.

Sorry, I don't understand. The real_dev is saved in xs->xso.real_dev,
and also bond's slave. It's straightforward. What else do I need to
explain?

I think Jakub means you need to make sure the real_dev is not freed during
xfrmdev_ops. See bond_ipsec_add_sa(). You unlock it too early and later
xfrmdev_ops is not protected.

Hangbin

On Wed, 2024-08-14 at 11:11 +0800, Hangbin Liu wrote:

On Wed, Aug 14, 2024 at 02:03:58AM +0000, Jianbo Liu wrote:

quoted

On Tue, 2024-08-13 at 07:14 -0700, Jakub Kicinski wrote:

quoted

On Tue, 13 Aug 2024 02:58:12 +0000 Jianbo Liu wrote:

quoted

quoted

quoted

+       rcu_read_lock();
+       bond = netdev_priv(bond_dev);
+       slave = rcu_dereference(bond->curr_active_slave);
+       real_dev = slave ? slave->dev : NULL;
+       rcu_read_unlock();  

What's holding onto real_dev once you drop the rcu lock
here?  

I think it should be xfrm state (and bond device).

Please explain it in the commit message in more certain terms.

Sorry, I don't understand. The real_dev is saved in xs-

quoted

xso.real_dev,

and also bond's slave. It's straightforward. What else do I need to
explain?

I think Jakub means you need to make sure the real_dev is not freed
during
xfrmdev_ops. See bond_ipsec_add_sa(). You unlock it too early and
later
xfrmdev_ops is not protected.

This RCU lock is to protect the reading of curr_active_slave, which is
pointing to a big stuct - slave struct, so there is no error to get
real_dev from slave->dev.

Thanks!
Jianbo

On Thu, Aug 15, 2024 at 01:23:05AM +0000, Jianbo Liu wrote:

On Wed, 2024-08-14 at 11:11 +0800, Hangbin Liu wrote:

quoted

On Wed, Aug 14, 2024 at 02:03:58AM +0000, Jianbo Liu wrote:

quoted

On Tue, 2024-08-13 at 07:14 -0700, Jakub Kicinski wrote:

quoted

On Tue, 13 Aug 2024 02:58:12 +0000 Jianbo Liu wrote:

quoted

quoted

quoted

+       rcu_read_lock();
+       bond = netdev_priv(bond_dev);
+       slave = rcu_dereference(bond->curr_active_slave);
+       real_dev = slave ? slave->dev : NULL;
+       rcu_read_unlock();  

What's holding onto real_dev once you drop the rcu lock
here?  

I think it should be xfrm state (and bond device).

Please explain it in the commit message in more certain terms.

Sorry, I don't understand. The real_dev is saved in xs-

quoted

xso.real_dev,

and also bond's slave. It's straightforward. What else do I need to
explain?

I think Jakub means you need to make sure the real_dev is not freed
during
xfrmdev_ops. See bond_ipsec_add_sa(). You unlock it too early and
later
xfrmdev_ops is not protected.

This RCU lock is to protect the reading of curr_active_slave, which is
pointing to a big stuct - slave struct, so there is no error to get
real_dev from slave->dev.

It's not about getting real_dev from slave->dev. As Jakub said, What's holding
on real_dev once you drop the rcu lock?

Thanks
Hangbin

On Thu, 2024-08-15 at 11:34 +0800, Hangbin Liu wrote:

On Thu, Aug 15, 2024 at 01:23:05AM +0000, Jianbo Liu wrote:

quoted

On Wed, 2024-08-14 at 11:11 +0800, Hangbin Liu wrote:

quoted

On Wed, Aug 14, 2024 at 02:03:58AM +0000, Jianbo Liu wrote:

quoted

On Tue, 2024-08-13 at 07:14 -0700, Jakub Kicinski wrote:

quoted

On Tue, 13 Aug 2024 02:58:12 +0000 Jianbo Liu wrote:

quoted

quoted

quoted

+       rcu_read_lock();
+       bond = netdev_priv(bond_dev);
+       slave = rcu_dereference(bond-

quoted

curr_active_slave);

+       real_dev = slave ? slave->dev : NULL;
+       rcu_read_unlock();  

What's holding onto real_dev once you drop the rcu lock
here?  

I think it should be xfrm state (and bond device).

Please explain it in the commit message in more certain
terms.

Sorry, I don't understand. The real_dev is saved in xs-

quoted

xso.real_dev,

and also bond's slave. It's straightforward. What else do I
need to
explain?

I think Jakub means you need to make sure the real_dev is not
freed
during
xfrmdev_ops. See bond_ipsec_add_sa(). You unlock it too early and
later
xfrmdev_ops is not protected.

This RCU lock is to protect the reading of curr_active_slave, which
is
pointing to a big stuct - slave struct, so there is no error to get
real_dev from slave->dev.

It's not about getting real_dev from slave->dev. As Jakub said,
What's holding
on real_dev once you drop the rcu lock?

As you mentioned the lock, I explained what's it used for, so we will
not mix basic concepts and make things complicated. 
As for Jakub's question, I already answered. And I'm waiting for his
reply so I can better undestand how to modify if there is any. 

Thanks!
Jianbo