Re: [PATCH net v3 3/4] ipmr: set up GSO state for tunnel vifs

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 0

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).

- [Medium] ip_encap() (net/ipv4/ipmr.c:1844-1846) now calls
  iptunnel_handle_offloads(skb, SKB_GSO_IPXIP4) and then…
- [Medium] ip_encap() (net/ipv4/ipmr.c:1862) still sets the outer header
  with `iph->tot_len = htons(skb->len)`.

--

ipmr: set up GSO state for tunnel vifs

This patch changes ip_encap() to call iptunnel_handle_offloads() with
SKB_GSO_IPXIP4 and then skb_set_inner_ipproto(skb, IPPROTO_IPIP), the same
way ipip_tunnel_xmit() does. It also reserves one outer IP ID per segment
through ip_select_ident_segs(). ip_encap() now returns an error, and
ipmr_prepare_xmit() passes that error on.

> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> index b9c544d48c452..6f2eb571f30f7 100644
> --- a/net/ipv4/ipmr.c
> +++ b/net/ipv4/ipmr.c
> @@ -1835,12 +1835,17 @@ static struct notifier_block ip_mr_notifier = {

[ ... ]

> +static int ip_encap(struct net *net, struct sk_buff *skb,
> +		    __be32 saddr, __be32 daddr)
>  {
> +	const struct iphdr *old_iph;
>  	struct iphdr *iph;
> -	const struct iphdr *old_iph = ip_hdr(skb);
>  
> +	if (iptunnel_handle_offloads(skb, SKB_GSO_IPXIP4))
> +		return -1;
> +	skb_set_inner_ipproto(skb, IPPROTO_IPIP);

[Severity: Medium]
What happens here if the GSO skb already has skb->encapsulation set?

ipip_tunnel_xmit() runs as ndo_start_xmit on a device with IPIP_FEATURES
and no hw_enc_features. Because of that, validate_xmit_skb() segments an
already-encapsulated GSO skb in software before it reaches
iptunnel_handle_offloads(). ip_encap() has no such filter.

GRO can hand ipmr skbs like this. For GRE, gre_gro_complete() sets
encapsulation = 1 and SKB_GSO_GRE, and inet_gro_complete() sets
inner_protocol to htons(ETH_P_IP). VXLAN and other UDP tunnels with a local
GRO socket behave the same way.

For these skbs, iptunnel_handle_offloads() skips skb_reset_inner_headers()
because encapsulation is already set. It then ORs SKB_GSO_IPXIP4 into the
existing tunnel gso_type. skb_set_inner_ipproto() overwrites the
inner_ipproto byte of the inner_protocol union and sets
inner_protocol_type to ENCAP_TYPE_IPPROTO.

For GRE, software segmentation goes through
inet_gso_segment()->ipip_gso_segment(), which now succeeds. After that,
gre_gso_segment() does:

net/ipv4/gre_offload.c:gre_gso_segment() {
    ...
	skb->protocol = skb->inner_protocol;
    ...
}

inner_protocol is now 0x0400 instead of 0x0800, so skb_mac_gso_segment()
fails. Doesn't that mean the packet is still dropped in validate_xmit_skb()
after DEV_STATS_INC(tx_packets)? That is the symptom the commit message
says this patch fixes.

For a UDP tunnel, skb_udp_tunnel_segment() takes the ENCAP_TYPE_IPPROTO
branch and calls inet_offloads[IPPROTO_IPIP]. inet_gso_segment() then
parses the inner Ethernet frame as an IPv4 header.

Some NICs have hw_enc_features covering both the existing tunnel type and
IPXIP4. On those, the doubly encapsulated skb would reach hardware with
inner headers that skip the middle tunnel layer.

Remote GRE or VXLAN traffic can reach this. It needs a multicast group
with an MFC entry that has a VIFF_TUNNEL oif:

ip_mr_input()->ipmr_queue_fwd_xmit()->ipmr_prepare_xmit()->ip_encap()

Would it make sense to segment in software, or reject, GSO skbs that
already have skb->encapsulation set before calling
iptunnel_handle_offloads()?

> +
> +	old_iph = ip_hdr(skb);
>  	skb_push(skb, sizeof(struct iphdr));
>  	skb->transport_header = skb->network_header;
>  	skb_reset_network_header(skb);
> @@ -1855,11 +1860,12 @@ static void ip_encap(struct net *net, struct sk_buff *skb,
>  	iph->protocol	=	IPPROTO_IPIP;
>  	iph->ihl	=	5;
>  	iph->tot_len	=	htons(skb->len);

[Severity: Medium]
Can tot_len be truncated here, now that ip_encap() turns GSO skbs into
valid IPXIP4 GSO packets?

The inner IPv4 datagram can be up to 65535 bytes. skb_gro_receive() only
rejects when p->len + len >= GRO_LEGACY_MAX_SIZE for non-TCP, or
>= gro_ipv4_max_size (default 65536) for TCP. Local UDP_SEGMENT is capped
at IP_MAX_MTU.

After the 20-byte push, skb->len can be 65536 to 65555. htons() then
truncates tot_len to 0 to 19, which is less than ihl * 4. With BIG TCP GRO
it can be any truncated value. ip_send_check() then computes the checksum
over that value.

The iptunnel_xmit() path this patch mirrors goes through __ip_local_out(),
which does:

	iph_set_totlen(iph, skb->len);

inet_gro_complete() does the same. iph_set_totlen() writes 0 above
IP_MAX_MTU.

Software segmentation hides the problem, because inet_gso_segment()
rewrites tot_len for each segment. Anything that sees the unsegmented GSO
skb sees the malformed header. That includes FORWARD and POST_ROUTING
netfilter (for example xt_length via iph_totlen()), egress tc/BPF, packet
taps and pass-through devices.

veth, for example, advertises NETIF_F_GSO_ENCAP_ALL in hw_enc_features and
NETIF_F_GSO_SOFTWARE in features. The peer namespace gets the GSO skb
intact, and ip_rcv_core() drops it:

net/ipv4/ip_input.c:ip_rcv_core() {
    ...
	len = iph_totlen(skb, iph);
	if (skb->len < len) {
    ...
	} else if (len < (iph->ihl*4))
		goto inhdr_error;
    ...
}

Both ipmr_queue_fwd_xmit() and ipmr_queue_output_xmit() reach this through
ipmr_prepare_xmit()->ip_encap().

When UDP GRO forwarding, fraglist GRO or TCP GRO aggregates multicast
traffic, a remote sender can pick segment sizes that put the aggregate in
this range. For example, 50 x 1310-byte UDP payloads gives 65528. Once a
VIFF_TUNNEL vif exists, a local UDP_SEGMENT sender can do the same.

At this commit, only DF-clear GSO skbs near 64K get past the MTU check in
ipmr_prepare_xmit(). The issue is still there at the end of the series.
The follow-up patch checks the length of each segment instead, which also
lets DF-set GSO aggregates reach this code.

Should this use iph_set_totlen(iph, skb->len) instead?

> -	ip_select_ident(net, skb, NULL);
> +	ip_select_ident_segs(net, skb, NULL, skb_shinfo(skb)->gso_segs ?: 1);
>  	ip_send_check(iph);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1792293419.git.julius%40bairaktaris.de