On Wed, Feb 26, 2025 at 11:05:47AM +0000, Cosmin Ratiu wrote:
What do you think about this idea?
Thanks a lot for the comments. I also skipped the DEAD xs in add_sa_all. What about the patch like:
This is what I had in mind, thanks for proposing it. Maybe you should package it in a new submission with a proper title/etc.? I'll do the initial review here.
This is a draft patch and I think there may have something need to be fixed. So I just paste it here :)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index e45bba240cbc..0e4db43a833a 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -537,6 +537,12 @@ static void bond_ipsec_add_sa_all(struct bonding *bond) } list_for_each_entry(ipsec, &bond->ipsec_list, list) {
/* No need to handle DEAD XFRM, as it has alreadybeen
* deleted and will be freed later.*/Nit: Maybe rephrase that as "Skip dead xfrm states, they'll be freed later."
if (ipsec->xs->km.state == XFRM_STATE_DEAD)continue;/* If new state is added before ipsec_lock acquired */ if (ipsec->xs->xso.real_dev == real_dev) continue; @@ -592,15 +598,6 @@ static void bond_ipsec_del_sa(struct xfrm_state *xs) real_dev->xfrmdev_ops->xdo_dev_state_delete(xs); out: netdev_put(real_dev, &tracker);
- mutex_lock(&bond->ipsec_lock);
- list_for_each_entry(ipsec, &bond->ipsec_list, list) {
if (ipsec->xs == xs) {list_del(&ipsec->list);kfree(ipsec);break;}- }
- mutex_unlock(&bond->ipsec_lock);
} static void bond_ipsec_del_sa_all(struct bonding *bond) @@ -617,6 +614,12 @@ static void bond_ipsec_del_sa_all(struct bonding *bond) mutex_lock(&bond->ipsec_lock); list_for_each_entry(ipsec, &bond->ipsec_list, list) {
/* No need to handle DEAD XFRM, as it has alreadybeen
* deleted and will be freed later.*/if (ipsec->xs->km.state == XFRM_STATE_DEAD)continue;If this doesn't free dead entries now and bond_ipsec_add_sa_all is called soon after, the pending bond_ipsec_free_sa() call will then hit the WARN_ON(xs->xso.real_dev != real_dev) before attempting to call free on the wrong device. To fix that, these entries should be freed here and the WARN_ON in bond_ipsec_free_sa() should be converted to an if...goto out, so that bond_ipsec_free_sa() calls would hit one of these conditions:
- "if (!slave)", when no active device exists.
- "if (!xs->xso.real_dev)", when xdo_dev_state_add() failed.
- "if (xs->xso.real_dev != real_dev)", when a DEAD xs was already
freed by bond_ipsec_del_sa_all() migration to a new device. In all 3 cases, xdo_dev_state_free() shouldn't be called, only xs removed from the bond->ipsec list.
I hope I didn't miss any corner case.
Thumb up! Thanks a lot for your review and comments. You thought much more than me. During bonding testing, we also found a case that would trigger the WARN_ON(xs->xso.real_dev != real_dev).
If we create active-backup mode bonding and create ipsec tunnel over bonding device, then remove bonding device. There is a possibility that the bond call bond_ipsec_del_sa_all() to delete the ipsec state first, then change active slave to another interface.
At the same time, ipsec gc was called and then bond_ipsec_free_sa(). This will cause the xs->xso.real_dev != active_slave as the failover triggered. The call traces looks like:
[14504.421247] bond0: (slave enp23s0f1np1): Enslaving as a backup interface with an up link [14506.761933] mlx5_core 0000:17:00.0: lag map active ports: 1 [14506.767520] mlx5_core 0000:17:00.0: shared_fdb:0 mode:hash [14550.992133] bond0: (slave enp23s0f0np0): Releasing backup interface [14550.994150] mlx5_core 0000:17:00.0: lag map active ports: 1, 2 [14550.998407] bond0: (slave enp23s0f1np1): making interface the new active one [14551.013286] ------------[ cut here ]------------ [14551.017912] WARNING: CPU: 7 PID: 1537 at drivers/net/bonding/bond_main.c:664 bond_ipsec_free_sa+0x9b/0xa0 [bonding] [14551.117875] Unloaded tainted modules: bonding(E):33 fjes(E):1 padlock_aes(E):2 [last unloaded: bonding(E)] [14551.148449] CPU: 7 UID: 0 PID: 1537 Comm: kworker/7:2 Kdump: loaded Tainted: G E 6.13.0-rc7+ #5 [14551.158536] Tainted: [E]=UNSIGNED_MODULE [14551.162461] Hardware name: Dell Inc. PowerEdge R750/0WT8Y6, BIOS 1.5.4 12/17/2021 [14551.169941] Workqueue: events xfrm_state_gc_task [14551.174559] RIP: 0010:bond_ipsec_free_sa+0x9b/0xa0 [bonding] [14551.180227] Code: 8b 85 38 05 00 00 65 ff 08 5b 5d c3 cc cc cc cc 5b 5d e9 e8 e3 01 da e8 e3 e3 01 da 48 83 bb b0 02 00 00 00 74 e3 0f 0b eb df <0f> 0b eb b4 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 [14551.198972] RSP: 0018:ff61163a49eb3e00 EFLAGS: 00010287 [14551.204200] RAX: ff42be3fe4bd8000 RBX: ff42be3fa7359d40 RCX: 00000000802a0025 [14551.211336] RDX: ff42be4edc534280 RSI: 00000000fffffe00 RDI: ff42be4edc534280 [14551.218476] RBP: ff42be3f50128000 R08: 0000000000000000 R09: 0000000000000001 [14551.225606] R10: 00000000802a0025 R11: ff42be404d917f60 R12: ff42be5e7edb4e80 [14551.232740] R13: ff42be4edc534280 R14: ffffffff9db3db40 R15: 0000000000000000 [14551.239872] FS: 0000000000000000(0000) GS:ff42be5e7ed80000(0000) knlGS:0000000000000000 [14551.247957] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [14551.253704] CR2: 00007fff69f55df0 CR3: 0000001158a22002 CR4: 0000000000773ef0 [14551.260836] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [14551.267970] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [14551.275101] PKRU: 55555554 [14551.277814] Call Trace: [14551.280268] <TASK> [14551.282374] ? show_trace_log_lvl+0x1b0/0x2f0 [14551.286742] ? show_trace_log_lvl+0x1b0/0x2f0 [14551.291102] ? xfrm_dev_state_free+0x84/0xb0 [14551.295374] ? bond_ipsec_free_sa+0x9b/0xa0 [bonding] [14551.300435] ? __warn.cold+0x93/0xf4 [14551.304020] ? bond_ipsec_free_sa+0x9b/0xa0 [bonding] [14551.309076] ? report_bug+0xff/0x140 [14551.312662] ? handle_bug+0x53/0x90 [14551.316157] ? exc_invalid_op+0x17/0x70 [14551.319994] ? asm_exc_invalid_op+0x1a/0x20 [14551.324183] ? bond_ipsec_free_sa+0x9b/0xa0 [bonding] [14551.329242] xfrm_dev_state_free+0x84/0xb0 [14551.333343] ___xfrm_state_destroy+0xe3/0x160 [14551.337701] xfrm_state_gc_task+0x7a/0xb0 [14551.341713] process_one_work+0x174/0x330 [14551.345729] worker_thread+0x252/0x390 [14551.349487] ? __pfx_worker_thread+0x10/0x10 [14551.353761] kthread+0xcf/0x100 [14551.356908] ? __pfx_kthread+0x10/0x10 [14551.360668] ret_from_fork+0x31/0x50 [14551.364249] ? __pfx_kthread+0x10/0x10 [14551.368009] ret_from_fork_asm+0x1a/0x30 [14551.371943] </TASK> [14551.374136] ---[ end trace 0000000000000000 ]--- [14551.735092] bond0: (slave enp23s0f1np1): Releasing backup interface [14552.110577] bond0 (unregistering): Released all slaves
This seems like another situation that could not simply fit 3. "if (xs->xso.real_dev != real_dev), goto out. I'm not sure what's the xs->km.state should be during xfrm_state_gc_task(). Is it also set to XFRM_STATE_DEAD, because I didn't see it.
Especially if the bond change active slave and xfrm_state_gc_task() run in parallel, like
bond_ipsec_del_sa_all() xfrm_state_gc_task() xfrm_dev_state_free() bond_ipsec_free_sa() bond_ipsec_add_sa_all()
If the xs->km.state is not XFRM_STATE_DEAD. How to avoid the WARN_ON(xs->xso.real_dev != real_dev) in bond_ipsec_free_sa() and how to make bond_ipsec_add_sa_all() not added the entry again.
Thanks Hangbin