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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Hangbin Liu wrote:
> On Thu, Aug 27, 2026 at 05:37:09PM -0400, Willem de Bruijn wrote:
> > Qihang wrote:
> > > 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.
 
> 
> > 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