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

On Fri, Oct 23, 2026 at 12:09 PM Willem de Bruijn
[off-list ref] wrote:

Jamal Hadi Salim wrote:

quoted

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 <redacted>
Link: https://lore.kernel.org/r/40fe2d7dfce13411bfbe9271c0f7c54fb7e88e3b.1782548651.git.bronzed_45_vested@icloud.com/ (local)
Link: https://lore.kernel.org/netdev/CAL4WiiqSMdYQYeeW0Fk_GK6ekR4xiS4zPrGKrbB9wZknpqf1-w@mail.gmail.com/ (local)
Cc: <redacted>
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 <redacted>
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?

cheers,
jamal

Simply running pskb_may_pull is cheap if no pull is needed.

quoted

              break;
      case TCA_MPLS_ACT_PUSH:
              new_lse = tcf_mpls_get_lse(NULL, p, !eth_p_mpls(skb_protocol(skb, true)));
--
2.43.0