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

Le lun. 26 oct. 2026 à 11:32, Jamal Hadi Salim [off-list ref] a écrit :

On Mon, Oct 26, 2026 at 5:42 AM Jamal Hadi Salim [off-list ref] wrote:

quoted

On Sat, Oct 24, 2026 at 10:28 AM Jamal Hadi Salim [off-list ref] wrote:

quoted

The issue:
When the configured inner protocol is ETH_P_TEB, 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, pull the inner Ethernet header into the linear area only
when p->tcfm_proto is ETH_P_TEB; drop the packet only when that pull
fails.  Other inner protocols (IP, MPLS, ...) have no inner Ethernet
header and must not be required to carry one.  The pull is a no-op when
the header is already linear.

Conditions to recreate the bug:
- egress tc filter on lo: action vlan pop_eth action mpls pop protocol teb
  (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)
Link: https://lore.kernel.org/netdev/willemdebruijn.kernel.26d1ddb3bd2ae@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>

For the series

pw-bot: cr

Eric, you are reading this? See the discussion with Willem.

And to be clear, I am suggesting we fix both the producers and consumers.

I don't think we need to touch loopback / eth_type_trans() directly
(that's the per-driver cost I pushed back on for tun). There is a single
natural choke point: the egress fall-through in sch_handle_egress(),
right before the skb goes back to the driver's ndo_start_xmit(). A
device advertises how much link-layer header it needs via
dev->min_header_len, and that is exactly the amount eth_type_trans()
will pull. One check there covers act_vlan, act_mpls with any inner
protocol, act_ife, pedit, and BPF, now and in the future:

diff --git a/net/core/dev.c b/net/core/dev.c
index 1a09e7869118059e92b53e00ed873dfc7485ffc3..ffc8c4c0dfa35fc6eee12bec799809b0e2476270

100644

--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -4757,6 +4757,16 @@ sch_handle_egress(struct sk_buff *skb, int

*ret, struct net_device *dev)
        }
        bpf_net_ctx_clear(bpf_net_ctx);

+       /* Egress actions and programs can pop or shrink the link-layer
+        * header. Drivers expect dev->min_header_len bytes in the linear
+        * area (e.g. eth_type_trans() in loopback_xmit()).
+        */
+       if (unlikely(!pskb_may_pull(skb, dev->min_header_len))) {
+               kfree_skb_reason(skb, SKB_DROP_REASON_PKT_TOO_SMALL);
+               *ret = NET_XMIT_DROP;
+               return NULL;
+       }
+
        return skb;
 }
 #else

It is cheap: only runs when an egress tc/tcx program is attached, it is
an inline compare, and pskb_may_pull() is a no-op when the header is
already linear. For non-Ethernet devices min_header_len is 0 so nothing
changes. It is the same dev->min_header_len gate your act_mirred patch
already uses, just at the other exit point.

I applied *only* this hunk on top of net (none of patches 1, 2) and ran
your three reproducers on lo egress under PANIC_ON_OOPS=y:

  1) vlan pop_eth + bpf skb_adjust_room(-1, BPF_ADJ_ROOM_MAC)
  2) vlan pop_eth + mpls pop protocol ip
  3) vlan pop_eth alone, non-linear inner eth (AF_PACKET+PACKET_VNET_HDR)

Unpatched: all three BUG in eth_type_trans() via loopback_xmit().
With the hunk above: all three are dropped cleanly, no BUG.

So for v4 I'd suggest:
  - drop patches 1 and 2 (the egress check subsumes them, and the
    ETH_P_TEB taste question goes away);
  - keep patch 3 (act_mirred) - that's the redirect boundary, a
    different exit;
  - keep patch 4 (act_ife) - it fixes the egress frame, not just the
    crash;
  - keep patches 5 and 6 (OVS) as-is - OVS actions can't run arbitrary
    BPF, those producer fixes are correct and already reviewed, and they
    can go in independently.

One gap this does not cover: the BPF redirect path
(__bpf_redirect_common() in net/core/filter.c) only checks
mac_header < network_header, so a 1-byte "mac header" still reaches
eth_type_trans() via __dev_forward_skb2()/loopback_xmit(). I haven't
reproduced it yet but it looks like the same bug and the same
dev->min_header_len guard would close it. Can look into that separately.

Thanks,

cheers,
jamal

quoted

quoted

---
v2 -> v3:
  - drop the pre-pop mac_len gate.  The pull is a no-op when the header
    is already linear, so the gate only avoided a cheap call (Willem de
    Bruijn).
  - restrict the pull to ETH_P_TEB.  act_mpls POP can target IP, MPLS,
    etc.; those inner protocols have no Ethernet header and must not be
    required to carry one (Willem de Bruijn).
---
 net/sched/act_mpls.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
index 4ea8b2e08c3a..35a6d27f76a4 100644
--- a/net/sched/act_mpls.c
+++ b/net/sched/act_mpls.c

@@ -79,6 +79,9 @@
                if (skb_mpls_pop(skb, p->tcfm_proto, mac_len,
                                 skb->dev && skb->dev->type == ARPHRD_ETHER))
                        goto drop;
+               if (p->tcfm_proto == htons(ETH_P_TEB) &&
+                   !pskb_may_pull(skb, ETH_HLEN))
+                       goto drop;
                break;
        case TCA_MPLS_ACT_PUSH:
                new_lse = tcf_mpls_get_lse(NULL, p, !eth_p_mpls(skb_protocol(skb, true)));
--

2.43.0