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

Hangbin Liu wrote:

On Thu, Aug 27, 2026 at 05:37:09PM -0400, Willem de Bruijn wrote:

quoted

Qihang wrote:

quoted

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 for the analysis.

I agree that 3 nd 4 are too invasive for the scope of the bug.

It's a bit odd that bond and team just take the hard_header_len of the
first slave. And that bond_create_header just passes to

Actually, bond will use the max hard_header_len of all slaves, see

netdev_compute_master_upper_features()

Awesome. That should address the issue.
 

quoted

curr_active_slave, while various bond modes like LAG will have
multiple concurrently active slaves.

That indicates that the intent is for all slaves to have the same
header length and header_ops->create callback.

And here seem you want to all saves also sync the header length?

I'm not sure if we should/could do this in the same function
netdev_compute_master_upper_features().

As long as the header length allocated is the max of all slaves'
requirements, no need to check again here, I think.

Thanks
Hangbin