Re: [net] bonding: use skb_cow_head() in bond_do_alb_xmit() and rlb_arp_xmit()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] In `rlb_arp_xmit()` (drivers/net/bonding/bond_alb.c:681) the
  patch adds `tx_slave != rcu_access_pointer(bond->curr_active_slave)`…

--

bonding: use skb_cow_head() in bond_do_alb_xmit() and rlb_arp_xmit()

This adds skb_cow_head() before rewriting the source MAC in the Ethernet
header and in the ARP payload, stops caching header pointers across the
cow, and switches to skb_eth_hdr() so the mac header offset is not assumed
to be set in the output path.  It also adds a new condition so that
arp->mac_src is only rewritten when tx_slave is not the current active
slave.

> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> index 654f051d0023..5f750f08a144 100644
> --- a/drivers/net/bonding/bond_alb.c
> +++ b/drivers/net/bonding/bond_alb.c
> @@ -678,9 +678,13 @@ static struct slave *rlb_arp_xmit(struct sk_buff *skb, struct bonding *bond)
>  	if (arp->op_code == htons(ARPOP_REPLY)) {
>  		/* the arp must be sent on the selected rx channel */
>  		tx_slave = rlb_choose_channel(skb, bond, arp);
> -		if (tx_slave)
> +		if (tx_slave && tx_slave != rcu_access_pointer(bond->curr_active_slave)) {
> +			if (unlikely(skb_cow_head(skb, 0)))
> +				return NULL;
> +			arp = (struct arp_pkt *)skb_network_header(skb);
>  			bond_hw_addr_copy(arp->mac_src, tx_slave->dev->dev_addr,
>  					  tx_slave->dev->addr_len);
> +		}

[Severity: Medium]
The changelog states:

  "When tx_slave is the active slave, arp->mac_src already matches its
  hardware address, so we can avoid an unnecessary copy and head
  reallocation."

Is that invariant actually established anywhere in rlb_arp_xmit()?  The only
check applied to arp->mac_src earlier in the function is:

  if (!bond_slave_has_mac_rcu(bond, arp->mac_src))
          return NULL;

and bond_slave_has_mac_rcu() in include/net/bonding.h matches any slave, not
just the active one:

  bond_for_each_slave_rcu(bond, tmp, iter)
          if (ether_addr_equal_64bits(mac, tmp->dev->dev_addr))
                  return true;
  return false;

In balance-alb each slave has a distinct MAC, so an ARP reply whose sender
MAC is a non-active slave's MAC passes this filter (for example an injected
reply via AF_PACKET with CAP_NET_RAW, or an upper VLAN whose MAC was set to a
slave MAC).

Can rlb_choose_channel() then return curr_active_slave for such a packet?  It
picks the channel independently of arp->mac_src, either via the existing
client entry:

  assigned_slave = client_info->slave;
  if (assigned_slave) {
          spin_unlock(&bond->mode_lock);
          return assigned_slave;
  }

or via __rlb_next_rx_slave(), which walks all transmitting slaves including
the active one:

  bond_for_each_slave_rcu(bond, slave, iter) {
          if (!bond_slave_can_tx(slave))
                  continue;

With the new gate, the rewrite is skipped in that case, so the ARP reply goes
out advertising a MAC that is not the RX channel just recorded in
client_info->slave.  Does the peer then steer return traffic to a slave the
RLB accounting believes is unassigned?  The early return above also leaves
client_info->ntt unset, so no corrective rlb_update_client() ARP is queued.

Note that rlb_update_client(), the other emitter of this field,
unconditionally advertises the assigned slave's address:

  skb = arp_create(ARPOP_REPLY, ETH_P_ARP,
                   client_info->ip_dst,
                   client_info->slave->dev,
                   client_info->ip_src,
                   client_info->mac_dst,
                   client_info->slave->dev->dev_addr,
                   client_info->mac_dst);

so the two paths now disagree about what mac_src should contain.

There is also a failover window.  The comparison is done under
rcu_read_lock() only, since bond->mode_lock is dropped inside
rlb_choose_channel(), while bond_alb_handle_active_change() publishes the new
active slave before the MAC swap:

  /* curr_active_slave must be set before calling alb_swap_mac_addr */
  if (swap_slave) {
          /* swap mac address */
          alb_swap_mac_addr(swap_slave, new_slave);

During that window, can tx_slave == curr_active_slave hold while the slave
MACs have not been swapped yet, making the skipped rewrite wrong even for a
normally generated ARP reply?

Would a value comparison be a safer way to express this optimization, for
example:

  if (tx_slave &&
      !ether_addr_equal_64bits(arp->mac_src, tx_slave->dev->dev_addr)) {

That keeps the fast path when the field already holds the right address
without relying on the pointer identity of curr_active_slave.

[ ... ]

> @@ -1351,7 +1354,9 @@ static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
>  
>  	if (tx_slave && bond_slave_can_tx(tx_slave)) {
>  		if (tx_slave != rcu_access_pointer(bond->curr_active_slave)) {
> -			ether_addr_copy(eth_data->h_source,
> +			if (unlikely(skb_cow_head(skb, 0)))
> +				return bond_tx_drop(bond->dev, skb);
> +			ether_addr_copy(skb_eth_hdr(skb)->h_source,
>  					tx_slave->dev->dev_addr);
>  		}

Related to the above: this is the same pointer comparison, evaluated later in
the transmit path for the same skb.  If curr_active_slave changes between the
check in rlb_arp_xmit() and this one, can the Ethernet source be rewritten
here while arp->mac_src was left untouched (or the reverse), leaving the two
headers inconsistent with each other?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902125809.2910366-1-edumazet%40google.com