Re: [PATCH net-next] ipv4: use zero IPID for atomic datagrams on connected sockets

[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: 1 · Low: 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).

- [Medium] TCP TSO/GSO skbs now carry IPv4 IDs that restart at 0 on every
  skb.
- [Low] The kernel-doc for struct inet_sock (include/net/inet_sock.h)
  still says `@inet_id - ID counter for DF pkts`.

--

ipv4: use zero IPID for atomic datagrams on connected sockets

This patch moves the DF && !ignore_df check in ip_select_ident_segs()
ahead of the per-socket private generator. On connected sockets, atomic
datagrams now get an IP ID of zero, and inet_id is used only for non-DF
packets or packets with skb->ignore_df set. The stated goal is to avoid
an atomic operation on a shared cache line and to keep TCP from touching
inet->inet_id in the fast path.

> diff --git a/include/net/ip.h b/include/net/ip.h
> index 6f602df72ee62..ffa4ba0b571fc 100644
> --- a/include/net/ip.h
> +++ b/include/net/ip.h
> @@ -584,6 +584,13 @@ static inline void ip_select_ident_segs(struct net *net, struct sk_buff *skb,
>  {
>  	struct iphdr *iph = ip_hdr(skb);
>  
> +	/* RFC 6864: the IPv4 ID of atomic datagrams has no meaning.
> +	 * DF packets without ignore_df can not be fragmented.
> +	 */
> +	if ((iph->frag_off & htons(IP_DF)) && !skb->ignore_df) {
> +		iph->id = 0;
> +		return;
> +	}

[Severity: Medium]
Does this also change the IDs of TCP TSO/GSO skbs, not only single
atomic datagrams?

__ip_queue_xmit() still passes the segment count:

	ip_select_ident_segs(net, skb, sk,
			     skb_shinfo(skb)->gso_segs ?: 1);

The new early return ignores segs. The template ID is now 0, and inet_id
is no longer advanced by segs.

TCP never sets SKB_GSO_TCP_FIXEDID, so inet_gso_segment() in
net/ipv4/af_inet.c takes the !fixedid path:

		} else {
			if (!fixedid)
				iph->id = htons(id++);

TSO hardware behaves the same way. From
Documentation/networking/segmentation-offloads.rst:

  "The default behavior is to increment the IP ID with every segment."

So each TSO skb goes out with IDs 0, 1, ..., n-1, and the next skb of
the flow starts again at 0. Before this patch, the IDs continued across
skbs.

On a Linux receiver, tcp_gro_receive()->gro_receive_network_flush()->
inet_gro_flush() checks ID continuity:

	if (count == 1 && !ipid_offset)
		NAPI_GRO_CB(p)->ip_fixedid |= 1 << inner;

	return ipid_offset ^ (count * !(NAPI_GRO_CB(p)->ip_fixedid & (1 << inner)));

At each sender skb boundary, the arriving segment has ID 0, the same as
the held head. So ipid_offset is 0 while count is not, and the flow gets
flushed.

Does this mean receiver GRO can no longer merge across sender TSO skbs?

It can get worse when a single-MSS skb (ID 0) is followed by a TSO skb
(IDs 0, 1, ...). On the second frame, count == 1 and ipid_offset == 0,
so the chain switches to fixed-ID mode. The next incrementing ID then
flushes it again.

How much this hurts probably depends on how the receiver's gro_max_size
compares with the sender's TSO skb size. BIG TCP receivers, small
cwnd-limited TSO bursts, and mixes of single-MSS and TSO skbs look the
most exposed. Connected UDP GSO already lost ID continuity before this
patch, because __ip_make_skb() uses ip_select_ident() with segs=1. The
new behaviour change is on the TCP path.

The commit message says DF packets on connected sockets get their IPID
set to zero, and the new comment says the ID "has no meaning". Neither
holds for multi-segment skbs. The "Minor side effects" paragraph also
does not mention the GRO impact.

Was this trade-off intended, and should the changelog mention it?

[Severity: Low]
This isn't a bug, but the kernel-doc for struct inet_sock in
include/net/inet_sock.h still says:

 * @inet_id - ID counter for DF pkts

After this change, inet_id is used only for non-DF packets and for DF
packets with skb->ignore_df set, which is the reverse of what the
comment says.

Could that comment be updated as well?

tcp_v4_connect() and tcp_v4_syn_recv_sock() still seed inet_id with
get_random_u16(). Non-DF TCP (for example IP_PMTUDISC_DONT) still needs
that, so only the comment is out of date.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929153834.566551-1-edumazet%40kernel.org