Re: [PATCH net v3] seg6: Fix unset mac header in SRv6 End.DT VRF decapsulation

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch restores the mac header only after the VRF
  l3mdev_l3_rcv handler returns.

Pre-existing issues:
- [Low] Concern: the other SRv6 decap behaviours (End.DX4/DX6, End.DT6
  legacy, End.DT46 non-VRF) leave mac_header and mac_len pointing at the…

--

seg6: Fix unset mac header in SRv6 End.DT VRF decapsulation

After the VRF l3mdev_l3_rcv handler returns, this commit restores a valid
mac header in end_dt_vrf_rcv() if the handler did not rebuild one. It
points the mac header at the network header and resets mac_len. With
that, UDP GSO re-segmentation no longer does a huge __skb_push() from an
unset mac header.

> Every other receive path delivers packets with a valid mac header:
> NIC drivers set it in eth_type_trans(), and tunnels without an L2
> header (ipip, sit, gre, tun) reset it to the network header after
> decapsulation.

[Severity: Low]
This isn't a bug introduced by this patch, but is this description
accurate for ipip, sit and gre?

In ip_tunnel_rcv() in net/ipv4/ip_tunnel.c, only the ARPHRD_ETHER case
touches the mac header:

	if (tunnel->dev->type == ARPHRD_ETHER) {
		skb->protocol = eth_type_trans(skb, tunnel->dev);
		skb_postpull_rcsum(skb, eth_hdr(skb), ETH_HLEN);
	} else {
		skb->dev = tunnel->dev;
	}

In the L3 tunnel case the outer mac header stays set. It is not reset to
the network header.

The non-VRF SRv6 decap behaviours (End.DX4/DX6, legacy End.DT6, non-VRF
End.DT46) end up in the same state after decap_and_validate(), which only
resets the network and transport headers.

That stale but set state looks harmless at runtime. __skb_gso_segment()
calls skb_reset_mac_header() and skb_reset_mac_len() before
skb_mac_gso_segment(), and the outer mac header is still inside the
buffer.

Could the commit message wording be adjusted, though?

> diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
> index 53f7b24525759..45d0aabc58152 100644
> --- a/net/ipv6/seg6_local.c
> +++ b/net/ipv6/seg6_local.c
> @@ -1200,6 +1200,16 @@ static struct sk_buff *end_dt_vrf_rcv(struct sk_buff *skb, u16 family,
>  		/* the skb buffer was consumed by the handler */
>  		return NULL;
>  
> +	/* the VRF device had its chance to rebuild the mac header for
> +	 * sniffers; restore a valid one pointing at the network header,
> +	 * like any L3 device without an L2 header, before the packet
> +	 * continues up the stack.
> +	 */
> +	if (!skb_mac_header_was_set(skb)) {
> +		skb_reset_mac_header(skb);
> +		skb_reset_mac_len(skb);
> +	}

[Severity: High]
Does this restore happen early enough? end_dt_vrf_rcv() calls
skb_unset_mac_header() before l3mdev_l3_rcv(). If no sniffer is attached
to the VRF device, the header is still unset when the PRE_ROUTING hook
runs inside the handler:

drivers/net/vrf.c:vrf_ip_rcv() {
    ...
	if (!list_empty(&vrf_dev->ptype_all)) {
		...
		err = vrf_add_mac_header_if_unset(skb, vrf_dev, ETH_P_IP,
						  orig_dev);
		...
	}

	skb = vrf_rcv_nfhook(NFPROTO_IPV4, NF_INET_PRE_ROUTING, skb, vrf_dev);
    ...
}

vrf_ip6_rcv() is structured the same way.

Suppose an NFQUEUE rule queues a GSO packet at that hook, either on a
queue without NFQA_CFG_F_GSO or as an SCTP GSO packet.
nfqnl_enqueue_packet() then does:

	nf_bridge_adjust_skb_data(skb);
	segs = skb_gso_segment(skb, 0);

and nf_bridge_adjust_skb_data() does:

	if (nf_bridge_info_get(skb))
		__skb_push(skb, skb_mac_header_len(skb));

With mac_header == 0xffff, skb_mac_header_len() is network_header - 0xffff,
which is roughly 4G as a u32.

Can that move skb->data far outside the buffer and corrupt skb->len before
skb_gso_segment() reads through it?

The bridge netfilter extension can still be attached at this point. An
IPv6+SRH packet that arrives on a bridge port with br_netfilter and
call-ip6tables enabled gets it from nf_bridge_alloc() in
br_nf_pre_routing_ipv6().

Nothing on this path drops it. br_pass_frame_up(), decap_and_validate(),
iptunnel_pull_offloads() and nf_reset_ct() all leave it in place.
nf_bridge_info_free() is only called on the bridge xmit paths.

ip_sabotage_in() also lets the packet through PRE_ROUTING on the VRF
device. sabotage_in_done was already set during the first PRE_ROUTING
pass on the bridge device:

	if (nf_bridge) {
		if (nf_bridge->sabotage_in_done)
			return NF_ACCEPT;

		if (!nf_bridge->in_prerouting &&
		    !netif_is_l3_master(skb->dev) &&
		    !netif_is_l3_slave(skb->dev)) {

The call chain would be:

ipv6_rcv()
  seg6_local_input_core()
    end_dt_vrf_rcv()
      skb_unset_mac_header()
      vrf_ip_rcv()
        vrf_rcv_nfhook(NF_INET_PRE_ROUTING)
          nfqnl_enqueue_packet()
            nf_bridge_adjust_skb_data()
              __skb_push(skb, huge)
            skb_gso_segment()

This looks like the same problem: an unset End.DT VRF mac header becomes a
huge unsigned __skb_push() before GSO. The difference is that it happens
inside the handler rather than after it returns.

Would it make sense to set a valid mac header before calling
l3mdev_l3_rcv()? Alternatively, the consumers inside the handler could be
made to handle the unset state.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021024738.17266-1-yymhvert%40gmail.com