Re: [PATCH] bpf, sockmap: Fix self-redirect copied_seq double-counting

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 2 · Medium: 0 · Low: 0

- [High] net/core/skmsg.c:1042-1047 (sk_psock_verdict_apply(),
  __SK_REDIRECT arm): the new self-redirect test compares only the…
- [High] net/core/skmsg.c:1042-1047 (sk_psock_verdict_apply()): for a
  self-redirect the patch drops the up-front tcp_eat_skb() and relies on…

--

bpf, sockmap: Fix self-redirect copied_seq double-counting

This changes the __SK_REDIRECT arm of sk_psock_verdict_apply() so that
tcp_eat_skb() is only called when the redirect destination differs from
psock->sk.  The intent is to avoid advancing tcp_sk->copied_seq twice when
a stream_verdict program redirects an skb back into the same socket's
ingress queue.

[Severity: High]
The commit message describes the problem as "self-redirect with
BPF_F_INGRESS", but this test only compares the destination socket and
never looks at the direction.  Does the same-socket *egress* case end up
here too?

skb_bpf_redirect_fetch() in include/linux/skmsg.h masks the flag bits off:

  static inline struct sock *skb_bpf_redirect_fetch(struct sk_buff *skb)
  {
	unsigned long sk_redir = skb->_sk_redir;

	return (struct sock *)(sk_redir & BPF_F_PTR_MASK);
  }

and bpf_sk_redirect_map() in net/core/sock_map.c stores the direction
separately, allowing flags == 0:

	if (unlikely(flags & ~(BPF_F_INGRESS)))
		return SK_DROP;
	...
	skb_bpf_set_redir(skb, sk, flags & BPF_F_INGRESS);

So a program doing bpf_sk_redirect_map(skb, &sock_map, idx, 0) where
sock_map[idx] resolves to the source socket satisfies dst == psock->sk and
skips tcp_eat_skb().

For that egress case nothing later advances copied_seq.  sk_psock_backlog()
computes:

		ingress = skb_bpf_ingress(skb);

which is false, so sk_psock_handle_skb() takes the transmit path:

		return skb_send_sock(psock->sk, skb, off, len);

and the skb is freed.  It never becomes an sk_msg with msg->sk == sk, so
the from_self accounting in __sk_msg_recvmsg():

		from_self = msg_rx->sk == sk;

never runs, and neither does the copied_from_self path in
tcp_bpf_recvmsg_parser() that the commit message relies on.

Meanwhile tcp_read_skb() already removed the bytes from the receive queue
without touching tp->copied_seq:

		__skb_unlink(skb, &sk->sk_receive_queue);

Can this leave tp->copied_seq permanently behind tp->rcv_nxt, so that
SIOCINQ reports data that no longer exists, __tcp_cleanup_rbuf() and
tcp_rcv_space_adjust() never run for those bytes, tcp_close() sees
data_was_unread != 0 and sends an RST instead of a FIN, and a later plain
tcp_recvmsg_locked() (after the psock is dropped) trips

	WARN(before(*seq, TCP_SKB_CB(skb)->seq), "TCP recvmsg seq # bug...

that is, the same copied_seq desync in the opposite direction?

Worth noting that the program cited as validation,
tools/testing/selftests/bpf/progs/mptcp_sockmap.c, uses exactly the egress
form:

	return bpf_sk_redirect_map(skb, &sock_map, redirect_idx, 0);

Would adding the direction to the condition cover this, e.g.

	if (dst != psock->sk || !skb_bpf_ingress(skb))
		tcp_eat_skb(psock->sk, skb);

[Severity: High]
A second question about the same condition: the accounting is now skipped
before the outcome of the redirect is known.  Is it guaranteed that the skb
really reaches ingress_msg and is read by tcp_bpf_recvmsg_parser()?

sk_psock_skb_redirect() has drop paths that free the skb and return -EIO
without any copied_seq update, and for a self-redirect psock_other is this
very same psock:

  net/core/skmsg.c:sk_psock_skb_redirect() {
	...
	if (!psock_other || sock_flag(sk_other, SOCK_DEAD)) {
		skb_bpf_redirect_clear(skb);
		sock_drop(from->sk, skb);
		return -EIO;
	}
	spin_lock_bh(&psock_other->ingress_lock);
	if (!sk_psock_test_state(psock_other, SK_PSOCK_TX_ENABLED)) {
		spin_unlock_bh(&psock_other->ingress_lock);
		skb_bpf_redirect_clear(skb);
		sock_drop(from->sk, skb);
		return -EIO;
	}
	...
  }

SK_PSOCK_TX_ENABLED can already be clear while sk->sk_data_ready is still
sk_psock_verdict_data_ready, because sk_psock_backlog() clears it on a hard
transmit/ingress error:

			if (ret == -EAGAIN) {
				...
			} else {
				sk_psock_clear_state(psock, SK_PSOCK_TX_ENABLED);
				...
			}

and sk_psock_stop() clears it during close/map-delete before the socket
backlog is drained.  The __SK_PASS arm just above tests the same two states
(SOCK_DEAD and !SK_PSOCK_TX_ENABLED), so they appear reachable at this
point.

Even after a successful skb_queue_tail(), skbs still on ingress_skb are
purged without accounting:

  net/core/skmsg.c:__sk_psock_zap_ingress() {
	while ((skb = skb_dequeue(&psock->ingress_skb)) != NULL) {
		skb_bpf_redirect_clear(skb);
		sock_drop(psock->sk, skb);
	}
	...
  }

and __sk_psock_purge_ingress_msg() frees queued sk_msgs the same way.

In all of these cases tcp_read_skb() has already unlinked the bytes from
sk->sk_receive_queue, and tcp_eat_skb() is the only place that catches up:

  net/ipv4/tcp_bpf.c:tcp_eat_skb() {
	copied = tcp->copied_seq + skb->len;
	WRITE_ONCE(tcp->copied_seq, copied);
	tcp_rcv_space_adjust(sk);
	__tcp_cleanup_rbuf(sk, skb->len);
  }

Note the sibling drop arm in this same switch keeps that contract:

  out_free:
		skb_bpf_redirect_clear(skb);
		tcp_eat_skb(psock->sk, skb);
		sock_drop(psock->sk, skb);

Should the skipped accounting be performed on the redirect-failure and
purge paths as well, or should tcp_eat_skb() only be skipped once the skb
is known to have been retained for same-socket ingress consumption?

 		err = sk_psock_skb_redirect(psock, skb);
 		break;
+	}
 	case __SK_DROP:
 	default:
 out_free:

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/b840c35fdfdf36e9fddedfa645b12699bc51aa34.1787968065.git.tanggeliang%40kylinos.cn