[PATCH net v2 0/6] net: pull short linear Ethernet headers before eth_type_trans()

[PATCH net v2 0/6] net: pull short linear Ethernet headers before eth_type_trans()

[Jamal Hadi Salim]

This series fixes a class of producer/consumer bugs in which an outer
header is popped and the packet is then handed to a consumer with fewer
than ETH_HLEN bytes in the linear area.  eth_type_trans() pulls ETH_HLEN
from that area and can BUG in __skb_pull() when the inner Ethernet header
is not linear.  The affected producers are act_vlan POP_ETH, act_mpls POP,
act_ife egress decode, and the Open vSwitch MPLS/TEB and NSH pops.  A
consumer-side guard in act_mirred rejects redirecting a headerless packet
to a device that expects a link-layer header.

Patch 1 makes act_vlan POP_ETH pull the inner Ethernet header after the
pop and drop only when the packet is genuinely shorter than ETH_HLEN.
Patch 2 does the same for act_mpls, gated on the pre-pop MAC length.
Patch 3 adds the act_mirred guard at the redirect boundary.  Patch 4
keeps act_ife egress decode from consuming the inner Ethernet header and
restores the header metadata.

Patches 5 and 6 establish the same linear-header invariant for the OVS
MPLS/TEB and NSH pops before restoring MAC_PROTO_ETHERNET.

Changes in v2:
  - Pull the inner header with pskb_may_pull(skb, ETH_HLEN) and drop only
    on failure, instead of dropping on the post-pop skb_headlen() check
    which may drop frags (Eric/Willem).
  - act_mpls: gate the pull on the pre-pop mac_len and remove the separate
    was_mpls state.
  - Drop the v1 TUN patch; add the act_mirred consumer-side guard.
  - Add the act_ife egress decode fix; decode is receive-side, and the
    egress path restores the frame for loopback-style transmit paths.
  - Keep the OVS TEB and NSH patches unchanged and restore their v1
    Reviewed-by trailers.
  - Drop two bouncing Cc addresses.

Jamal Hadi Salim (6):
  net/sched: act_vlan: pull the inner Ethernet header after POP_ETH
  net/sched: act_mpls: pull the inner Ethernet header after POP
  net/sched: act_mirred: reject redirects without a link-layer header
  net/sched: act_ife: do not consume the inner header on egress
  net: openvswitch: ensure a linear inner Ethernet header on TEB pop
  net: openvswitch: ensure a linear inner Ethernet header on NSH pop

 net/openvswitch/actions.c | 12 +++++++++---
 net/sched/act_ife.c       |  4 ++++
 net/sched/act_mirred.c    |  7 +++++++
 net/sched/act_mpls.c      |  2 ++
 net/sched/act_vlan.c      |  2 ++
 5 files changed, 24 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH net v2 1/6] net/sched: act_vlan: pull the inner Ethernet header after POP_ETH

[Jamal Hadi Salim]

The issue:
act_vlan POP_ETH removes the outer Ethernet header and leaves the inner
Ethernet header at the head of the packet.  A non-linear skb can have the
inner header in a page fragment, so a following eth_type_trans() may find
less than ETH_HLEN bytes in the linear area and BUG in __skb_pull().

The Fix:
Pull the inner Ethernet header into the linear area after skb_eth_pop():
drop the packet only when pskb_may_pull(skb, ETH_HLEN) fails.  This keeps
valid non-linear packets whose inner header sits in frags.

Conditions to recreate the bug:
- egress tc filter on lo: action vlan pop_eth.
- AF_PACKET SOCK_RAW with PACKET_VNET_HDR, hdr_len=28, and a non-linear
  eth / inner-eth frame.
- The inner header is in page frags; a following eth_type_trans() can BUG
  in __skb_pull().

Fixes: 19fbcb36a39e ("net/sched: act_vlan: Add {POP,PUSH}_ETH actions")
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/willemdebruijn.kernel.38722ee638a21@gmail.com/
Link: https://lore.kernel.org/netdev/CAL4WiipJLzZ9ywOPaFHZ+wePTOuO+6+aaRCVXMHz+xmmQm2hcA@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>

---
v1 -> v2:
  - pull the inner Ethernet header with pskb_may_pull() and drop only
    when the packet is too short;
  - keep the OVS TEB and NSH members and restore their Reviewed-by
    trailers;
  - replace the TUN patch with the act_mirred consumer guard and add
    the act_ife egress fix.
---
 net/sched/act_vlan.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 6a375fdc63dd..bca95e0bbf64 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -79,6 +79,8 @@ TC_INDIRECT_SCOPE int tcf_vlan_act(struct sk_buff *skb,
 		err = skb_eth_pop(skb);
 		if (err)
 			goto drop;
+		if (!pskb_may_pull(skb, ETH_HLEN))
+			goto drop;
 		break;
 	case TCA_VLAN_ACT_PUSH_ETH:
 		err = skb_eth_push(skb, p->tcfv_push_dst, p->tcfv_push_src);
-- 
2.43.0

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

[Jamal Hadi Salim]

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;
 		break;
 	case TCA_MPLS_ACT_PUSH:
 		new_lse = tcf_mpls_get_lse(NULL, p, !eth_p_mpls(skb_protocol(skb, true)));
-- 
2.43.0

[PATCH net v2 3/6] net/sched: act_mirred: reject redirects without a link-layer header

[Jamal Hadi Salim]

The issue:
tcf_mirred_to_dev() may redirect a packet that has no link-layer header to
a device that expects one.  TUN builds a valid L3 skb with mac_len == 0;
mirred then pushes zero bytes and the IP header is parsed as Ethernet by
eth_type_trans().  The same happens after a VLAN POP_ETH or MPLS POP chain
whose producer did not leave a full inner Ethernet header.

The Fix:
After the existing push/pull adjustment, when the target expects data at
the MAC header, require a link-layer header of at least
dev->min_header_len bytes and pull it into the linear area.  Drop the
packet otherwise.  This mirrors the check bpf_redirect() already performs
and keeps the legitimate push_eth-then-mirred case working.

Conditions to recreate the bug:
- create a TUN device with IFF_TUN | IFF_VNET_HDR | IFF_NO_PI;
- attach egress mirred redirect to an Ethernet device (for example lo);
- write a virtio_net_hdr with hdr_len=1 and an 8192-byte payload.
The unfixed kernel parses the IP header as Ethernet and can BUG in
eth_type_trans() via loopback_xmit.

Fixes: 53592b364001 ("net/sched: act_mirred: Implement ingress actions")
Suggested-by: Eric Dumazet <edumazet@kernel.org>
Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/act_mirred.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 553342c55cf7..e3e0e4aa70a1 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -316,6 +316,11 @@ static int tcf_mirred_to_dev(struct sk_buff *skb, struct tcf_mirred *m,
 		}
 	}
 
+	if (!expects_nh &&
+	    (unlikely(skb_network_offset(skb_to_send) < dev->min_header_len) ||
+	     !pskb_may_pull(skb_to_send, dev->min_header_len)))
+		goto err_cant_do;
+
 	skb_to_send->skb_iif = skb->dev->ifindex;
 	skb_to_send->dev = dev;
 
@@ -335,6 +340,8 @@ static int tcf_mirred_to_dev(struct sk_buff *skb, struct tcf_mirred *m,
 	return retval;
 
 err_cant_do:
+	if (skb_to_send != skb)
+		kfree_skb(skb_to_send);
 	if (is_redirect)
 		retval = TC_ACT_SHOT;
 	tcf_action_inc_overlimit_qstats(&m->common);
-- 
2.43.0

[PATCH net v2 4/6] net/sched: act_ife: do not consume the inner header on egress

[Jamal Hadi Salim]

The issue:
tcf_ife_decode() unconditionally calls eth_type_trans(), which pulls
ETH_HLEN.  On egress this consumes the inner Ethernet header; a following
eth_type_trans() from loopback_xmit() then parses L3 data as Ethernet and
can BUG in __skb_pull() when the remaining linear area is short.

The Fix:
Parse the inner Ethernet header with eth_type_trans() in both directions,
then restore the egress data position and MAC length.  On ingress the
existing behavior is unchanged.  On egress, push the inner Ethernet header
back after parsing so the xmit path sees the original frame and later
actions see a consistent network header.

Decode is a receive-side operation; the egress branch keeps the frame
intact for transmit paths such as loopback.  tcf_ife_is_eth_skb() accepts
Ethernet and loopback devices generally.

Conditions to recreate the bug:
- egress tc filter on lo: action ife decode pipe;
- AF_PACKET SOCK_RAW frame: outer Ethernet with ETH_P_IFE, IFE metadata,
  inner Ethernet and an 8192-byte payload;
- the egress pipe re-enters eth_type_trans() via loopback_xmit() after the
  action consumed the inner header.

Fixes: ef6980b6becb ("introduce IFE action")
Cc: stable@vger.kernel.org
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/act_ife.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 2afd68983ece..3946a9589bb8 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -769,6 +769,10 @@ static int tcf_ife_decode(struct sk_buff *skb, const struct tc_action *a,
 
 	skb->protocol = eth_type_trans(skb, skb->dev);
 	skb_reset_network_header(skb);
+	if (!skb_at_tc_ingress(skb)) {
+		skb_push(skb, ETH_HLEN);
+		skb_reset_mac_len(skb);
+	}
 
 	return action;
 }
-- 
2.43.0

[PATCH net v2 5/6] net: openvswitch: ensure a linear inner Ethernet header on TEB pop

[Jamal Hadi Salim]

The issue:
pop_mpls() restores MAC_PROTO_ETHERNET when the popped ethertype is
ETH_P_TEB, but skb_mpls_pop() only guarantees that the MPLS shim is in
the linear area.  When the inner Ethernet header sits in a page
fragment, the datapath now claims an Ethernet header it cannot access
linearly.

Output to an OVS internal vport then calls eth_type_trans(), which
pulls ETH_HLEN from an empty linear area and triggers the BUG in
__skb_pull():

  kernel BUG at ./include/linux/skbuff.h:2830!
  RIP: 0010:eth_type_trans+0x335/0x500
  internal_dev_recv
  ovs_vport_send
  do_output
  do_execute_actions
  ovs_execute_actions
  ovs_dp_process_packet
  ovs_vport_receive
  internal_dev_xmit

Conditions to recreate the bug:
On a system with 4 KiB pages and CONFIG_OPENVSWITCH and CONFIG_PACKET
enabled, use CAP_NET_ADMIN to create an OVS datapath and bring its local
internal port up with MTU 9000.  Install a flow matching an Ethernet/MPLS
packet with one bottom-of-stack label and actions pop_eth,
pop_mpls(0x6558), output to the local port.  With CAP_NET_RAW, send a
4128-byte AF_PACKET/SOCK_RAW frame containing outer Ethernet, the MPLS
label, inner Ethernet and 4096 payload bytes.  The two pops consume the
linear headers and expose the inner Ethernet header in page fragments.

The Fix:
Pull the inner Ethernet header into the linear area before restoring
MAC_PROTO_ETHERNET.

Fixes: f66b53fdbb22 ("openvswitch: New MPLS actions for layer 2 tunnelling")
Link: https://lore.kernel.org/netdev/e85fa16e-df3c-43dd-bb3a-9edca07200ea@kernel.org/
Cc: stable@vger.kernel.org
Reviewed-by: Ilya Maximets <i.maximets@ovn.org>
Reviewed-by: Eric Dumazet <edumazet@kernel.org>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/openvswitch/actions.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 27152bca8a8c..03b31e33e880 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -153,8 +153,11 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
 	if (err)
 		return err;
 
-	if (ethertype == htons(ETH_P_TEB))
+	if (ethertype == htons(ETH_P_TEB)) {
+		if (!pskb_may_pull(skb, ETH_HLEN))
+			return -ENOMEM;
 		key->mac_proto = MAC_PROTO_ETHERNET;
+	}
 
 	invalidate_flow_key(key);
 	return 0;
-- 
2.43.0

[PATCH net v2 6/6] net: openvswitch: ensure a linear inner Ethernet header on NSH pop

[Jamal Hadi Salim]

The issue:
pop_nsh() restores MAC_PROTO_ETHERNET when the popped NSH next-protocol
is TUN_P_ETHERNET, but nsh_pop() only guarantees that the NSH header is
in the linear area.  When the inner Ethernet header sits in a page
fragment, the datapath then claims an Ethernet header it cannot access
linearly.

Output to an OVS internal vport then calls eth_type_trans(), which
pulls ETH_HLEN from an empty linear area and triggers the BUG in
__skb_pull():

  kernel BUG at ./include/linux/skbuff.h:2830!
  RIP: 0010:eth_type_trans+0x335/0x500
  internal_dev_recv
  ovs_vport_send
  do_output
  do_execute_actions
  ovs_execute_actions
  ovs_dp_process_packet
  ovs_vport_receive
  internal_dev_xmit

Conditions to recreate the bug:
On a system with 4 KiB pages and CONFIG_OPENVSWITCH and CONFIG_PACKET
enabled, use CAP_NET_ADMIN to create an OVS datapath and bring its local
internal port up with MTU 9000.  Install a flow matching an Ethernet/NSH
packet with a 24-byte MD1 header whose next-protocol is TUN_P_ETHERNET,
and actions pop_eth, pop_nsh, output to the local port.  With
CAP_NET_RAW, send a 4148-byte AF_PACKET/SOCK_RAW frame containing outer
Ethernet, the NSH header, inner Ethernet and 4096 payload bytes.  The
two pops consume the linear headers and expose the inner Ethernet header
in page fragments.

The fix:
Pull the inner Ethernet header into the linear area before restoring
MAC_PROTO_ETHERNET.  This is the NSH counterpart of the MPLS/TEB
producer fix in the previous patch.

Fixes: b2d0f5d5dc53 ("openvswitch: enable NSH support")
Link: https://lore.kernel.org/netdev/e85fa16e-df3c-43dd-bb3a-9edca07200ea@kernel.org/
Cc: stable@vger.kernel.org
Reviewed-by: Jiri Benc <jbenc@redhat.com>
Reviewed-by: Ilya Maximets <i.maximets@ovn.org>
Reviewed-by: Eric Dumazet <edumazet@kernel.org>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/openvswitch/actions.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 03b31e33e880..cb7e16c6a8ef 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -314,10 +314,13 @@ static int pop_nsh(struct sk_buff *skb, struct sw_flow_key *key)
 		return err;
 
 	/* safe right before invalidate_flow_key */
-	if (skb->protocol == htons(ETH_P_TEB))
+	if (skb->protocol == htons(ETH_P_TEB)) {
+		if (!pskb_may_pull(skb, ETH_HLEN))
+			return -ENOMEM;
 		key->mac_proto = MAC_PROTO_ETHERNET;
-	else
+	} else {
 		key->mac_proto = MAC_PROTO_NONE;
+	}
 	invalidate_flow_key(key);
 	return 0;
 }
-- 
2.43.0

Re: [PATCH net v2 6/6] net: openvswitch: ensure a linear inner Ethernet header on NSH pop

[Aaron Conole]

Jamal Hadi Salim <jhs@mojatatu.com> writes:

> The issue:
> pop_nsh() restores MAC_PROTO_ETHERNET when the popped NSH next-protocol
> is TUN_P_ETHERNET, but nsh_pop() only guarantees that the NSH header is
> in the linear area.  When the inner Ethernet header sits in a page
> fragment, the datapath then claims an Ethernet header it cannot access
> linearly.
>
> Output to an OVS internal vport then calls eth_type_trans(), which
> pulls ETH_HLEN from an empty linear area and triggers the BUG in
> __skb_pull():
>
>   kernel BUG at ./include/linux/skbuff.h:2830!
>   RIP: 0010:eth_type_trans+0x335/0x500
>   internal_dev_recv
>   ovs_vport_send
>   do_output
>   do_execute_actions
>   ovs_execute_actions
>   ovs_dp_process_packet
>   ovs_vport_receive
>   internal_dev_xmit
>
> Conditions to recreate the bug:
> On a system with 4 KiB pages and CONFIG_OPENVSWITCH and CONFIG_PACKET
> enabled, use CAP_NET_ADMIN to create an OVS datapath and bring its local
> internal port up with MTU 9000.  Install a flow matching an Ethernet/NSH
> packet with a 24-byte MD1 header whose next-protocol is TUN_P_ETHERNET,
> and actions pop_eth, pop_nsh, output to the local port.  With
> CAP_NET_RAW, send a 4148-byte AF_PACKET/SOCK_RAW frame containing outer
> Ethernet, the NSH header, inner Ethernet and 4096 payload bytes.  The
> two pops consume the linear headers and expose the inner Ethernet header
> in page fragments.
>
> The fix:
> Pull the inner Ethernet header into the linear area before restoring
> MAC_PROTO_ETHERNET.  This is the NSH counterpart of the MPLS/TEB
> producer fix in the previous patch.
>
> Fixes: b2d0f5d5dc53 ("openvswitch: enable NSH support")
> Link: https://lore.kernel.org/netdev/e85fa16e-df3c-43dd-bb3a-9edca07200ea@kernel.org/
> Cc: stable@vger.kernel.org
> Reviewed-by: Jiri Benc <jbenc@redhat.com>
> Reviewed-by: Ilya Maximets <i.maximets@ovn.org>
> Reviewed-by: Eric Dumazet <edumazet@kernel.org>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> ---

Reviweed-by: Aaron Conole <aconole@redhat.com>

Re: [PATCH net v2 5/6] net: openvswitch: ensure a linear inner Ethernet header on TEB pop

[Aaron Conole]

Jamal Hadi Salim <jhs@mojatatu.com> writes:

> The issue:
> pop_mpls() restores MAC_PROTO_ETHERNET when the popped ethertype is
> ETH_P_TEB, but skb_mpls_pop() only guarantees that the MPLS shim is in
> the linear area.  When the inner Ethernet header sits in a page
> fragment, the datapath now claims an Ethernet header it cannot access
> linearly.
>
> Output to an OVS internal vport then calls eth_type_trans(), which
> pulls ETH_HLEN from an empty linear area and triggers the BUG in
> __skb_pull():
>
>   kernel BUG at ./include/linux/skbuff.h:2830!
>   RIP: 0010:eth_type_trans+0x335/0x500
>   internal_dev_recv
>   ovs_vport_send
>   do_output
>   do_execute_actions
>   ovs_execute_actions
>   ovs_dp_process_packet
>   ovs_vport_receive
>   internal_dev_xmit
>
> Conditions to recreate the bug:
> On a system with 4 KiB pages and CONFIG_OPENVSWITCH and CONFIG_PACKET
> enabled, use CAP_NET_ADMIN to create an OVS datapath and bring its local
> internal port up with MTU 9000.  Install a flow matching an Ethernet/MPLS
> packet with one bottom-of-stack label and actions pop_eth,
> pop_mpls(0x6558), output to the local port.  With CAP_NET_RAW, send a
> 4128-byte AF_PACKET/SOCK_RAW frame containing outer Ethernet, the MPLS
> label, inner Ethernet and 4096 payload bytes.  The two pops consume the
> linear headers and expose the inner Ethernet header in page fragments.
>
> The Fix:
> Pull the inner Ethernet header into the linear area before restoring
> MAC_PROTO_ETHERNET.
>
> Fixes: f66b53fdbb22 ("openvswitch: New MPLS actions for layer 2 tunnelling")
> Link: https://lore.kernel.org/netdev/e85fa16e-df3c-43dd-bb3a-9edca07200ea@kernel.org/
> Cc: stable@vger.kernel.org
> Reviewed-by: Ilya Maximets <i.maximets@ovn.org>
> Reviewed-by: Eric Dumazet <edumazet@kernel.org>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> ---

Reviewed-by: Aaron Conole <aconole@redhat.com>

Re: [PATCH net v2 4/6] net/sched: act_ife: do not consume the inner header on egress

[Eric Dumazet]

Le ven. 23 oct. 2026 à 13:59, Jamal Hadi Salim <jhs@mojatatu.com> a écrit :
>
> The issue:
> tcf_ife_decode() unconditionally calls eth_type_trans(), which pulls
> ETH_HLEN.  On egress this consumes the inner Ethernet header; a following
> eth_type_trans() from loopback_xmit() then parses L3 data as Ethernet and
> can BUG in __skb_pull() when the remaining linear area is short.
>
> The Fix:
> Parse the inner Ethernet header with eth_type_trans() in both directions,
> then restore the egress data position and MAC length.  On ingress the
> existing behavior is unchanged.  On egress, push the inner Ethernet header
> back after parsing so the xmit path sees the original frame and later
> actions see a consistent network header.
>
> Decode is a receive-side operation; the egress branch keeps the frame
> intact for transmit paths such as loopback.  tcf_ife_is_eth_skb() accepts
> Ethernet and loopback devices generally.
>
> Conditions to recreate the bug:
> - egress tc filter on lo: action ife decode pipe;
> - AF_PACKET SOCK_RAW frame: outer Ethernet with ETH_P_IFE, IFE metadata,
>   inner Ethernet and an 8192-byte payload;
> - the egress pipe re-enters eth_type_trans() via loopback_xmit() after the
>   action consumed the inner header.
>
> Fixes: ef6980b6becb ("introduce IFE action")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> ---

Reviewed-by: Eric Dumazet <edumazet@kernel.org>

Re: [PATCH net v2 3/6] net/sched: act_mirred: reject redirects without a link-layer header

[Eric Dumazet]

Le ven. 23 oct. 2026 à 13:59, Jamal Hadi Salim <jhs@mojatatu.com> a écrit :
>
> The issue:
> tcf_mirred_to_dev() may redirect a packet that has no link-layer header to
> a device that expects one.  TUN builds a valid L3 skb with mac_len == 0;
> mirred then pushes zero bytes and the IP header is parsed as Ethernet by
> eth_type_trans().  The same happens after a VLAN POP_ETH or MPLS POP chain
> whose producer did not leave a full inner Ethernet header.
>
> The Fix:
> After the existing push/pull adjustment, when the target expects data at
> the MAC header, require a link-layer header of at least
> dev->min_header_len bytes and pull it into the linear area.  Drop the
> packet otherwise.  This mirrors the check bpf_redirect() already performs
> and keeps the legitimate push_eth-then-mirred case working.
>
> Conditions to recreate the bug:
> - create a TUN device with IFF_TUN | IFF_VNET_HDR | IFF_NO_PI;
> - attach egress mirred redirect to an Ethernet device (for example lo);
> - write a virtio_net_hdr with hdr_len=1 and an 8192-byte payload.
> The unfixed kernel parses the IP header as Ethernet and can BUG in
> eth_type_trans() via loopback_xmit.
>
> Fixes: 53592b364001 ("net/sched: act_mirred: Implement ingress actions")
> Suggested-by: Eric Dumazet <edumazet@kernel.org>
> Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> ---
>  net/sched/act_mirred.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index 553342c55cf7..e3e0e4aa70a1 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -316,6 +316,11 @@ static int tcf_mirred_to_dev(struct sk_buff *skb, struct tcf_mirred *m,
>                 }
>         }
>
> +       if (!expects_nh &&
> +           (unlikely(skb_network_offset(skb_to_send) < dev->min_header_len) ||
> +            !pskb_may_pull(skb_to_send, dev->min_header_len)))
> +               goto err_cant_do;
> +

Oops, sorry this breaks tools/testing/selftests/net/forwarding/tc_mpls_l2vpn.sh

My bad, the skb_network_offset() part I suggested in v1 is wrong.

Please remove it:

diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index b0390b08600c4a709b18f6d92aebe6629081c735..0bd73c5072a90d3e952d0a724461adcf0307592e
100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -317,8 +317,7 @@ static int tcf_mirred_to_dev(struct sk_buff *skb,
struct tcf_mirred *m,
        }

        if (!expects_nh &&
-           (unlikely(skb_network_offset(skb_to_send) < dev->min_header_len) ||
-            !pskb_may_pull(skb_to_send, dev->min_header_len)))
+           !pskb_may_pull(skb_to_send, dev->min_header_len))
                goto err_cant_do;

        skb_to_send->skb_iif = skb->dev->ifindex;

Keep only the pskb_may_pull() which is enough to prevent
eth_type_trans() from crashing on a short linear part.

Thanks!

Re: [PATCH net v2 3/6] net/sched: act_mirred: reject redirects without a link-layer header

[Jamal Hadi Salim]

On Fri, Oct 23, 2026 at 9:17 AM Eric Dumazet <edumazet@kernel.org> wrote:
>
> Le ven. 23 oct. 2026 à 13:59, Jamal Hadi Salim <jhs@mojatatu.com> a écrit :
> >
> > The issue:
> > tcf_mirred_to_dev() may redirect a packet that has no link-layer header to
> > a device that expects one.  TUN builds a valid L3 skb with mac_len == 0;
> > mirred then pushes zero bytes and the IP header is parsed as Ethernet by
> > eth_type_trans().  The same happens after a VLAN POP_ETH or MPLS POP chain
> > whose producer did not leave a full inner Ethernet header.
> >
> > The Fix:
> > After the existing push/pull adjustment, when the target expects data at
> > the MAC header, require a link-layer header of at least
> > dev->min_header_len bytes and pull it into the linear area.  Drop the
> > packet otherwise.  This mirrors the check bpf_redirect() already performs
> > and keeps the legitimate push_eth-then-mirred case working.
> >
> > Conditions to recreate the bug:
> > - create a TUN device with IFF_TUN | IFF_VNET_HDR | IFF_NO_PI;
> > - attach egress mirred redirect to an Ethernet device (for example lo);
> > - write a virtio_net_hdr with hdr_len=1 and an 8192-byte payload.
> > The unfixed kernel parses the IP header as Ethernet and can BUG in
> > eth_type_trans() via loopback_xmit.
> >
> > Fixes: 53592b364001 ("net/sched: act_mirred: Implement ingress actions")
> > Suggested-by: Eric Dumazet <edumazet@kernel.org>
> > Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > ---
> >  net/sched/act_mirred.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> > index 553342c55cf7..e3e0e4aa70a1 100644
> > --- a/net/sched/act_mirred.c
> > +++ b/net/sched/act_mirred.c
> > @@ -316,6 +316,11 @@ static int tcf_mirred_to_dev(struct sk_buff *skb, struct tcf_mirred *m,
> >                 }
> >         }
> >
> > +       if (!expects_nh &&
> > +           (unlikely(skb_network_offset(skb_to_send) < dev->min_header_len) ||
> > +            !pskb_may_pull(skb_to_send, dev->min_header_len)))
> > +               goto err_cant_do;
> > +
>
> Oops, sorry this breaks tools/testing/selftests/net/forwarding/tc_mpls_l2vpn.sh
>
> My bad, the skb_network_offset() part I suggested in v1 is wrong.
>
> Please remove it:
>
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index b0390b08600c4a709b18f6d92aebe6629081c735..0bd73c5072a90d3e952d0a724461adcf0307592e
> 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -317,8 +317,7 @@ static int tcf_mirred_to_dev(struct sk_buff *skb,
> struct tcf_mirred *m,
>         }
>
>         if (!expects_nh &&
> -           (unlikely(skb_network_offset(skb_to_send) < dev->min_header_len) ||
> -            !pskb_may_pull(skb_to_send, dev->min_header_len)))
> +           !pskb_may_pull(skb_to_send, dev->min_header_len))
>                 goto err_cant_do;
>
>         skb_to_send->skb_iif = skb->dev->ifindex;
>
> Keep only the pskb_may_pull() which is enough to prevent
> eth_type_trans() from crashing on a short linear part.
>

Grrr.. Eric, you are not allowed to comment after this.
I should include running those tests going forward when appropriate. I
only test tdc and reproducers.

cheers,
jamal
> Thanks!

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

[Eric Dumazet]

Le ven. 23 oct. 2026 à 13:59, Jamal Hadi Salim <jhs@mojatatu.com> a écrit :
>
> 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().
>

Reviewed-by: Eric Dumazet<edumazet@kernel.org>

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

[Willem de Bruijn]

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?

More importantly, can it hide a bug?

Simply running pskb_may_pull is cheap if no pull is needed.

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

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

[Jamal Hadi Salim]

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?

cheers,
jamal

> Simply running pskb_may_pull is cheap if no pull is needed.
> >               break;
> >       case TCA_MPLS_ACT_PUSH:
> >               new_lse = tcf_mpls_get_lse(NULL, p, !eth_p_mpls(skb_protocol(skb, true)));
> > --
> > 2.43.0
> >
> >
>
>

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

[Jamal Hadi Salim]

On Fri, Oct 23, 2026 at 2:09 PM Jamal Hadi Salim <jhs@mojatatu.com> 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?

Lol - never mind, stoopid question.

cheers,
jamal

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

[Willem de Bruijn]

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

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

[Jamal Hadi Salim]

. On Fri, Oct 23, 2026 at 6:50 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> 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.
>

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

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

[Jamal Hadi Salim]

On Fri, Oct 23, 2026 at 7:46 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> . On Fri, Oct 23, 2026 at 6:50 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > 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.
> >
>
> 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
>

I am going to send it without this 1/6 change - it just doesn't feel
like good taste. I will add a comment in the changes to say i
considered it but decided not to. I also spent time eyeballing the
code, i dont think there's a potential way to craft a packet that will
hit the perceived issue. In any case the 24 hour embargo lifts in a
few hours so i am not posting yet.

cheers,
jamal

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

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

[Willem de Bruijn]

Jamal Hadi Salim wrote:
> On Fri, Oct 23, 2026 at 7:46 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > . On Fri, Oct 23, 2026 at 6:50 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > 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.
> > >
> >
> > 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
> >
> 
> I am going to send it without this 1/6 change - it just doesn't feel
> like good taste. I will add a comment in the changes to say i
> considered it but decided not to. I also spent time eyeballing the
> code, i dont think there's a potential way to craft a packet that will
> hit the perceived issue. In any case the 24 hour embargo lifts in a
> few hours so i am not posting yet.
> 
> cheers,
> jamal
> 
> > thoughts?

I'm actually a little confused why the tc layer has to anticipate the
needs of downstream code and pull headers here.

That directly lead to this guessing of what may be the next header,
such as Ethernet. Which may often be correct, but not always.

All protocol parsing code itself should already be robust and call
pskb_may_pull where it is needed. Sounds like there are some targets
post TC where that is not true *and* we think there are too many to
update those targets directly, rather than this transformation logic
that exposes them to unpulled packets?

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

[Jamal Hadi Salim]

On Sat, Oct 24, 2026 at 12:14 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jamal Hadi Salim wrote:
> > On Fri, Oct 23, 2026 at 7:46 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > >
> > > . On Fri, Oct 23, 2026 at 6:50 PM Willem de Bruijn
> > > <willemdebruijn.kernel@gmail.com> wrote:
> > > >
> > > > 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.
> > > >
> > >
> > > 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
> > >
> >
> > I am going to send it without this 1/6 change - it just doesn't feel
> > like good taste. I will add a comment in the changes to say i
> > considered it but decided not to. I also spent time eyeballing the
> > code, i dont think there's a potential way to craft a packet that will
> > hit the perceived issue. In any case the 24 hour embargo lifts in a
> > few hours so i am not posting yet.
> >
> > cheers,
> > jamal
> >
> > > thoughts?
>
> I'm actually a little confused why the tc layer has to anticipate the
> needs of downstream code and pull headers here.
>
> That directly lead to this guessing of what may be the next header,
> such as Ethernet. Which may often be correct, but not always.
>
> All protocol parsing code itself should already be robust and call
> pskb_may_pull where it is needed. Sounds like there are some targets
> post TC where that is not true *and* we think there are too many to
> update those targets directly, rather than this transformation logic
> that exposes them to unpulled packets?

Not "too many updates" rather the main motivation here (which i kinda
empathize with) is pushback from Eric that you have to "fix at the
producer" to avoid performance degradation at the "consumer". i.e
rather fix the vlan action than have loopback vet as in the original
patch  (and you just witnessed him stand up and push back on tun for
that same reason ;->).
IOW, we are anticipating so that eth type transfer doesnt bug out
(_for ethernet headers_) when invoked at the downstream consumer
(loopback etc). Essentially it is a game of whack-a-mole until the
next bug or malicious use shows up.
Generally, policing intoxicated drivers from getting on the autobahn
isn't the best philosophy; it's better to complement that with a
"driver beware, you may encounter drunks" warning but that would slow
down the fast car on the autobahn ;->

cheers,
jamal

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

[Jamal Hadi Salim]

On Sun, Oct 25, 2026 at 4:55 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Sat, Oct 24, 2026 at 12:14 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Jamal Hadi Salim wrote:
> > > On Fri, Oct 23, 2026 at 7:46 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > > >
> > > > . On Fri, Oct 23, 2026 at 6:50 PM Willem de Bruijn
> > > > <willemdebruijn.kernel@gmail.com> wrote:
> > > > >
> > > > > 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.
> > > > >
> > > >
> > > > 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
> > > >
> > >
> > > I am going to send it without this 1/6 change - it just doesn't feel
> > > like good taste. I will add a comment in the changes to say i
> > > considered it but decided not to. I also spent time eyeballing the
> > > code, i dont think there's a potential way to craft a packet that will
> > > hit the perceived issue. In any case the 24 hour embargo lifts in a
> > > few hours so i am not posting yet.
> > >
> > > cheers,
> > > jamal
> > >
> > > > thoughts?
> >
> > I'm actually a little confused why the tc layer has to anticipate the
> > needs of downstream code and pull headers here.
> >
> > That directly lead to this guessing of what may be the next header,
> > such as Ethernet. Which may often be correct, but not always.
> >
> > All protocol parsing code itself should already be robust and call
> > pskb_may_pull where it is needed. Sounds like there are some targets
> > post TC where that is not true *and* we think there are too many to
> > update those targets directly, rather than this transformation logic
> > that exposes them to unpulled packets?
>
> Not "too many updates" rather the main motivation here (which i kinda
> empathize with) is pushback from Eric that you have to "fix at the
> producer" to avoid performance degradation at the "consumer". i.e
> rather fix the vlan action than have loopback vet as in the original
> patch  (and you just witnessed him stand up and push back on tun for
> that same reason ;->).
> IOW, we are anticipating so that eth type transfer doesnt bug out
> (_for ethernet headers_) when invoked at the downstream consumer
> (loopback etc). Essentially it is a game of whack-a-mole until the
> next bug or malicious use shows up.
> Generally, policing intoxicated drivers from getting on the autobahn
> isn't the best philosophy; it's better to complement that with a
> "driver beware, you may encounter drunks" warning but that would slow
> down the fast car on the autobahn ;->

Actually the more i think about this the more i am concluding just
fixing the producers wont be sufficient (Eric!).
If i have an incompetent or malicious ebpf program it could do
whatever it wants.. I am going to try something with ebpf.

cheers,
jamal

> cheers,
> jamal

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

[Jamal Hadi Salim]

On Sun, Oct 25, 2026 at 3:36 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Sun, Oct 25, 2026 at 4:55 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > On Sat, Oct 24, 2026 at 12:14 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > Jamal Hadi Salim wrote:
> > > > On Fri, Oct 23, 2026 at 7:46 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > > > >
> > > > > . On Fri, Oct 23, 2026 at 6:50 PM Willem de Bruijn
> > > > > <willemdebruijn.kernel@gmail.com> wrote:
> > > > > >
> > > > > > 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.
> > > > > >
> > > > >
> > > > > 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
> > > > >
> > > >
> > > > I am going to send it without this 1/6 change - it just doesn't feel
> > > > like good taste. I will add a comment in the changes to say i
> > > > considered it but decided not to. I also spent time eyeballing the
> > > > code, i dont think there's a potential way to craft a packet that will
> > > > hit the perceived issue. In any case the 24 hour embargo lifts in a
> > > > few hours so i am not posting yet.
> > > >
> > > > cheers,
> > > > jamal
> > > >
> > > > > thoughts?
> > >
> > > I'm actually a little confused why the tc layer has to anticipate the
> > > needs of downstream code and pull headers here.
> > >
> > > That directly lead to this guessing of what may be the next header,
> > > such as Ethernet. Which may often be correct, but not always.
> > >
> > > All protocol parsing code itself should already be robust and call
> > > pskb_may_pull where it is needed. Sounds like there are some targets
> > > post TC where that is not true *and* we think there are too many to
> > > update those targets directly, rather than this transformation logic
> > > that exposes them to unpulled packets?
> >
> > Not "too many updates" rather the main motivation here (which i kinda
> > empathize with) is pushback from Eric that you have to "fix at the
> > producer" to avoid performance degradation at the "consumer". i.e
> > rather fix the vlan action than have loopback vet as in the original
> > patch  (and you just witnessed him stand up and push back on tun for
> > that same reason ;->).
> > IOW, we are anticipating so that eth type transfer doesnt bug out
> > (_for ethernet headers_) when invoked at the downstream consumer
> > (loopback etc). Essentially it is a game of whack-a-mole until the
> > next bug or malicious use shows up.
> > Generally, policing intoxicated drivers from getting on the autobahn
> > isn't the best philosophy; it's better to complement that with a
> > "driver beware, you may encounter drunks" warning but that would slow
> > down the fast car on the autobahn ;->
>
> Actually the more i think about this the more i am concluding just
> fixing the producers wont be sufficient (Eric!).
> If i have an incompetent or malicious ebpf program it could do
> whatever it wants.. I am going to try something with ebpf.

And the reason i am bringing up ebpf is because i noticed sashikos
after like several iterations of a patch find a way to sneak in some
ebpf corner case. So i am sure that is coming (if indeed an issue can
be reproduced)

cheers,
jamal

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

[Jamal Hadi Salim]

On Sun, Oct 25, 2026 at 3:39 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Sun, Oct 25, 2026 at 3:36 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > On Sun, Oct 25, 2026 at 4:55 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > >
> > > On Sat, Oct 24, 2026 at 12:14 PM Willem de Bruijn
> > > <willemdebruijn.kernel@gmail.com> wrote:
> > > >
> > > > Jamal Hadi Salim wrote:
> > > > > On Fri, Oct 23, 2026 at 7:46 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > > > > >
> > > > > > . On Fri, Oct 23, 2026 at 6:50 PM Willem de Bruijn
> > > > > > <willemdebruijn.kernel@gmail.com> wrote:
> > > > > > >
> > > > > > > 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.
> > > > > > >
> > > > > >
> > > > > > 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
> > > > > >
> > > > >
> > > > > I am going to send it without this 1/6 change - it just doesn't feel
> > > > > like good taste. I will add a comment in the changes to say i
> > > > > considered it but decided not to. I also spent time eyeballing the
> > > > > code, i dont think there's a potential way to craft a packet that will
> > > > > hit the perceived issue. In any case the 24 hour embargo lifts in a
> > > > > few hours so i am not posting yet.
> > > > >
> > > > > cheers,
> > > > > jamal
> > > > >
> > > > > > thoughts?
> > > >
> > > > I'm actually a little confused why the tc layer has to anticipate the
> > > > needs of downstream code and pull headers here.
> > > >
> > > > That directly lead to this guessing of what may be the next header,
> > > > such as Ethernet. Which may often be correct, but not always.
> > > >
> > > > All protocol parsing code itself should already be robust and call
> > > > pskb_may_pull where it is needed. Sounds like there are some targets
> > > > post TC where that is not true *and* we think there are too many to
> > > > update those targets directly, rather than this transformation logic
> > > > that exposes them to unpulled packets?
> > >
> > > Not "too many updates" rather the main motivation here (which i kinda
> > > empathize with) is pushback from Eric that you have to "fix at the
> > > producer" to avoid performance degradation at the "consumer". i.e
> > > rather fix the vlan action than have loopback vet as in the original
> > > patch  (and you just witnessed him stand up and push back on tun for
> > > that same reason ;->).
> > > IOW, we are anticipating so that eth type transfer doesnt bug out
> > > (_for ethernet headers_) when invoked at the downstream consumer
> > > (loopback etc). Essentially it is a game of whack-a-mole until the
> > > next bug or malicious use shows up.
> > > Generally, policing intoxicated drivers from getting on the autobahn
> > > isn't the best philosophy; it's better to complement that with a
> > > "driver beware, you may encounter drunks" warning but that would slow
> > > down the fast car on the autobahn ;->
> >
> > Actually the more i think about this the more i am concluding just
> > fixing the producers wont be sufficient (Eric!).
> > If i have an incompetent or malicious ebpf program it could do
> > whatever it wants.. I am going to try something with ebpf.
>
> And the reason i am bringing up ebpf is because i noticed sashikos
> after like several iterations of a patch find a way to sneak in some
> ebpf corner case. So i am sure that is coming (if indeed an issue can
> be reproduced)

confirmed.

    #include <linux/bpf.h>
    #include <linux/pkt_cls.h>
    #define SEC(NAME) __attribute__((section(NAME), used))
    static long (*bpf_skb_adjust_room)(void *skb, __s32 len_diff,
                                       __u32 mode, __u64 flags) =
            (void *)BPF_FUNC_skb_adjust_room;
    SEC("action")
    int shrink(struct __sk_buff *skb)
    {
            bpf_skb_adjust_room(skb, -1, BPF_ADJ_ROOM_MAC, 0);
            return TC_ACT_OK;
    }
    char _license[] SEC("license") = "GPL";


 ip link set lo up
    tc qdisc add dev lo clsact
    tc filter add dev lo egress matchall \
        action vlan pop_eth pipe \
        action bpf object-file /mnt/shrink.o section action

tc filter show

    action order 1: vlan  pop_eth pipe
    action order 2: bpf shrink.o:[action] ... jited default-action pipe

And boom:
    [    6.748140] kernel BUG at ./include/linux/skbuff.h:2830!
    [    6.748154] Oops: invalid opcode: 0000 [#1] SMP KASAN NOPTI
    [    6.749434] CPU: 1 UID: 0 PID: 301 Comm: mini_poc Not tainted
7.3.0-rc7+ #271 PREEMPT(full)
    [    6.750115] RIP: 0010:eth_type_trans+0x335/0x500
    ...
    [    6.757723]  loopback_xmit+0x1c1/0x580
    ...
    [    6.778076] Kernel panic - not syncing: Fatal exception in interrupt

Eric, any ideas?

cheers,
jamal

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

[Jamal Hadi Salim]

On Sun, Oct 25, 2026 at 4:50 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Sun, Oct 25, 2026 at 3:39 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > On Sun, Oct 25, 2026 at 3:36 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > >
> > > On Sun, Oct 25, 2026 at 4:55 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > > >
> > > > On Sat, Oct 24, 2026 at 12:14 PM Willem de Bruijn
> > > > <willemdebruijn.kernel@gmail.com> wrote:
> > > > >
> > > > > Jamal Hadi Salim wrote:
> > > > > > On Fri, Oct 23, 2026 at 7:46 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > > > > > >
> > > > > > > . On Fri, Oct 23, 2026 at 6:50 PM Willem de Bruijn
> > > > > > > <willemdebruijn.kernel@gmail.com> wrote:
> > > > > > > >
> > > > > > > > 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.
> > > > > > > >
> > > > > > >
> > > > > > > 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
> > > > > > >
> > > > > >
> > > > > > I am going to send it without this 1/6 change - it just doesn't feel
> > > > > > like good taste. I will add a comment in the changes to say i
> > > > > > considered it but decided not to. I also spent time eyeballing the
> > > > > > code, i dont think there's a potential way to craft a packet that will
> > > > > > hit the perceived issue. In any case the 24 hour embargo lifts in a
> > > > > > few hours so i am not posting yet.
> > > > > >
> > > > > > cheers,
> > > > > > jamal
> > > > > >
> > > > > > > thoughts?
> > > > >
> > > > > I'm actually a little confused why the tc layer has to anticipate the
> > > > > needs of downstream code and pull headers here.
> > > > >
> > > > > That directly lead to this guessing of what may be the next header,
> > > > > such as Ethernet. Which may often be correct, but not always.
> > > > >
> > > > > All protocol parsing code itself should already be robust and call
> > > > > pskb_may_pull where it is needed. Sounds like there are some targets
> > > > > post TC where that is not true *and* we think there are too many to
> > > > > update those targets directly, rather than this transformation logic
> > > > > that exposes them to unpulled packets?
> > > >
> > > > Not "too many updates" rather the main motivation here (which i kinda
> > > > empathize with) is pushback from Eric that you have to "fix at the
> > > > producer" to avoid performance degradation at the "consumer". i.e
> > > > rather fix the vlan action than have loopback vet as in the original
> > > > patch  (and you just witnessed him stand up and push back on tun for
> > > > that same reason ;->).
> > > > IOW, we are anticipating so that eth type transfer doesnt bug out
> > > > (_for ethernet headers_) when invoked at the downstream consumer
> > > > (loopback etc). Essentially it is a game of whack-a-mole until the
> > > > next bug or malicious use shows up.
> > > > Generally, policing intoxicated drivers from getting on the autobahn
> > > > isn't the best philosophy; it's better to complement that with a
> > > > "driver beware, you may encounter drunks" warning but that would slow
> > > > down the fast car on the autobahn ;->
> > >
> > > Actually the more i think about this the more i am concluding just
> > > fixing the producers wont be sufficient (Eric!).
> > > If i have an incompetent or malicious ebpf program it could do
> > > whatever it wants.. I am going to try something with ebpf.
> >
> > And the reason i am bringing up ebpf is because i noticed sashikos
> > after like several iterations of a patch find a way to sneak in some
> > ebpf corner case. So i am sure that is coming (if indeed an issue can
> > be reproduced)
>
> confirmed.
>
>     #include <linux/bpf.h>
>     #include <linux/pkt_cls.h>
>     #define SEC(NAME) __attribute__((section(NAME), used))
>     static long (*bpf_skb_adjust_room)(void *skb, __s32 len_diff,
>                                        __u32 mode, __u64 flags) =
>             (void *)BPF_FUNC_skb_adjust_room;
>     SEC("action")
>     int shrink(struct __sk_buff *skb)
>     {
>             bpf_skb_adjust_room(skb, -1, BPF_ADJ_ROOM_MAC, 0);
>             return TC_ACT_OK;
>     }
>     char _license[] SEC("license") = "GPL";
>
>
>  ip link set lo up
>     tc qdisc add dev lo clsact
>     tc filter add dev lo egress matchall \
>         action vlan pop_eth pipe \
>         action bpf object-file /mnt/shrink.o section action
>
> tc filter show
>
>     action order 1: vlan  pop_eth pipe
>     action order 2: bpf shrink.o:[action] ... jited default-action pipe
>
> And boom:
>     [    6.748140] kernel BUG at ./include/linux/skbuff.h:2830!
>     [    6.748154] Oops: invalid opcode: 0000 [#1] SMP KASAN NOPTI
>     [    6.749434] CPU: 1 UID: 0 PID: 301 Comm: mini_poc Not tainted
> 7.3.0-rc7+ #271 PREEMPT(full)
>     [    6.750115] RIP: 0010:eth_type_trans+0x335/0x500
>     ...
>     [    6.757723]  loopback_xmit+0x1c1/0x580
>     ...
>     [    6.778076] Kernel panic - not syncing: Fatal exception in interrupt
>
> Eric, any ideas?
>

And worse, i just tested a non ethernet next header for mpls (the TEB
check is a bad idea) and it killed the kernel as well:

tc filter add dev lo egress protocol all flower action vlan pop_eth
pipe action mpls pop protocol ip pipe action  gact pass
BUG at include/linux/skbuff.h:2830 in eth_type_trans  via loopback_xmit

I am going to withdraw this series - lets reach some consensus first.

cheers,
jamal
> cheers,
> jamal

Re: [PATCH net v2 1/6] net/sched: act_vlan: pull the inner Ethernet header after POP_ETH

[Eric Dumazet]

Le ven. 23 oct. 2026 à 13:59, Jamal Hadi Salim <jhs@mojatatu.com> a écrit :
>
> The issue:
> act_vlan POP_ETH removes the outer Ethernet header and leaves the inner
> Ethernet header at the head of the packet.  A non-linear skb can have the
> inner header in a page fragment, so a following eth_type_trans() may find
> less than ETH_HLEN bytes in the linear area and BUG in __skb_pull().
>
> The Fix:
> Pull the inner Ethernet header into the linear area after skb_eth_pop():
> drop the packet only when pskb_may_pull(skb, ETH_HLEN) fails.  This keeps
> valid non-linear packets whose inner header sits in frags.
>
> Conditions to recreate the bug:
> - egress tc filter on lo: action vlan pop_eth.
> - AF_PACKET SOCK_RAW with PACKET_VNET_HDR, hdr_len=28, and a non-linear
>   eth / inner-eth frame.
> - The inner header is in page frags; a following eth_type_trans() can BUG
>   in __skb_pull().
>
> Fixes: 19fbcb36a39e ("net/sched: act_vlan: Add {POP,PUSH}_ETH actions")
> 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/willemdebruijn.kernel.38722ee638a21@gmail.com/
> Link: https://lore.kernel.org/netdev/CAL4WiipJLzZ9ywOPaFHZ+wePTOuO+6+aaRCVXMHz+xmmQm2hcA@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>
>

Reviewed-by: Eric Dumazet <edumazet@kernel.org>

Thanks!

Re: [PATCH net v2 0/6] net: pull short linear Ethernet headers before eth_type_trans()

[Jakub Kicinski]

On Fri, 23 Oct 2026 07:59:15 -0400 Jamal Hadi Salim wrote:
> This series fixes a class of producer/consumer bugs in which an outer
> header is popped and the packet is then handed to a consumer with fewer
> than ETH_HLEN bytes in the linear area.  eth_type_trans() pulls ETH_HLEN
> from that area and can BUG in __skb_pull() when the inner Ethernet header
> is not linear.  The affected producers are act_vlan POP_ETH, act_mpls POP,
> act_ife egress decode, and the Open vSwitch MPLS/TEB and NSH pops.  A
> consumer-side guard in act_mirred rejects redirecting a headerless packet
> to a device that expects a link-layer header.
> 
> Patch 1 makes act_vlan POP_ETH pull the inner Ethernet header after the
> pop and drop only when the packet is genuinely shorter than ETH_HLEN.
> Patch 2 does the same for act_mpls, gated on the pre-pop MAC length.
> Patch 3 adds the act_mirred guard at the redirect boundary.  Patch 4
> keeps act_ife egress decode from consuming the inner Ethernet header and
> restores the header metadata.
> 
> Patches 5 and 6 establish the same linear-header invariant for the OVS
> MPLS/TEB and NSH pops before restoring MAC_PROTO_ETHERNET.

NIPA CI flagged a regression that appears to be introduced by this patch
(or one of the closely related act_vlan/act_mirred patches in the same
series).

The selftest tools/testing/selftests/net/forwarding/tc_mpls_l2vpn.sh now
fails reliably (both the "ping" and "ping6" sub-tests, on both the
regular and debug forwarding test configs, consistently on retry as
well):

  # TEST: ping    [FAIL]
  # TEST: ping6   [FAIL]

The test builds an Ethernet-over-MPLS L2VPN using exactly the tc action
sequence this series touches:

  - ingress: mpls mac_push + vlan push_eth + mirred egress redirect
  - ingress: vlan pop_eth + mpls pop protocol teb + mirred egress redirect

dmesg on the test run shows mirred failing to redirect the traffic:

  tc mirred to Houston: device veth2 is down
  tc mirred to Houston: device veth3 is down

This is the same "vlan pop_eth followed by mpls pop" chain described in
this patch's own commit message as the bug reproduction scenario, so it
looks like the mac_len/pskb_may_pull bookkeeping added here may not be
correctly handling the ingress case used by this test, causing otherwise
valid packets built by this chain to be mishandled and the redirect to
fail.

Could you take a look at whether the new POP header-pull logic correctly
handles the "vlan pop_eth -> mpls pop protocol teb -> mirred redirect"
chain on ingress? It would also be worth double-checking this alongside
the companion act_vlan POP_ETH and act_mirred link-layer-header patches
from the same series, since they touch the same pipeline.

Re: [PATCH net v2 0/6] net: pull short linear Ethernet headers before eth_type_trans()

[Jamal Hadi Salim]

On Fri, Oct 23, 2026 at 3:28 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 23 Oct 2026 07:59:15 -0400 Jamal Hadi Salim wrote:
> > This series fixes a class of producer/consumer bugs in which an outer
> > header is popped and the packet is then handed to a consumer with fewer
> > than ETH_HLEN bytes in the linear area.  eth_type_trans() pulls ETH_HLEN
> > from that area and can BUG in __skb_pull() when the inner Ethernet header
> > is not linear.  The affected producers are act_vlan POP_ETH, act_mpls POP,
> > act_ife egress decode, and the Open vSwitch MPLS/TEB and NSH pops.  A
> > consumer-side guard in act_mirred rejects redirecting a headerless packet
> > to a device that expects a link-layer header.
> >
> > Patch 1 makes act_vlan POP_ETH pull the inner Ethernet header after the
> > pop and drop only when the packet is genuinely shorter than ETH_HLEN.
> > Patch 2 does the same for act_mpls, gated on the pre-pop MAC length.
> > Patch 3 adds the act_mirred guard at the redirect boundary.  Patch 4
> > keeps act_ife egress decode from consuming the inner Ethernet header and
> > restores the header metadata.
> >
> > Patches 5 and 6 establish the same linear-header invariant for the OVS
> > MPLS/TEB and NSH pops before restoring MAC_PROTO_ETHERNET.
>
> NIPA CI flagged a regression that appears to be introduced by this patch
> (or one of the closely related act_vlan/act_mirred patches in the same
> series).
>
> The selftest tools/testing/selftests/net/forwarding/tc_mpls_l2vpn.sh now
> fails reliably (both the "ping" and "ping6" sub-tests, on both the
> regular and debug forwarding test configs, consistently on retry as
> well):
>
>   # TEST: ping    [FAIL]
>   # TEST: ping6   [FAIL]
>
> The test builds an Ethernet-over-MPLS L2VPN using exactly the tc action
> sequence this series touches:
>
>   - ingress: mpls mac_push + vlan push_eth + mirred egress redirect
>   - ingress: vlan pop_eth + mpls pop protocol teb + mirred egress redirect
>
> dmesg on the test run shows mirred failing to redirect the traffic:
>
>   tc mirred to Houston: device veth2 is down
>   tc mirred to Houston: device veth3 is down
>
> This is the same "vlan pop_eth followed by mpls pop" chain described in
> this patch's own commit message as the bug reproduction scenario, so it
> looks like the mac_len/pskb_may_pull bookkeeping added here may not be
> correctly handling the ingress case used by this test, causing otherwise
> valid packets built by this chain to be mishandled and the redirect to
> fail.
>
> Could you take a look at whether the new POP header-pull logic correctly
> handles the "vlan pop_eth -> mpls pop protocol teb -> mirred redirect"
> chain on ingress? It would also be worth double-checking this alongside
> the companion act_vlan POP_ETH and act_mirred link-layer-header patches
> from the same series, since they touch the same pipeline.

Victor monitors NIPA but is currently away for a few days. I will take
a look. Eric pointed out this broke:
tools/testing/selftests/net/forwarding/tc_mpls_l2vpn.sh
I'm working on updating that and will try to fix this as well..

cheers,
jamal

Re: [PATCH net v2 0/6] net: pull short linear Ethernet headers before eth_type_trans()

[Jamal Hadi Salim]

On Fri, Oct 23, 2026 at 4:49 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Fri, Oct 23, 2026 at 3:28 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Fri, 23 Oct 2026 07:59:15 -0400 Jamal Hadi Salim wrote:
> > > This series fixes a class of producer/consumer bugs in which an outer
> > > header is popped and the packet is then handed to a consumer with fewer
> > > than ETH_HLEN bytes in the linear area.  eth_type_trans() pulls ETH_HLEN
> > > from that area and can BUG in __skb_pull() when the inner Ethernet header
> > > is not linear.  The affected producers are act_vlan POP_ETH, act_mpls POP,
> > > act_ife egress decode, and the Open vSwitch MPLS/TEB and NSH pops.  A
> > > consumer-side guard in act_mirred rejects redirecting a headerless packet
> > > to a device that expects a link-layer header.
> > >
> > > Patch 1 makes act_vlan POP_ETH pull the inner Ethernet header after the
> > > pop and drop only when the packet is genuinely shorter than ETH_HLEN.
> > > Patch 2 does the same for act_mpls, gated on the pre-pop MAC length.
> > > Patch 3 adds the act_mirred guard at the redirect boundary.  Patch 4
> > > keeps act_ife egress decode from consuming the inner Ethernet header and
> > > restores the header metadata.
> > >
> > > Patches 5 and 6 establish the same linear-header invariant for the OVS
> > > MPLS/TEB and NSH pops before restoring MAC_PROTO_ETHERNET.
> >
> > NIPA CI flagged a regression that appears to be introduced by this patch
> > (or one of the closely related act_vlan/act_mirred patches in the same
> > series).
> >
> > The selftest tools/testing/selftests/net/forwarding/tc_mpls_l2vpn.sh now
> > fails reliably (both the "ping" and "ping6" sub-tests, on both the
> > regular and debug forwarding test configs, consistently on retry as
> > well):
> >
> >   # TEST: ping    [FAIL]
> >   # TEST: ping6   [FAIL]
> >
> > The test builds an Ethernet-over-MPLS L2VPN using exactly the tc action
> > sequence this series touches:
> >
> >   - ingress: mpls mac_push + vlan push_eth + mirred egress redirect
> >   - ingress: vlan pop_eth + mpls pop protocol teb + mirred egress redirect
> >
> > dmesg on the test run shows mirred failing to redirect the traffic:
> >
> >   tc mirred to Houston: device veth2 is down
> >   tc mirred to Houston: device veth3 is down
> >
> > This is the same "vlan pop_eth followed by mpls pop" chain described in
> > this patch's own commit message as the bug reproduction scenario, so it
> > looks like the mac_len/pskb_may_pull bookkeeping added here may not be
> > correctly handling the ingress case used by this test, causing otherwise
> > valid packets built by this chain to be mishandled and the redirect to
> > fail.
> >
> > Could you take a look at whether the new POP header-pull logic correctly
> > handles the "vlan pop_eth -> mpls pop protocol teb -> mirred redirect"
> > chain on ingress? It would also be worth double-checking this alongside
> > the companion act_vlan POP_ETH and act_mirred link-layer-header patches
> > from the same series, since they touch the same pipeline.
>
> Victor monitors NIPA but is currently away for a few days. I will take
> a look. Eric pointed out this broke:
> tools/testing/selftests/net/forwarding/tc_mpls_l2vpn.sh
> I'm working on updating that and will try to fix this as well..

ok, the new update fixes this. I was planning to wait for the
obligatory 24 hours (so tommorow my AM)- unless you say otherwise..


cheers,
jamal

> cheers,
> jamal