Re: [PATCH net v2 1/2] tcp: exclude old ACKs from tcp fast path

[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: 0 · Low: 2

- [Low] The patch fixes a live, security-relevant input-validation defect
  (three Reported-by tags, RFC5961 reference) but carries no Fixes: tag,…
- [Low] The kerneldoc-style block comment above tcp_rcv_established()
  (net/ipv4/tcp_input.c:6477-6499) enumerates the conditions that…

--

tcp: exclude old ACKs from tcp fast path

This changes the header prediction test in tcp_rcv_established() so that
segments whose ack_seq is below SND.UNA no longer enter the fast path, in
addition to the existing rejection of ack_seq above SND.NXT.  Such segments
now fall through to the slow path where tcp_ack() applies the RFC5961
validation and challenge ACK handling.

No Link: tags are present in the commit header.

Two notes below.

[Severity: Low]
The changelog refers to the validation added by commit 3d501dd326fb1c7
("tcp: do not accept ACK of bytes we never sent") in prose:

    Such ACKs will fall through to the slow path, where tcp_ack()
    performs the appropriate validation and challenge ACK handling
    according to RFC5961 and Commit 3d501dd326fb1c7 ("tcp: do not
    accept ACK of bytes we never sent").

Should this carry a Fixes: tag as well, so stable tooling can pick it up?

git blame on the replaced predicate in tcp_rcv_established() points at
31770e34e43d6 ("tcp: Revert \"tcp: remove header prediction\""), and
3d501dd326fb is the commit that added the too-old-ACK check the fast path
was bypassing, so either looks like a concrete candidate.

There are three Reported-by tags but no Link: to the report, and the
changelog does not state the user-visible impact.  Could both be added?

> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 0f60a1dbf927..89f9f3913b52 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -6539,7 +6539,7 @@ void tcp_rcv_established(struct sock *sk, struct sk_buff *skb)
>  
>  	if ((tcp_flag_word(th) & TCP_HP_BITS) == tp->pred_flags &&
>  	    TCP_SKB_CB(skb)->seq == tp->rcv_nxt &&
> -	    !after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt)) {
> +	    between(TCP_SKB_CB(skb)->ack_seq, tp->snd_una, tp->snd_nxt)) {

[Severity: Low]
Should the block comment above tcp_rcv_established() be updated for this
new rejection class?  It enumerates the cases that disable the fast path
and then claims the enumeration is complete:

 *	- Unexpected TCP flags/window values/header lengths are received
 *	  (detected by checking the TCP header against pred_flags)
 *	- Data is sent in both directions. Fast path only supports pure senders
 *	  or pure receivers (this means either the sequence number or the ack
 *	  value must stay constant)
 *	- Unexpected TCP option.
 *
 *	When these conditions are not satisfied it drops into a standard
 *	receive procedure patterned after RFC793 to handle all cases.
 *	The first three cases are guaranteed by proper pred_flags setting,
 *	the rest is checked inline. Fast processing is turned on in
 *	tcp_data_queue when everything is OK.

Nothing in that list mentions ACK sequence validity.  After this change, a
reordered or duplicated pure ACK with ack_seq strictly below SND.UNA takes
the slow path on a plain pure-sender/pure-receiver connection with matching
pred_flags, which the list above says would be fast-pathed.  A bullet for
ack_seq outside [SND.UNA, SND.NXT] would keep the comment in agreement with
the predicate.

>  		int tcp_header_len = tp->tcp_header_len;
>  		s32 delta = 0;
>  		int flag = 0;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909075644.1408171-1-inbal.lipshtat%40mail.huji.ac.il