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
