Re: [PATCH net v2 2/6] net/sched: act_mpls: pull the inner Ethernet header after POP

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

Jamal Hadi Salim wrote:
> On Fri, Oct 23, 2026 at 12:09 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Jamal Hadi Salim wrote:
> > > The issue:
> > > act_mpls POP can leave the inner Ethernet header in a page fragment after
> > > skb_mpls_pop(), either on an L3 device (mac_len == 0) or chained after
> > > act_vlan POP_ETH.  A following eth_type_trans() can then BUG in
> > > __skb_pull().
> > >
> > > The Fix:
> > > After the pop, if the original MAC header is shorter than ETH_HLEN, pull
> > > the inner Ethernet header into the linear area and drop only when
> > > pskb_may_pull(skb, ETH_HLEN) fails.  skb_mpls_pop() does nothing when the
> > > skb was not MPLS, so no separate state is needed.
> > >
> > > Conditions to recreate the bug:
> > > - egress tc filter on lo: action vlan pop_eth action mpls pop
> > >   (chained), or on an L3 device where mac_len == 0.
> > > - AF_PACKET SOCK_RAW with PACKET_VNET_HDR, hdr_len=28, and a non-linear
> > >   eth / mpls / inner-eth frame.
> > > - The inner header is in page frags; a following eth_type_trans() can BUG
> > >   in __skb_pull().
> > >
> > > Fixes: 2a2ea50870ba ("net: sched: add mpls manipulation actions to TC")
> > > Reported-by: Vega <vega@nebusec.ai>
> > > Link: https://lore.kernel.org/r/40fe2d7dfce13411bfbe9271c0f7c54fb7e88e3b.1782548651.git.bronzed_45_vested@icloud.com/
> > > Link: https://lore.kernel.org/netdev/CAL4WiiqSMdYQYeeW0Fk_GK6ekR4xiS4zPrGKrbB9wZknpqf1-w@mail.gmail.com/
> > > Cc: stable@vger.kernel.org
> > > Suggested-by: Jakub Kicinski <kuba@kernel.org>
> > > Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > > Suggested-by: Eric Dumazet <edumazet@kernel.org>
> > > Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> > > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > > ---
> > >  net/sched/act_mpls.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
> > > index 4ea8b2e08c3a..5363bca892fb 100644
> > > --- a/net/sched/act_mpls.c
> > > +++ b/net/sched/act_mpls.c
> > > @@ -79,6 +79,8 @@ TC_INDIRECT_SCOPE int tcf_mpls_act(struct sk_buff *skb,
> > >               if (skb_mpls_pop(skb, p->tcfm_proto, mac_len,
> > >                                skb->dev && skb->dev->type == ARPHRD_ETHER))
> > >                       goto drop;
> > > +             if (mac_len < ETH_HLEN && !pskb_may_pull(skb, ETH_HLEN))
> > > +                     goto drop;
> >
> > Is this first condition needed?
> >
> 
> That was me conjecturing from Eric saying in v1 "...the check can only
> matter when the original MAC header is short.."
> Since i have to send a v3 (Thanks Eric;->) I will remove it
> 
> > More importantly, can it hide a bug?
> >
> 
> What kind of bug?

Two kinds. One, if original mac_len is larger, gets popped and the
resultant packet is still too short for ETH_HLEN afterwards.

But this can probably not happen, as skb_mpls_pop only pops one MPLS
tag.

What this patch tries to ensure is that the subsequent Ethernet
header is linear.

The second potential bug is this assumption that after the MPLS tag
lies an Ethernet header. Is that a valid assumption? Or could we drop
a valid packet with smaller header.

My LLM raised similar points and says it better than I could:

  1. Missing protocol check: tcf_mpls_act handles popping MPLS to arbitrary inner protocols (ETH_P_IP, ETH_P_IPV6, ETH_P_MPLS_UC, ETH_P_TEB).
  Requiring ETH_HLEN (14 bytes) unconditionally when mac_len < ETH_HLEN drops valid non-Ethernet packets shorter than 14 bytes (e.g. popping
  an outer label to reveal a 4-byte inner label stack entry on an L3 path or after pop_eth). Pulling ETH_HLEN only applies when popping to
  Ethernet / TEB (p->tcfm_proto == htons(ETH_P_TEB)), exactly as done in patch 5 for Open vSwitch (pop_mpls).
  2. Gating on mac_len < ETH_HLEN: mac_len here is the pre-pop outer MAC length. pskb_may_pull is already a no-op if the linear buffer is
  sufficient; gating on pre-pop mac_len is unnecessary and risks skipping the pull when mac_len >= ETH_HLEN even if the revealed header is in
  fragments.
  3. Downstream metadata invariant: Popping to ETH_P_TEB exposes an inner Ethernet header at skb->data, but skb_mpls_pop leaves skb->mac_len
  at 0 and resets the network header to 0 (skb_network_offset(skb) == 0). Downstream actions such as act_mirred (with patch 3's
  skb_network_offset(skb) < dev->min_header_len check) will treat the packet as headerless and drop it, causing regressions in L2VPN
  forwarding (tc_mpls_l2vpn.sh). When popping to TEB, the L2 MAC header boundaries (mac_len and network header offset) need to be properly
  established.
  ──────
  ### Summary of comments:

  • Patch 2/6: Missing inner protocol check (p->tcfm_proto == htons(ETH_P_TEB)) before pulling ETH_HLEN; drops non-Ethernet payloads < 14
  bytes.
  • Patch 2/6: Unsound pre-pop mac_len < ETH_HLEN gate.
  • Patch 2/6: Incomplete header bookkeeping (mac_len and network header offset) on TEB pop breaks subsequent redirect actions (act_mirred).*