Le mar. 27 oct. 2026 à 14:07, Jamal Hadi Salim <[email protected]> a écrit : > > On Tue, Oct 27, 2026 at 5:52 AM Eric Dumazet <[email protected]> wrote: > > > > Le mar. 27 oct. 2026 à 10:17, Jamal Hadi Salim <[email protected]> a écrit : > > > > > > On Tue, Oct 27, 2026 at 4:26 AM Eric Dumazet <[email protected]> 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: > > > > > > > > - 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]/ > > > > > > > > > > Looks interesting ;-> I dont think you are covering xdp/bpf holes but > > > we can worry about that later. > > > I will spend time later this morning looking closely and actually > > > running it. Will keep you posted. > > > > Thanks Jamal. > > > > I went through the holes you listed earlier: > > > > 1) bpf_skb_adjust_room() + bpf_redirect(): > > An egress redirect ends in the target ndo_start_xmit(): > > loopback (patch 2), veth (already pulls ETH_HLEN), OVS internal > > port (patch 4). An ingress redirect goes through > > __dev_forward_skb2() (patch 3). The repro in patch 3 uses > > bpf_skb_change_head(), the only helper I found that leaves > > 1 <= mac_len < ETH_HLEN with an almost empty head, passing the > > mac_header < network_header check in __bpf_redirect_common(). > > > > 2) bpf_clone_redirect(): same __bpf_redirect() path as 1). > > > > 3) ingress: after tc ingress, eth_type_trans() is only called again > > if the packet is redirected, see 1). > > > > 4) XDP: bpf_xdp_adjust_head() and bpf_xdp_adjust_tail() refuse to > > leave less than ETH_HLEN bytes in the linear part, and multi-buffer > > tail shrinking only trims frags. skbs built from xdp buffers > > (XDP_PASS, veth XDP, generic XDP) have a full Ethernet header in > > skb->head. > > > > Let's use something that can be plugged / unplugged easily in the future. > > I reviewed and tested all existing issue, they are fixed - kudos ;-> > But, but.... you missed one;-> see attached patch.
Yes, I mostly focused on eth_type_trans(). > > Additional questions: > 1) Would be interesting to see what performance degradation happened here. NIC drivers still use the plain eth_type_trans(). Only loopback, __dev_forward_skb2() and OVS internal ports pay for one inline skb_headlen() comparison. > 2) I think it is reasonable i send my original patches 1-3 to net-next > when it opens as "hardening" patches Yes. > 3) Is it correct to put multiple Fixes: in net patches (you have i > think 4 in one of them)? Sashiko complained once that a patch I sent > made stable backporting hard. Either you pick one or split into > multiple patches. I realize picking a "good" fix tag is very hard in > this case. Yes, both are real origins. Stable can use the oldest. That's common practice. > > cheers, > jamal _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
