Re: [PATCH] net: tcp: fix fraglist GRO flushing on alternating TCP PSH flag

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

netdev-bot+sashiko@ wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 2 · Medium: 1 · 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).
> 
> - [High] Forwarded TCP FIN/PSH can be dropped from the wire when a
>   fraglist GRO skb falls back to tcp_gso_segment()/skb_segment().
> - [High] FIN/PSH in a fraglist tail segment is invisible to netfilter,
>   conntrack, flowtable, tc and BPF, which all read only the aggregate's…
> - [Medium] The commit message calls the removed check 'redundant' and
>   only discusses PSH.
> 
> --
> 
> net: tcp: fix fraglist GRO flushing on alternating TCP PSH flag
> 
> This patch removes the unmasked TCP flag word comparison from the is_flist
> branch of tcp_gro_receive(). Segments whose PSH/FIN flags differ from the
> held packet can then be appended to a fraglist GRO skb. The goal is to stop
> premature flushes when a sender alternates [ACK] and [PSH, ACK] segments
> in forwarding and tethering setups.
> 
> > This redundant check causes premature flushing on any incoming PSH packet,
> > shattering aggregation and increasing softirq overhead.
> 
> [ ... ]
> 
> > a single fraglist, and are safely flushed together upon encountering the
> > PSH flag at `out_check_final`, preserving the required TCP push semantics.
> 
> [Severity: Medium]
> Is the removed check really redundant? Commit 8d95dc474f85 ("net: add code
> for TCP fraglist GRO") added it to the is_flist branch on purpose.
>
> The general path can mask FIN|PSH out of its first comparison only because
> it later ORs those flags into the held packet's header:
> 
>     tcp_flag_word(th2) |= flags & (TCP_FLAG_FIN | TCP_FLAG_PSH);

Indeed. PSH bits are important information and are expected to close
a context.
 
> The is_flist branch jumps to out_check_final before it reaches that line.
> So the unmasked comparison was what kept every fraglist member's flags
> identical to the head's.
> 
> The removal also relaxes FIN, not only PSH. Data-carrying FIN segments,
> such as the last segment of a response, can now join a fraglist. The
> commit message doesn't mention this.
> 
> The claim that push semantics are preserved seems to hold only when the
> skb goes through skb_segment_list(). Could the commit message describe the
> FIN change and the cases raised in the comments on the diff below?
> 
> > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> > index e74d99ca9face..54b40ec95a080 100644
> > --- a/net/ipv4/tcp_offload.c
> > +++ b/net/ipv4/tcp_offload.c
> > @@ -328,7 +328,6 @@ struct sk_buff *tcp_gro_receive(struct list_head *head, struct sk_buff *skb,
> >  	flush |= skb_cmp_decrypted(p, skb);
> >  
> >  	if (unlikely(NAPI_GRO_CB(p)->is_flist)) {
> > -		flush |= (__force int)(flags ^ tcp_flag_word(th2));
> 
> [Severity: High]
> Can this drop a forwarded FIN or PSH from the wire?
> 
> Without this check, skb_gro_receive_list() links an in-sequence [PSH, ACK]
> or [FIN, ACK] data segment into the held skb's frag_list. The is_flist
> branch then jumps to out_check_final. That skips the only place where
> FIN/PSH reach the head header:
> 
>     tcp_flag_word(th2) |= flags & (TCP_FLAG_FIN | TCP_FLAG_PSH);
> 
> tcp4_gro_complete() and tcp6_gro_complete() return early for is_flist.
> The head can't carry FIN/PSH itself, because out_check_final would have
> flushed it. So the head header has neither flag, while the last frag_list
> member has them.
> 
> tcp4_gso_segment() uses __tcp4_gso_segment_list() only when the geometry
> still matches and SKB_GSO_DODGY is clear:
> 
>     if ((skb_pagelen(skb) - th->doff * 4 == skb_shinfo(skb)->gso_size) &&
>         !(skb_shinfo(skb)->gso_type & SKB_GSO_DODGY))
>         return __tcp4_gso_segment_list(skb, features);
> 
> Otherwise it falls back to tcp_gso_segment()->skb_segment(). That path
> copies head_skb's headers, TCP flags included, into every segment.
> 
> tcp_gso_segment() then clears fin/psh on every segment except the last:
> 
>     while (skb->next) {
>         th->fin = th->psh = 0;
> 
> The last segment keeps the head's flags, which never had FIN/PSH.
> tcp6_gso_segment() has the same fallback.
> 
> These can trigger the fallback on forwarded traffic:
> 
>   bpf_skb_proto_4_to_6() and bpf_skb_proto_6_to_4(), reached through
>   bpf_skb_change_proto() (for example CLAT/464xlat when tethering), set
>   SKB_GSO_DODGY on GSO skbs.
> 
>   bpf_skb_net_grow() and bpf_skb_net_shrink() set SKB_GSO_DODGY.
>   bpf_skb_net_grow() can also linearize frag_list skbs when
>   BPF_F_ADJ_ROOM_FIXED_GSO is not set.
> 
>   Any pull or linearize that changes the geometry has the same effect,
>   for example skb_ensure_writable(skb, skb->len) in NAT mangling.
> 
> In those cases the receiver doesn't get the FIN until the sender
> retransmits it, and PSH is silently cleared.
> 
> A fraglist skb redirected to a local TCP socket after GRO (DNAT/TPROXY) is
> also parsed from the head header only. TCP has nothing like UDP's
> local-delivery fraglist handling, so the FIN would be missed there too.
> 
> ORing the tail's flags into th2 in the is_flist branch doesn't seem to fix
> this. skb_segment_list() emits the head skb with its own TCP header, so the
> first segment would then carry a spurious FIN/PSH.
> 
> [Severity: High]
> Does this also hide the tail's FIN/PSH from netfilter, conntrack,
> flowtable, tc and BPF? All of these read only the aggregate's head header
> before segmentation.
> 
> In conntrack, get_conntrack_index() only checks the head's tcph->fin, so
> the FIN transition never fires. tcp_in_window() computes:
> 
>     end = segment_seq_plus_len(seq, skb->len, dataoff, tcph);
> 
> skb->len covers all of the fraglist payload. The +1 for FIN is missing,
> because tcph is the head header. The peer's ACK of the FIN then hits:
> 
>     if (!before(sack, receiver->td_end + 1))
>         return nf_tcp_store_invalid(ct, sender, log, NFCT_TCP_INVALID,
>                                     NF_TCP_LOG_ACK_OVER, receiver->td_end + 1);
> 
> That ACK is classified invalid, and so are the peer's own FIN and the
> final ACK. A typical "ct state invalid drop" rule would then drop the
> teardown packets. Without such a rule, the entry stays ESTABLISHED instead
> of moving to FIN_WAIT/CLOSE_WAIT.
> 
> In the flowtable, nf_flow_state_check() reads fin/rst only from the head
> header, so NF_FLOW_CLOSING is never set for the offloaded flow.
> 
> tcp_mt() in xt_tcpudp and the nft tcp flags match also read only the head
> header, so a FORWARD rule that matches FIN or PSH won't fire.
> skb_segment_list() copies only the MAC header from the head and keeps each
> member's own TCP header. The FIN/PSH therefore still goes out on the wire
> and bypasses that policy.
> 
> Before this patch, the unmasked comparison guaranteed that the head header
> described every member of the fraglist.
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009060319.6229-1-shiming.cheng%40mediatek.com