Re: [PATCH net 1/2] bonding: reject frames with insufficient headroom in bond_header_create

Hi Willem,

I'll resend this as v2 with a cover letter.

On scope: I grepped every header_ops->create.  Five devices delegate to a
lower device (bond, team, macvlan, ipvlan, 6lowpan), but only bond and
team re-select that lower device under RCU -- the rest bind it at netdev
creation.  So bond and team are the full scope.

Re Fixes: I used 950803f because it introduced bond_header_create (where
this check lives); before it the type-confusion BUG masked this race.
Happy to point at 1284cd3a2b74 instead if you prefer.

On the fix location, I'd rather get your read before writing more.  The
options I see:

  1. per-wrapper headroom reject in bond/team only -- small, safe,
     backport-friendly, but not generic (the next stacked device needs
     its own).

  2. central check in dev_hard_header on dev->hard_header_len -- one
     place, but I think it's unsafe for bond: bond_dev->hard_header_len
     only follows the first slave (bond_setup_by_slave),
     bond_change_active_slave flips curr_active_slave without touching
     it, and same-type slaves can have different hard_header_len (two GRE
     tunnels).  So it can under-reject.  Would need bond to maintain
     hard_header_len differently first.

  3. resolve the effective {dev, ops, hard_header_len} as one RCU triple
     via a generic callback, so the caller never cares which subordinate
     the master picks.  Truly generic and race-free, but it's a new ndo --
     net-next material, awkward for stable backport.

  4. make header_ops->create() take an explicit headroom contract and
     fail cleanly instead of pushing blind -- cleanest long-term, but
     touches every create() in the tree.  Too heavy for now.

My v1 is option 1 (per-wrapper).  Want one of these, a combination,
or something else entirely?

Thanks,
Qihang


On Wed, Aug 26, 2026 at 10:02 AM Willem de Bruijn
[off-list ref] wrote:

Qihang wrote:

quoted

From: Qihang Tang <redacted>

Thanks for working on this Qihang.

Minor: a patch series should generally have a cover letter

quoted

AF_PACKET SOCK_DGRAM sends reserve skb headroom from a snapshot of
bond_dev->hard_header_len.  A concurrent bond type change can switch the
active slave to one with a larger hard_header_len between that snapshot
and bond_header_create(), so the slave's create() pushes or writes past
skb->head.

Are we certain that bond and team are the only devices that implement
header_ops->create?

quoted

The hard_header_len snapshot series that fixed the SOCK_RAW send paths
deferred this SOCK_DGRAM race: dev->header_ops is the stable
bond_header_ops, so snapshotting header_ops in the caller does not help.

Reject the frame if skb headroom is smaller than the active slave's
hard_header_len, before delegating under the existing rcu_read_lock.

Fixes: 950803f72547 ("bonding: fix type confusion in bond_setup_by_slave()")

What's the rationale behind blaming this SHA1?

quoted

Cc: stable@vger.kernel.org
Cc: Willem de Bruijn <willemb@google.com>
Signed-off-by: Qihang Tang <redacted>
---
 drivers/net/bonding/bond_main.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 522eab060f9e..9ec663610dfd 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c

@@ -1524,10 +1524,24 @@ static int bond_header_create(struct sk_buff *skb, struct net_device *bond_dev,
      slave = rcu_dereference(bond->curr_active_slave);
      if (slave) {
              slave_ops = READ_ONCE(slave->dev->header_ops);
-             if (slave_ops && slave_ops->create)
+             if (slave_ops && slave_ops->create) {
+                     unsigned int hlen = READ_ONCE(slave->dev->hard_header_len);
+
+                     /* Headroom was reserved from a snapshot of
+                      * bond_dev->hard_header_len that may predate this
+                      * slave (concurrent bond type change); reject if
+                      * insufficient for the slave's create(), which
+                      * pushes its own hlen.
+                      */
+                     if (skb_headroom(skb) < hlen) {
+                             ret = -EINVAL;
+                             goto unlock;
+                     }
                      ret = slave_ops->create(skb, slave->dev,
                                              type, daddr, saddr, len);
+             }

Ideally we could fix this in dev_hard_header or PF_PACKET itself,
rather than in each implementation that's in scope.

But there seems no way around holding this rcu reference on the slave
device while re-checking.

quoted

      }
+unlock:
      rcu_read_unlock();
      return ret;
 }
--
2.50.1 (Apple Git-155)