. On Fri, Oct 23, 2026 at 6:50 PM Willem de Bruijn
[off-list ref] wrote:
Jamal Hadi Salim wrote:
quoted
On Fri, Oct 23, 2026 at 12:09 PM Willem de Bruijn
[off-list ref] wrote:
quoted
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
quoted
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.
tbh, I was thinking more about skb frags earlier.
You are right only ETH_P_TEB promises an inner Ethernet header; for
other inner protocols the pull is wrong, so I’ll check for
p->tcfm_proto == htons(ETH_P_TEB). I didnt have this check in the
current fix - good you responded before i sent out.
1/6 (act_vlan POP_ETH) has a similar unconditional
pskb_may_pull(ETH_HLEN). A brute approach is also to check for TEB:
if (skb->protocol == htons(ETH_P_TEB) && !pskb_may_pull(skb, ETH_HLEN))
goto drop;
Note: skb_eth_pop() only removes the outer ethernet header but does
not change skb->protocol
thoughts?
cheers,
jamal
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).*