Skip to content

Commit fd4e41e

Browse files
Cosmin Ratiuklassert
authored andcommitted
bonding: Mark active offloaded xfrm_states
When the active link is changed for a bond device, the existing xfrm states need to be migrated over to the new link. This is done with: - bond_ipsec_del_sa_all() goes through the offloaded states list and removes all of them from hw. - bond_ipsec_add_sa_all() re-offloads all states to the new device. But because the offload status of xfrm states isn't marked in any way, there can be bugs. When all bond links are down, bond_ipsec_del_sa_all() unoffloads everything from the previous active link. If the same link then comes back up, nothing gets reoffloaded by bond_ipsec_add_sa_all(). This results in a stack trace like this a bit later when user space removes the offloaded rules, because mlx5e_xfrm_del_state() is asked to remove a rule that's no longer offloaded: [] Call Trace: [] <TASK> [] ? __warn+0x7d/0x110 [] ? mlx5e_xfrm_del_state+0x90/0xa0 [mlx5_core] [] ? report_bug+0x16d/0x180 [] ? handle_bug+0x4f/0x90 [] ? exc_invalid_op+0x14/0x70 [] ? asm_exc_invalid_op+0x16/0x20 [] ? mlx5e_xfrm_del_state+0x73/0xa0 [mlx5_core] [] ? mlx5e_xfrm_del_state+0x90/0xa0 [mlx5_core] [] bond_ipsec_del_sa+0x1ab/0x200 [bonding] [] xfrm_dev_state_delete+0x1f/0x60 [] __xfrm_state_delete+0x196/0x200 [] xfrm_state_delete+0x21/0x40 [] xfrm_del_sa+0x69/0x110 [] xfrm_user_rcv_msg+0x11d/0x300 [] ? release_pages+0xca/0x140 [] ? copy_to_user_tmpl.part.0+0x110/0x110 [] netlink_rcv_skb+0x54/0x100 [] xfrm_netlink_rcv+0x31/0x40 [] netlink_unicast+0x1fc/0x2d0 [] netlink_sendmsg+0x1e4/0x410 [] __sock_sendmsg+0x38/0x60 [] sock_write_iter+0x94/0xf0 [] vfs_write+0x338/0x3f0 [] ksys_write+0xba/0xd0 [] do_syscall_64+0x4c/0x100 [] entry_SYSCALL_64_after_hwframe+0x4b/0x53 There's also another theoretical bug: Calling bond_ipsec_del_sa_all() multiple times can result in corruption in the driver implementation if the double-free isn't tolerated. This isn't nice. Before the "Fixes" commit, xs->xso.real_dev was set to NULL when an xfrm state was unoffloaded from a device, but a race with netdevsim's .xdo_dev_offload_ok() accessing real_dev was considered a sufficient reason to not set real_dev to NULL anymore. This unfortunately introduced the new bugs. Since .xdo_dev_offload_ok() was significantly refactored by [1] and there are no more users in the stack of xso.real_dev, that race is now gone and xs->xso.real_dev can now once again be used to represent which device (if any) currently holds the offloaded rule. Go one step further and set real_dev after add/before delete calls, to catch any future driver misuses of real_dev. [1] https://lore.kernel.org/netdev/[email protected]/ Fixes: f8cde98 ("bonding: fix xfrm real_dev null pointer dereference") Signed-off-by: Cosmin Ratiu <[email protected]> Reviewed-by: Leon Romanovsky <[email protected]> Reviewed-by: Nikolay Aleksandrov <[email protected]> Reviewed-by: Hangbin Liu <[email protected]> Tested-by: Hangbin Liu <[email protected]> Signed-off-by: Steffen Klassert <[email protected]>
1 parent 43eca05 commit fd4e41e

File tree

1 file changed

+5
-3
lines changed

1 file changed

+5
-3
lines changed

drivers/net/bonding/bond_main.c

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -496,9 +496,9 @@ static int bond_ipsec_add_sa(struct net_device *bond_dev,
496496
goto out;
497497
}
498498

499-
xs->xso.real_dev = real_dev;
500499
err = real_dev->xfrmdev_ops->xdo_dev_state_add(real_dev, xs, extack);
501500
if (!err) {
501+
xs->xso.real_dev = real_dev;
502502
ipsec->xs = xs;
503503
INIT_LIST_HEAD(&ipsec->list);
504504
mutex_lock(&bond->ipsec_lock);
@@ -540,12 +540,12 @@ static void bond_ipsec_add_sa_all(struct bonding *bond)
540540
if (ipsec->xs->xso.real_dev == real_dev)
541541
continue;
542542

543-
ipsec->xs->xso.real_dev = real_dev;
544543
if (real_dev->xfrmdev_ops->xdo_dev_state_add(real_dev,
545544
ipsec->xs, NULL)) {
546545
slave_warn(bond_dev, real_dev, "%s: failed to add SA\n", __func__);
547-
ipsec->xs->xso.real_dev = NULL;
546+
continue;
548547
}
548+
ipsec->xs->xso.real_dev = real_dev;
549549
}
550550
out:
551551
mutex_unlock(&bond->ipsec_lock);
@@ -629,6 +629,7 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
629629
__func__);
630630
continue;
631631
}
632+
ipsec->xs->xso.real_dev = NULL;
632633
real_dev->xfrmdev_ops->xdo_dev_state_delete(real_dev,
633634
ipsec->xs);
634635
if (real_dev->xfrmdev_ops->xdo_dev_state_free)
@@ -664,6 +665,7 @@ static void bond_ipsec_free_sa(struct net_device *bond_dev,
664665

665666
WARN_ON(xs->xso.real_dev != real_dev);
666667

668+
xs->xso.real_dev = NULL;
667669
if (real_dev && real_dev->xfrmdev_ops &&
668670
real_dev->xfrmdev_ops->xdo_dev_state_free)
669671
real_dev->xfrmdev_ops->xdo_dev_state_free(real_dev, xs);

0 commit comments

Comments
 (0)