Re: [PATCH net-next 3/6] bonding: complete rollback of bond_enslave() error paths

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] bonding: RCU use-after-free of new_slave in bond_enslave()
  ALB/TLB unwind
- [Medium] bonding: bond_alb_deinit_slave() in bond_enslave() corrupts
  MACs and RLB
- [Medium] bonding: wrong bond_3ad_unbind_slave() ordering in
  bond_enslave() unwind
--

commit fe641519a15c6edad3aff89eac3b2de3f443285d
Author: Hangbin Liu <hangbin.liu@linux.dev>

bonding: complete rollback of bond_enslave() error paths

This commit adds rollback steps in bond_enslave() to unbind 802.3ad slaves,
deinitialize ALB/TLB slave state, and restore the fail_over_mac setting on
error.

> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 80b6c18ee9c28..f696a0d150346 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c

[ ... ]

> @@ -2388,6 +2391,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>  	netdev_rx_handler_unregister(slave_dev);
>  
>  err_detach:
> +	if (BOND_MODE(bond) == BOND_MODE_8023AD)
> +		bond_3ad_unbind_slave(new_slave);
>  	vlan_vids_del_by_dev(slave_dev, bond_dev);
>  	if (rcu_access_pointer(bond->primary_slave) == new_slave)
>  		RCU_INIT_POINTER(bond->primary_slave, NULL);

[Severity: Medium]
Does calling bond_3ad_unbind_slave() at err_detach run too late when
new_slave was linked, and too early when enslavement failed before
bond_master_upper_dev_link()?

Unlike __bond_release_one(), which calls netdev_rx_handler_unregister() and
bond_3ad_unbind_slave() before bond_upper_dev_unlink(), bond_enslave()
unlinks new_slave from bond_slave_list(bond) at err_upper_unlink first.  If
new_slave is the only slave in an 802.3ad bond and
bond_3ad_state_machine_handler() activates its aggregator and raises
bond_dev's carrier before a later step such as dev_xdp_propagate() fails,
bond_upper_dev_unlink() empties the slave list before
bond_3ad_unbind_slave() runs.  Inside bond_3ad_unbind_slave(),
__get_first_agg(port) then returns NULL, skipping ad_agg_selection_logic()
and bond_3ad_set_carrier() and leaving carrier on with zero slaves.
Unlinking new_slave before netdev_rx_handler_unregister() also leaves the
active aggregator detached from bond_slave_list(bond) while
synchronize_net() sleeps.

Conversely, if bond_enslave() fails before bond_master_upper_dev_link()
(for example at netdev_rx_handler_register()), err_detach still calls
bond_3ad_unbind_slave(), which unconditionally calls ad_lacpdu_send(port)
and transmits an LACPDU on the unenslaved device.

> @@ -2402,6 +2407,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>  	slave_disable_netpoll(new_slave);
>  
>  err_close:
> +	if (alb_initialized)
> +		bond_alb_deinit_slave(bond, new_slave);
>  	if (!netif_is_bond_master(slave_dev))
>  		slave_dev->priv_flags &= ~IFF_BONDING;
>  	dev_close(slave_dev);

[Severity: Medium]
Can calling bond_alb_deinit_slave() after synchronize_rcu() cause a
use-after-free of new_slave?

Once bond_master_upper_dev_link() links new_slave into the bond, concurrent
TLB or ALB transmit paths can place new_slave into tx_hashtbl or
rx_hashtbl.  In __bond_release_one(), bond_alb_deinit_slave() clears those
hash tables before synchronize_rcu().  Here at err_close,
bond_alb_deinit_slave() runs after synchronize_rcu() at err_detach, with no
grace period before kobject_put(&new_slave->kobj) frees new_slave at
err_free:

CPU0                                  CPU1
bond_enslave()
  bond_master_upper_dev_link()
  ...
  goto err_set_xdp;
  bond_upper_dev_unlink()
  synchronize_rcu()
                                      rcu_read_lock()
                                      bond_do_alb_xmit()
                                        tlb_choose_channel()
                                          tx_slave = new_slave
  bond_alb_deinit_slave()
  ...
  kobject_put(&new_slave->kobj)
    kfree(new_slave)
                                        __tlb_clear_slave(bond, tx_slave)
                                          <-- use-after-free

[Severity: Medium]
Can calling bond_alb_deinit_slave() on an enslavement failure corrupt an
existing slave's MAC address or put the active slave into promiscuous mode?

bond_alb_init_slave() only updates new_slave->dev->dev_addr (and picks a
free permanent MAC if new_slave->perm_hwaddr collides with an existing
slave), while err_restore_mac already restores new_slave->dev->dev_addr to
new_slave->perm_hwaddr.  Because bond_select_active_slave() is only reached
after all error paths, no existing slave ever borrows new_slave's MAC
during bond_enslave().

When bond_alb_deinit_slave() calls alb_change_hw_addr_on_detach(), a
collision between new_slave->perm_hwaddr and an existing slave found_slave
causes it to swap new_slave's temporary MAC onto found_slave, overwriting
found_slave's legitimate MAC address:

drivers/net/bonding/bond_alb.c:alb_change_hw_addr_on_detach() {
    ...
	if (perm_curr_diff && perm_bond_diff) {
		found_slave = bond_slave_has_mac(bond, slave->perm_hwaddr);

		if (found_slave) {
			alb_swap_mac_addr(slave, found_slave);
			alb_fasten_mac_swap(bond, slave, found_slave);
		}
	}
}

In addition, when rlb_enabled is set, bond_alb_deinit_slave() calls
rlb_clear_slave().  Because new_slave is never curr_active_slave during
bond_enslave(), rlb_clear_slave() unconditionally calls
rlb_teach_disabled_mac_on_primary(bond, new_slave->dev->dev_addr), even if
bond_enslave() failed at vlan_vids_add_by_dev() before new_slave was linked
to the bond.  That puts curr_active_slave into promiscuous mode and sends
loopback learning packets with the unenslaved device's MAC address on
curr_active_slave.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-bond_enslave_restruct-v1-0-364c549e78db@kylinos.cn?part=3