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.
You previously suggested catching most of the TC modifications with a
single check in sch_handle_(e|in)gress. Is that no longer needed?
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..
>
> - 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
