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

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

[Eric Dumazet]

In bond_do_alb_xmit() and rlb_arp_xmit(), make sure to unclone
skb head via skb_cow_head() before modifying the source MAC address
(Ethernet header and ARP payload) to avoid silent corruption if
the skb is shared or cloned. Avoid caching the header pointers
across skb_cow_head().

In rlb_arp_xmit(), also check that
tx_slave != rcu_access_pointer(bond->curr_active_slave)
before modifying arp->mac_src. 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.

Also, we should not assume mac header is set in output path.

Use skb_eth_hdr() instead of eth_hdr() to fix the issue,
and remove now redundant skb_reset_mac_header() calls.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
Cc: Jay Vosburgh <jv@jvosburgh.net>
---
 drivers/net/bonding/bond_alb.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 654f051d0023929a48536c378996e0ef39b75d88..5f750f08a144791589b5ed0f44b1b68ab99e6e61 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);
+		}
 		netdev_dbg(bond->dev, "(slave %s): Server sent ARP Reply packet\n",
 			   tx_slave ? tx_slave->dev->name : "NULL");
 	} else if (arp->op_code == htons(ARPOP_REQUEST)) {
@@ -1340,7 +1344,6 @@ static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
 				    struct slave *tx_slave)
 {
 	struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
-	struct ethhdr *eth_data = eth_hdr(skb);
 
 	if (!tx_slave) {
 		/* unbalanced or unassigned, send through primary */
@@ -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);
 		}
 
@@ -1375,8 +1380,7 @@ struct slave *bond_xmit_tlb_slave_get(struct bonding *bond,
 	struct ethhdr *eth_data;
 	u32 hash_index;
 
-	skb_reset_mac_header(skb);
-	eth_data = eth_hdr(skb);
+	eth_data = skb_eth_hdr(skb);
 
 	/* Do not TX balance any multicast or broadcast */
 	if (!is_multicast_ether_addr(eth_data->h_dest)) {
@@ -1428,8 +1432,7 @@ struct slave *bond_xmit_alb_slave_get(struct bonding *bond,
 	u32 hash_index = 0;
 	int hash_size = 0;
 
-	skb_reset_mac_header(skb);
-	eth_data = eth_hdr(skb);
+	eth_data = skb_eth_hdr(skb);
 
 	switch (ntohs(skb->protocol)) {
 	case ETH_P_IP: {
-- 
2.55.0.966.g6673acef38-goog

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

[Hangbin Liu]

On Wed, Sep 02, 2026 at 12:58:09PM +0000, Eric Dumazet wrote:
> In bond_do_alb_xmit() and rlb_arp_xmit(), make sure to unclone
> skb head via skb_cow_head() before modifying the source MAC address
> (Ethernet header and ARP payload) to avoid silent corruption if
> the skb is shared or cloned. Avoid caching the header pointers
> across skb_cow_head().
> 
> In rlb_arp_xmit(), also check that
> tx_slave != rcu_access_pointer(bond->curr_active_slave)
> before modifying arp->mac_src. 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.
> 
> Also, we should not assume mac header is set in output path.
> 
> Use skb_eth_hdr() instead of eth_hdr() to fix the issue,
> and remove now redundant skb_reset_mac_header() calls.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> Cc: Jay Vosburgh <jv@jvosburgh.net>
> ---
>  drivers/net/bonding/bond_alb.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> index 654f051d0023929a48536c378996e0ef39b75d88..5f750f08a144791589b5ed0f44b1b68ab99e6e61 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);
> +		}
>  		netdev_dbg(bond->dev, "(slave %s): Server sent ARP Reply packet\n",
>  			   tx_slave ? tx_slave->dev->name : "NULL");
>  	} else if (arp->op_code == htons(ARPOP_REQUEST)) {
> @@ -1340,7 +1344,6 @@ static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
>  				    struct slave *tx_slave)
>  {
>  	struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
> -	struct ethhdr *eth_data = eth_hdr(skb);
>  
>  	if (!tx_slave) {
>  		/* unbalanced or unassigned, send through primary */
> @@ -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);
>  		}
>  
> @@ -1375,8 +1380,7 @@ struct slave *bond_xmit_tlb_slave_get(struct bonding *bond,
>  	struct ethhdr *eth_data;
>  	u32 hash_index;
>  
> -	skb_reset_mac_header(skb);
> -	eth_data = eth_hdr(skb);
> +	eth_data = skb_eth_hdr(skb);
>  
>  	/* Do not TX balance any multicast or broadcast */
>  	if (!is_multicast_ether_addr(eth_data->h_dest)) {
> @@ -1428,8 +1432,7 @@ struct slave *bond_xmit_alb_slave_get(struct bonding *bond,
>  	u32 hash_index = 0;
>  	int hash_size = 0;
>  
> -	skb_reset_mac_header(skb);
> -	eth_data = eth_hdr(skb);
> +	eth_data = skb_eth_hdr(skb);
>  
>  	switch (ntohs(skb->protocol)) {
>  	case ETH_P_IP: {
> -- 
> 2.55.0.966.g6673acef38-goog
> 

Looks reasonable to me. Thanks!

Reviewed-by: Hangbin Liu <liuhangbin@kylinos.cn>

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

[netdev-bot+sashiko]

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

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

[Eric Dumazet]

On Thu, Sep 3, 2026 at 4:09 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> 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

Okay, I will use  ether_addr_equal_64bits(arp->mac_src,
tx_slave->dev->dev_addr) in V2 if this can make AI happy.