Le mar. 27 oct. 2026 à 15:40, Willem de Bruijn
<[email protected]> a écrit :
>
> Eric Dumazet wrote:
> > eth_type_trans() assumes ETH_HLEN bytes are present in skb->head.
> > NIC drivers always provide this, and they should keep using the fast
> > version, without an extra pskb_may_pull().
> >
> > Devices looping skbs from their transmit path back to their receive
> > path get skbs built by the stack instead. tc actions (act_vlan pop_eth,
> > act_ife decode), BPF helpers (bpf_skb_change_head() before a redirect)
> > or OVS actions (pop_eth followed by pop_mpls/pop_nsh) can leave fewer
> > than ETH_HLEN bytes in the linear part, and eth_type_trans() then hits
> > the BUG() in __skb_pull().
> >
> > Instead of fixing every producer, or adding a check to all
> > eth_type_trans() users, fix the few consumers exposed to this:
>
> Definitely a nice approach.
Thanks!
>
> You previously suggested catching most of the TC modifications with a
> single check in sch_handle_(e|in)gress. Is that no longer needed?
Not for these crashes. The sch_handle_egress() check would only cover
tc egress. It would miss OVS and the redirect paths you mention, and it
would add a check for every packet going through clsact. Fixing the
consumers covers all producers, including future ones, at almost no
cost. NIC drivers are not affected.
We can still add it later as hardening, if another class of bugs shows up.
>
> The few mirred/redirect cases are a bane, causing so many issues. And
> escape any such check, as they call netif_receive_skb, netif_rx,
> dev_queue_xmit, directly.
>
> If it's only act_mirred, bpf_redirect and OVS, maybe a single
> validation helper across all three before they pass to the rx/tx
> path, that for starters calls eth_type_trans_careful. It quite likely
> won't be the last packet corruption they are capable of..
I agree these are the problems, and Jamal just found another one in
__bpf_redirect_neigh(), which does its own skb_pull(ETH_HLEN).
A single helper is tempting, but what it must check depends on the
target:
- Egress to an Ethernet device: ETH_HLEN at skb_mac_header().
- Egress to an L3 device (tun, ipip, wireguard...): no MAC header at all.
- Ingress: data at the network header. eth_type_trans() is not
called again, except through __dev_forward_skb2() (patch 3).
Also, the original loopback crash needs no redirect at all:
"vlan pop_eth" on lo egress is enough.
So I would keep this series as the minimal fix for net/stable, and
look at a common helper for net-next, probably based on
dev->min_header_len / dev->hard_header_len of the target device,
called from tcf_mirred_forward(), skb_do_redirect() and OVS do_output().
Thanks!
>
> >
> > - Patch 1 adds eth_type_trans_careful(), which pulls the Ethernet
> > header first and returns a drop reason on failure.
> > - Patch 2 uses it in loopback_xmit().
> > - Patch 3 uses it in __dev_forward_skb2(), used by BPF redirects to
> > ingress and by veth, macvlan, ipvlan, netdevsim and l2tp_eth.
> > - Patch 4 uses it in OVS internal_dev_recv(), covering both the MPLS
> > (TEB) and NSH pop cases at once (only the MPLS case was tested).
> > - Patch 5 (from Jamal) fixes act_ife decode on egress, which was
> > consuming the inner Ethernet header. This is no longer a crash after
> > patch 2, but egress decode never delivered a correct frame.
> >
> > Other eth_type_trans() users already pull the header or never see
> > such skbs:
> > - vrf_process_v{4,6}_outbound() already pull ETH_HLEN + IP header
> > before vrf_local_xmit().
> > - tun already uses pskb_may_pull_reason(skb, ETH_HLEN).
> > - XDP: bpf_xdp_adjust_head() and bpf_xdp_adjust_tail() keep at least
> > ETH_HLEN bytes in the linear part, and multi-buffer tail shrinking
> > only trims frags.
> >
> > Tested under virtme-ng with AF_PACKET reproducers. Without the series,
> > all four cases crash in eth_type_trans():
> > - lo egress "action vlan pop_eth", eth/eth/8192 bytes frame
> > - lo egress "action ife decode pipe", IFE frame with 8192 bytes payload
> > - lo egress "action vlan pop_eth continue", then a BPF program doing
> > bpf_skb_change_head(skb, 1, 0) + bpf_redirect(veth1, BPF_F_INGRESS)
> > - OVS flow "pop_eth,pop_mpls(0x6558),0" to the internal port,
> > eth/mpls/eth/4096 bytes frame
> > With the series, no crash. An ICMP echo inside an IFE frame decoded
> > on lo egress is now received and answered. Normal traffic on lo,
> > veth (including 8000 byte pings) and vrf is unchanged.
> >
> > Link:
> > https://lore.kernel.org/netdev/[email protected]/
> > Link:
> > https://lore.kernel.org/netdev/[email protected]/
> >
> > Eric Dumazet (4):
> > net: add eth_type_trans_careful()
> > loopback: use eth_type_trans_careful()
> > net: use eth_type_trans_careful() in __dev_forward_skb2()
> > openvswitch: use eth_type_trans_careful() in internal_dev_recv()
> >
> > Jamal Hadi Salim (1):
> > net/sched: act_ife: do not consume the inner header on egress
> >
> > drivers/net/loopback.c | 8 +++++++-
> > include/linux/etherdevice.h | 28 ++++++++++++++++++++++++++++
> > net/core/dev.c | 18 +++++++++++++-----
> > net/openvswitch/vport-internal_dev.c | 8 +++++++-
> > net/sched/act_ife.c | 4 ++++
> > 5 files changed, 59 insertions(+), 7 deletions(-)
> >
> > --
> > 2.53.0
> >
>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
