On Tue, Oct 27, 2026 at 11:50 AM Eric Dumazet <[email protected]> wrote:
>
> 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).
I didnt add my reviewed-by or tested-by because i wasnt sure if you
are going to resend and include the patch i sent?
cheers,
jamal
> 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
