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

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

[Qihang]

From: Qihang Tang <q.h.hack.winter@gmail.com>

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.

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()")
Cc: stable@vger.kernel.org
Cc: Willem de Bruijn <willemb@google.com>
Signed-off-by: Qihang Tang <q.h.hack.winter@gmail.com>
---
 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);
+		}
 	}
+unlock:
 	rcu_read_unlock();
 	return ret;
 }
-- 
2.50.1 (Apple Git-155)

[PATCH net 2/2] team: reject frames with insufficient headroom in team_header_create

[Qihang]

From: Qihang Tang <q.h.hack.winter@gmail.com>

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

This is the same race as in bond_header_create(); the snapshot series
that fixed the SOCK_RAW send paths deferred it.  dev->header_ops is the
stable team_header_ops, so snapshotting header_ops in the caller does
not help.

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

Fixes: 425000dbf173 ("team: fix header_ops type confusion with non-Ethernet ports")
Cc: stable@vger.kernel.org
Cc: Willem de Bruijn <willemb@google.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Qihang Tang <q.h.hack.winter@gmail.com>
---
 drivers/net/team/team_core.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/net/team/team_core.c b/drivers/net/team/team_core.c
index feaa75fbf8fc..2742a4bfc9d0 100644
--- a/drivers/net/team/team_core.c
+++ b/drivers/net/team/team_core.c
@@ -2269,10 +2269,24 @@ static int team_header_create(struct sk_buff *skb, struct net_device *team_dev,
 	port = team_header_port_get_rcu(team, true);
 	if (port) {
 		port_ops = READ_ONCE(port->dev->header_ops);
-		if (port_ops && port_ops->create)
+		if (port_ops && port_ops->create) {
+			unsigned int hlen = READ_ONCE(port->dev->hard_header_len);
+
+			/* Headroom was reserved from a snapshot of
+			 * team_dev->hard_header_len that may predate this
+			 * port (concurrent team type change); reject if
+			 * insufficient for the port's create(), which
+			 * pushes its own hlen.
+			 */
+			if (skb_headroom(skb) < hlen) {
+				ret = -EINVAL;
+				goto unlock;
+			}
 			ret = port_ops->create(skb, port->dev,
 					       type, daddr, saddr, len);
+		}
 	}
+unlock:
 	rcu_read_unlock();
 	return ret;
 }
-- 
2.50.1 (Apple Git-155)

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

[Willem de Bruijn]

Qihang wrote:
> From: Qihang Tang <q.h.hack.winter@gmail.com>

Thanks for working on this Qihang.

Minor: a patch series should generally have a cover letter

> 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?
 
> 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?

> Cc: stable@vger.kernel.org
> Cc: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Qihang Tang <q.h.hack.winter@gmail.com>
> ---
>  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.



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

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

[Hangbin Liu]

Hi Qinghang,

On Mon, Aug 24, 2026 at 10:18:01AM +0800, Qihang wrote:
> From: Qihang Tang <q.h.hack.winter@gmail.com>
> 
> 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.

How can bonding change its device type while still sending skbs?
Bonding does not allow enslaving devices of different types. We have
to remove all slaves before changing its type.

Thanks
Hangbin

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

[Qihang]

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
<willemdebruijn.kernel@gmail.com> wrote:
>
> Qihang wrote:
> > From: Qihang Tang <q.h.hack.winter@gmail.com>
>
> Thanks for working on this Qihang.
>
> Minor: a patch series should generally have a cover letter
>
> > 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?
>
> > 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?
>
> > Cc: stable@vger.kernel.org
> > Cc: Willem de Bruijn <willemb@google.com>
> > Signed-off-by: Qihang Tang <q.h.hack.winter@gmail.com>
> > ---
> >  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.
>
>
>
> >       }
> > +unlock:
> >       rcu_read_unlock();
> >       return ret;
> >  }
> > --
> > 2.50.1 (Apple Git-155)
> >
>
>

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

[Qihang]

Hi Hangbin,

Thanks for the catch. You're right, my wording was inaccurate. This is not
about an active slave switch or changing the bond type while sending.

The issue is a hard_header_len update race in bond_setup_by_slave(). An
in-flight AF_PACKET SOCK_DGRAM send can snapshot the old
bond_dev->hard_header_len
for skb headroom reservation, then bond_setup_by_slave() updates it and
installs a new active slave. The existing send path can continue after this
because dev_close()/dev_open() does not synchronize with already running
packet_snd().

Later bond_header_create() uses the new active slave's hard_header_len, which
may be larger than the reserved headroom.

For the first-slave case, the bond can be slaveless before enslaving, so
removing existing slaves does not prevent this window.

Thanks,
Qihang

On Thu, Aug 27, 2026 at 9:10 AM Hangbin Liu <hangbin.liu@linux.dev> wrote:
>
> Hi Qinghang,
>
> On Mon, Aug 24, 2026 at 10:18:01AM +0800, Qihang wrote:
> > From: Qihang Tang <q.h.hack.winter@gmail.com>
> >
> > 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.
>
> How can bonding change its device type while still sending skbs?
> Bonding does not allow enslaving devices of different types. We have
> to remove all slaves before changing its type.
>
> Thanks
> Hangbin

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

[Jiayuan Chen]

在 8/27/26 11:13 AM, Qihang 写道:
> 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.
Can you check whether IPoIB + bond hits the issue you describe? That combo
isn't affected by 950803f (ipoib_hard_header doesn't use netdev_priv, no 
type

confusion), so if it overflows, the Fixes tag shouldn't point to 950803f.

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

[Willem de Bruijn]

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
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.

But this is not enforced.

Given that this seems a bond specific issue, that was copied in teams,
fair to solve it there. Doubly so as due to RCU we have no easy path
to solve it elsewhere. So +1.
> Thanks,
> Qihang
> 
> 
> On Wed, Aug 26, 2026 at 10:02 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Qihang wrote:
> > > From: Qihang Tang <q.h.hack.winter@gmail.com>
> >
> > Thanks for working on this Qihang.
> >
> > Minor: a patch series should generally have a cover letter
> >
> > > 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?
> >
> > > 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?
> >
> > > Cc: stable@vger.kernel.org
> > > Cc: Willem de Bruijn <willemb@google.com>
> > > Signed-off-by: Qihang Tang <q.h.hack.winter@gmail.com>
> > > ---
> > >  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.
> >
> >
> >
> > >       }
> > > +unlock:
> > >       rcu_read_unlock();
> > >       return ret;
> > >  }
> > > --
> > > 2.50.1 (Apple Git-155)
> > >
> >
> >

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

[Hangbin Liu]

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()


> 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().

Thanks
Hangbin

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

[Willem de Bruijn]

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

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

[Qihang]

Yeah, agreed on steady state.

One thing to flag: packet_snd snapshots dev->hard_header_len at
af_packet.c:3023 (no lock) but pushes the header at :3059 onto whatever
curr_active_slave is then. A concurrent enslave that bumps it in between
-- first non-Ethernet slave 14->24, or a bigger-header failover -- can
leave the in-flight frame short and trip skb_under_panic (oops/panic,
not silent corruption; local DoS for a privileged user).

Happy to drop v1, or hand over the create-time check if you want it closed.