Re: [PATCH bpf v2 09/15] bpf, sockmap: Check the TCP state in recvmsg()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Concern (commit message accuracy): the concern says the commit
  message wrongly claims icsk_ack.rcv_mss "can be 0 in TCP_CLOSE",…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but psock data
  from an old connection still advances tp->copied_seq of the next…

--

bpf, sockmap: Check the TCP state in recvmsg()

This commit makes tcp_bpf_recvmsg_parser() call __tcp_cleanup_rbuf() only
when tcp_can_send_ack() returns true. The reason is that psock data can
outlive the connection it arrived on, for example after tcp_disconnect().
tcp_can_send_ack() moves from net/mptcp/protocol.c to include/net/tcp.h so
both users can share it.

> __tcp_select_window() divides by icsk_ack.rcv_mss, which can be 0 in
> TCP_CLOSE.

[Severity: Low]
This isn't a bug, and the statement looks accurate. Still, a reader may
wonder how rcv_mss gets to 0 in TCP_CLOSE, because tcp_disconnect()
explicitly guards against it:

net/ipv4/tcp.c:tcp_disconnect() {
    ...
	inet_csk_delack_init(sk);
	/* Initialize rcv_mss to TCP_MIN_MSS to avoid division by 0
	 * issue in __tcp_select_window()
	 */
	icsk->icsk_ack.rcv_mss = TCP_MIN_MSS;
    ...
}

It seems to happen when a later listen() on the disconnected socket fails.
inet_csk_listen_start() calls inet_csk_delack_init(), which zeroes rcv_mss.
If get_port() or hash() then fails, the socket returns to TCP_CLOSE with
rcv_mss still 0:

net/ipv4/inet_connection_sock.c:inet_csk_listen_start() {
    ...
	inet_csk_delack_init(sk);
    ...
	err = sk->sk_prot->get_port(sk, inet->inet_num);
    ...
	inet_sk_set_state(sk, TCP_CLOSE);
	return err;
}

Would it help to mention this path, and that TCP_LISTEN is covered as
well, in the commit message?

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index ef2e15de0fe3b..3d703f1517d88 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -343,7 +343,8 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>  	if (!peek)
>  		WRITE_ONCE(tcp->copied_seq, seq);
>  	tcp_rcv_space_adjust(sk);
> -	if (copied > 0)
> +	/* Data on the psock outlives the connection that it came in on */
> +	if (copied > 0 && tcp_can_send_ack(sk))
>  		__tcp_cleanup_rbuf(sk, copied);

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but the
new comment points at it. If psock data outlives the connection, do the
copied_seq and tcp_rcv_space_adjust() updates just above the new check
still charge bytes from the old connection to the new one?

SK_PASS data is tagged as self-originated in sk_psock_skb_ingress_self():

	msg->sk = sk;

tcp_read_skb() does not advance copied_seq for these skbs. Instead,
tcp_bpf_recvmsg_parser() adds them later:

	seq += copied_from_self;
    ...
	if (!peek)
		WRITE_ONCE(tcp->copied_seq, seq);

tcp_disconnect() purges sk_receive_queue and sets copied_seq to rcv_nxt.
It leaves psock->ingress_msg alone, and the socket stays in the sockmap.
A later connect() resets copied_seq to the new rcv_nxt.

Here is a possible sequence:

  socket in a sockmap with an sk_skb stream_verdict prog returning SK_PASS
  data is received but not read (D bytes in psock->ingress_msg)
  connect(AF_UNSPEC) -> tcp_disconnect()
  connect() to a new peer, reaches ESTABLISHED
  recvmsg() -> tcp_bpf_recvmsg_parser() reads the D stale bytes

After this, copied_seq is new rcv_nxt + D and stays D bytes ahead for the
rest of the connection.

tcp_rcv_space_adjust() then computes inq = rcv_nxt - copied_seq = -D. That
inflates copied on every measurement and grows sk_rcvbuf and window_clamp
through tcp_rcvbuf_grow() for no reason.

If the socket later leaves the sockmap and falls back to
tcp_recvmsg_locked(), offset = *seq - TCP_SKB_CB(skb)->seq starts at D.
Wouldn't that silently skip the first D bytes of the new connection's
data? Or, when D is not less than skb->len, wouldn't it trigger the
"TCP recvmsg seq # bug 2" WARN in tcp_recvmsg_locked() or the
"cleanup rbuf bug" WARN in tcp_cleanup_rbuf()?

No later patch in the series seems to change how copied_seq is updated
here.

Should data that arrived on an earlier connection be kept out of the
copied_seq update too, and not only out of __tcp_cleanup_rbuf()?

>  
>  unlock:

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102072112.4160195-1-alexei.starovoitov%40gmail.com