* Re: [PATCH] tcp: drain out_of_order_queue and update SACKs in tcp_send_rcvq()
2026-09-19 21:52 [PATCH] tcp: drain out_of_order_queue and update SACKs in tcp_send_rcvq() Hui Peng
@ 2026-09-20 21:55 ` netdev-bot+sashiko
2026-09-21 12:53 ` Eric Dumazet
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sashiko @ 2026-09-20 21:55 UTC (permalink / raw)
To: benquike
Cc: edumazet, ncardwell, kuniyu, davem, kuba, pabeni, horms, netdev,
linux-kernel
Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 0 · Low: 4
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] net/ipv4/tcp_input.c, tcp_send_rcvq(): the newly added
unconditional `tcp_ofo_queue(sk)` call imports full receive-path…
- [Low] net/ipv4/tcp_input.c, tcp_send_rcvq() becomes the second caller
of the OFO-drain sequence but replicates only two of the five steps…
- [Low] Commit message factual error: it states that tcp_send_rcvq()
"advances tp->copied_seq and tp->rcv_nxt".
- [Low] The change asserts a bug without any reproducer, warning,
syzkaller report or stated user-visible symptom, yet carries `Fixes:…
- [Low] net/ipv4/tcp_input.c, tcp_send_rcvq(): the new tcp_ofo_queue()
call does not establish the per-processing-pass D-SACK precondition…
--
tcp: drain out_of_order_queue and update SACKs in tcp_send_rcvq()
This adds an out-of-order queue drain plus a SACK block update to
tcp_send_rcvq(), so that segments in tp->out_of_order_queue which become
in-order after TCP_REPAIR injects data into the receive queue are
coalesced and the SACK bookkeeping is pruned.
A few questions about the commit message and the new call site follow.
> When TCP_REPAIR injects data into the receive queue via tcp_send_rcvq() and
> advances tp->copied_seq and tp->rcv_nxt, any segments already sitting in
^^^^^^^^^^^^^^
[Severity: Low]
Does tcp_send_rcvq() actually advance tp->copied_seq? Reading the
function, the only sequence update comes from tcp_queue_rcv():
net/ipv4/tcp_input.c:tcp_queue_rcv() {
...
tcp_rcv_nxt_update(tcp_sk(sk), TCP_SKB_CB(skb)->end_seq);
...
}
which touches rcv_nxt only. For the receive repair queue, copied_seq
appears to be written only by the TCP_QUEUE_SEQ setsockopt:
net/ipv4/tcp.c:do_tcp_setsockopt() {
...
} else if (tp->repair_queue == TCP_RECV_QUEUE) {
if (tp->rcv_nxt != tp->copied_seq) {
err = -EPERM;
} else {
WRITE_ONCE(tp->rcv_nxt, val);
WRITE_ONCE(tp->copied_seq, val);
}
...
}
and that path additionally requires sk->sk_state == TCP_CLOSE.
Could the changelog also state the user-visible symptom and the
preconditions to reach it (CAP_NET_ADMIN, TCP_REPAIR enabled on a live
socket whose out_of_order_queue is already non-empty)?
> tp->out_of_order_queue that are now covered or contiguous with rcv_nxt are
> not coalesced or removed from tp->rx_opt.num_sacks. Drain
^^^^^^^^^^^^^^^^^^^^
> out_of_order_queue via tcp_ofo_queue(sk) and update SACK blocks via
> tcp_sack_remove(tp) after advancing rcv_nxt in tcp_send_rcvq().
>
> Fixes: 292e8d8c8538 ("tcp: Move rcvq sending to tcp_input.c")
[Severity: Low]
Small wording point first: segments are not "removed from
tp->rx_opt.num_sacks", num_sacks is a count of SACK blocks.
On the Fixes: tag, is there a reproducer, warning or syzkaller report
behind this? The tag targets a 2012 commit, so this pulls a behaviour
change of the privileged TCP_REPAIR path into stable trees.
The state being fixed also looks transient, since the normal receive path
already reconciles it on the next in-sequence segment:
net/ipv4/tcp_input.c:tcp_data_queue() {
...
if (!RB_EMPTY_ROOT(&tp->out_of_order_queue)) {
tcp_ofo_queue(sk);
...
if (tp->rx_opt.num_sacks)
tcp_sack_remove(tp);
...
}
and tcp_fin(), tcp_prune_ofo_queue(), tcp_close() and tcp_disconnect()
all purge the tree or reset the SACK state. In the CRIU restore flow the
out-of-order tree is empty anyway, because TCP_QUEUE_SEQ only allows
moving rcv_nxt while sk_state == TCP_CLOSE.
Would it make sense to either demonstrate the failure or drop the Fixes:
tag and present this as hardening?
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 92bc60716f33..8a7c38a4b1ec 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -5589,6 +5589,11 @@ int tcp_send_rcvq(struct sock *sk, struct msghdr *msg, size_t size)
> WARN_ON_ONCE(fragstolen); /* should not happen */
> __kfree_skb(skb);
> }
> + if (!RB_EMPTY_ROOT(&tcp_sk(sk)->out_of_order_queue)) {
> + tcp_ofo_queue(sk);
[Severity: High]
Can this run the TCP state machine from inside sendmsg()? If an
out-of-order segment carrying TCPHDR_FIN becomes in-order because the
injected bytes closed the hole, tcp_ofo_queue() reaches:
net/ipv4/tcp_input.c:tcp_ofo_queue() {
...
if (unlikely(fin)) {
tcp_fin(sk);
/* tcp_fin() purges tp->out_of_order_queue,
* so we must end this loop right now.
*/
break;
}
...
}
and tcp_fin() then does:
net/ipv4/tcp_input.c:tcp_fin() {
inet_csk_schedule_ack(sk);
WRITE_ONCE(sk->sk_shutdown, sk->sk_shutdown | RCV_SHUTDOWN);
sock_set_flag(sk, SOCK_DONE);
...
skb_rbtree_purge(&tp->out_of_order_queue);
if (tcp_is_sack(tp))
tcp_sack_reset(&tp->rx_opt);
...
}
The call site here is reached from tcp_sendmsg_locked():
net/ipv4/tcp.c:tcp_sendmsg_locked() {
if (unlikely(tp->repair)) {
if (tp->repair_queue == TCP_RECV_QUEUE) {
copied = tcp_send_rcvq(sk, msg, size);
goto out_nopush;
}
...
}
sk_stream_wait_connect() confines sk_state to ESTABLISHED/CLOSE_WAIT (or
SYN_RECV for passive fastopen), so the FIN_WAIT arms and tcp_done() are
not reachable from here. The ESTABLISHED -> CLOSE_WAIT transition,
RCV_SHUTDOWN, SOCK_DONE, the skb_rbtree_purge() of the remaining
out-of-order peer data, the SACK reset and the sk_state_change()/
sk_wake_async(POLL_IN) wakeups are reachable though, and
tcp_send_rcvq() still returns size, so a repair tool sees success with no
indication that the read side was shut down and queued peer data
discarded.
Separately, tcp_ofo_queue() computes dsack_high from tp->rcv_nxt as just
advanced by the locally injected bytes:
net/ipv4/tcp_input.c:tcp_ofo_queue() {
__u32 dsack_high = tp->rcv_nxt;
...
if (before(TCP_SKB_CB(skb)->seq, dsack_high)) {
__u32 dsack = dsack_high;
...
tcp_dsack_extend(sk, TCP_SKB_CB(skb)->seq, dsack);
}
...
}
Does that make the socket record and later advertise a D-SACK for a range
the peer never retransmitted, feeding the peer's spurious-retransmission
and congestion-undo logic?
Would it be better to reconcile the stale state locally here
(skb_rbtree_purge() plus tcp_sack_reset()), or to suppress the FIN and
D-SACK side effects while tp->repair is set, matching the quiescence
policy TCP_QUEUE_SEQ already enforces?
[Severity: Low]
On the same call, is tp->rx_opt.dsack guaranteed to be clear here? The
existing caller clears it before it can reach tcp_ofo_queue():
net/ipv4/tcp_input.c:tcp_data_queue() {
reason = SKB_DROP_REASON_NOT_SPECIFIED;
tp->rx_opt.dsack = 0;
...
}
Otherwise the flag is only cleared when the option is actually written
out in tcp_options_write(), or by tcp_disconnect(). With a deferred
delayed ACK the flag can still be set when the repair write runs, and
then:
net/ipv4/tcp_input.c:tcp_dsack_extend() {
if (!tp->rx_opt.dsack)
tcp_dsack_set(sk, seq, end_seq);
else
tcp_sack_extend(tp->duplicate_sack, seq, end_seq);
}
takes the else branch against the stale tp->duplicate_sack[0]. For a
disjoint range tcp_sack_extend() returns false and the return value is
ignored, so is the D-SACK for the segment processed in this pass silently
lost while the stale block stays?
> + if (tcp_sk(sk)->rx_opt.num_sacks)
> + tcp_sack_remove(tcp_sk(sk));
> + }
> return size;
[Severity: Low]
This new site performs two of the five steps the pre-existing caller
performs:
net/ipv4/tcp_input.c:tcp_data_queue() {
...
if (!RB_EMPTY_ROOT(&tp->out_of_order_queue)) {
tcp_ofo_queue(sk);
/* RFC5681. 4.2. SHOULD send immediate ACK, when
* gap in queue is filled.
*/
if (RB_EMPTY_ROOT(&tp->out_of_order_queue))
inet_csk(sk)->icsk_ack.pending |= ICSK_ACK_NOW;
}
if (tp->rx_opt.num_sacks)
tcp_sack_remove(tp);
tcp_fast_path_check(sk);
...
if (!sock_flag(sk, SOCK_DEAD))
tcp_data_ready(sk);
...
}
Is the omission of the ICSK_ACK_NOW hint, tcp_fast_path_check() and the
data-ready notification intentional? pred_flags stays zeroed by the
earlier tcp_data_queue_ofo() until some later in-order segment reaches
tcp_data_queue(), and bytes moved into sk_receive_queue by this drain
produce no wakeup.
If the divergence is deliberate, could the changelog say so?
>
> err_free:
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919215236.3470714-1-benquike%40gmail.com
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] tcp: drain out_of_order_queue and update SACKs in tcp_send_rcvq()
2026-09-19 21:52 [PATCH] tcp: drain out_of_order_queue and update SACKs in tcp_send_rcvq() Hui Peng
2026-09-20 21:55 ` netdev-bot+sashiko
@ 2026-09-21 12:53 ` Eric Dumazet
1 sibling, 0 replies; 3+ messages in thread
From: Eric Dumazet @ 2026-09-21 12:53 UTC (permalink / raw)
To: Hui Peng
Cc: ncardwell, kuniyu, davem, kuba, pabeni, horms, netdev, linux-kernel
On Sat, Sep 19, 2026 at 11:52 PM Hui Peng <benquike@gmail.com> wrote:
>
> When TCP_REPAIR injects data into the receive queue via tcp_send_rcvq() and
> advances tp->copied_seq and tp->rcv_nxt, any segments already sitting in
tcp_send_rcvq() does not advance tp->copied_seq. The only sequence
update comes from tcp_queue_rcv() -> tcp_rcv_nxt_update(), which touches
rcv_nxt. copied_seq of the receive queue is only written from
TCP_QUEUE_SEQ, which requires TCP_CLOSE state.
> tp->out_of_order_queue that are now covered or contiguous with rcv_nxt are
> not coalesced or removed from tp->rx_opt.num_sacks. Drain
num_sacks is a counter, nothing is "removed from" it.
> out_of_order_queue via tcp_ofo_queue(sk) and update SACK blocks via
> tcp_sack_remove(tp) after advancing rcv_nxt in tcp_send_rcvq().
>
> Fixes: 292e8d8c8538 ("tcp: Move rcvq sending to tcp_input.c")
This commit is a pure code move, it changed no behavior.
> Assisted-by: LLM
> Signed-off-by: Hui Peng <benquike@gmail.com>
What is the bug exactly ? What is the user visible symptom ?
No repro, no syzbot report, no packetdrill test.
For the CRIU flow the out-of-order queue is empty, because TCP_QUEUE_SEQ
only lets you move rcv_nxt while the socket is in TCP_CLOSE state.
You need CAP_NET_ADMIN, and you have to enable TCP_REPAIR on a live
ESTABLISHED socket that already has segments queued out of order, to
even get there. And the state you describe is transient: the next
in-order segment hits tcp_data_queue(), which already calls
tcp_ofo_queue() and tcp_sack_remove(). tcp_fin(), tcp_prune_ofo_queue(),
tcp_disconnect() and tcp_close() all purge the rb-tree as well.
So this is a "Fixes:" tag on a 2012 commit, sending a behavior change of
a privileged path to all stable trees, for something that is not
demonstrated to be a bug.
> ---
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 92bc60716f33..8a7c38a4b1ec 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -5589,6 +5589,11 @@ int tcp_send_rcvq(struct sock *sk, struct msghdr *msg, size_t size)
> WARN_ON_ONCE(fragstolen); /* should not happen */
> __kfree_skb(skb);
> }
> + if (!RB_EMPTY_ROOT(&tcp_sk(sk)->out_of_order_queue)) {
> + tcp_ofo_queue(sk);
And this is actively dangerous: you are running the input path from
sendmsg().
If one of the out-of-order skbs carries a FIN and the injected bytes
close the hole, tcp_ofo_queue() calls tcp_fin(), and we get
ESTABLISHED -> CLOSE_WAIT, RCV_SHUTDOWN, SOCK_DONE,
skb_rbtree_purge(&tp->out_of_order_queue), tcp_sack_reset(),
inet_csk_schedule_ack() and the sk_state_change()/sk_wake_async()
wakeups, from a sendmsg() that still returns size to the caller.
A repair tool would see a successful write and a shut down read side.
Also, tcp_ofo_queue() computes dsack_high from tp->rcv_nxt, which you
just advanced with locally injected bytes. So we would record and later
advertise DSACK for ranges the peer never retransmitted, and feed its
undo logic. tp->rx_opt.dsack is not cleared here either, the existing
caller clears it at the beginning of tcp_data_queue().
> + if (tcp_sk(sk)->rx_opt.num_sacks)
> + tcp_sack_remove(tcp_sk(sk));
> + }
> return size;
>
> err_free:
And you copied two of the five steps tcp_data_queue() performs:
no ICSK_ACK_NOW, no tcp_fast_path_check(), no tcp_data_ready(), so bytes
can be moved to sk_receive_queue with no wakeup.
If you really want to address this, do not call the input path. Peer
data sitting out of order is meaningless once the receive stream is
being rewritten, so purging the rb-tree and resetting the SACK state
would be the sane thing to do. But please come with a repro or a
packetdrill test first, showing an actual problem.
pw-bot: reject
^ permalink raw reply [flat|nested] 3+ messages in thread