Jamal Hadi Salim <[email protected]> writes:
> 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.
>
> Additional questions:
> 1) Would be interesting to see what performance degradation happened here.
> 2) I think it is reasonable i send my original patches 1-3 to net-next
> when it opens as "hardening" patches
> 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.
I don't believe multiple fixes is disallowed. At least for OVS side, we
recently had:
0dbc2398fca3 ("openvswitch: only skb_tx_error() a packet we are about to drop")
This tags multiple fixes (dated Aug 22). And I think 'net' has a few as
well (like 06e3f54e8b22).
Maybe Sashiko was feeling ornery, although the complaint that backports
can be harder is probably true :)
> cheers,
> jamal
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
