[PATCH bpf 0/2] bpf, sockmap: fix copied_seq left behind when a stream parser is removed

[PATCH bpf 0/2] bpf, sockmap: fix copied_seq left behind when a stream parser is removed

[Sechang Lim]

A stream-parser sockmap socket can be removed with SK_PASS data still
parked on its ingress_msg queue. The copied_seq rollback for that data is
never repaid, leaving copied_seq behind sk_receive_queue, so the native
tcp_recvmsg() warns. Patch 1 settles copied_seq on removal. Patch 2 adds a
selftest.

Sechang Lim (2):
  bpf, sockmap: settle copied_seq when a stream parser is removed
  selftests/bpf: test sockmap strparser recover with undelivered ingress

 net/ipv4/tcp_bpf.c                            |  9 +++
 .../selftests/bpf/prog_tests/sockmap_basic.c  | 59 +++++++++++++++++++
 2 files changed, 68 insertions(+)

-- 
2.43.0

[PATCH bpf 1/2] bpf, sockmap: settle copied_seq when a stream parser is removed

[Sechang Lim]

tcp_bpf_strp_read_sock() rolls tp->copied_seq back by the SK_PASS bytes
parked on the psock ingress_msg queue; tcp_bpf_recvmsg_parser() repays it
as those bytes are delivered. When the socket leaves the sockmap they are
purged undelivered and nothing repays the rollback, so copied_seq is left
behind sk_receive_queue and the native tcp_recvmsg() warns:

  TCP recvmsg seq # bug: copied 66913561, seq 6691356A, rcvnxt 66913572, fl 40
  WARNING: net/ipv4/tcp.c:2733 at tcp_recvmsg_locked+0x2d0/0x1270
   tcp_recvmsg+0xba/0x340
   inet_recvmsg+0x7a/0x370
   sock_recvmsg+0xef/0x110
   __sys_recvfrom+0x132/0x1e0

Settle copied_seq to the parser's consume point as the socket leaves the
sockmap so it cannot trail the receive queue.

Fixes: 36b62df5683c ("bpf: Fix wrong copied_seq calculation")
Signed-off-by: Sechang Lim <rhkrqnwk98@gmail.com>
---
 net/ipv4/tcp_bpf.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index cc0bd73f36b6..918f8da02c39 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -715,6 +715,15 @@ int tcp_bpf_update_proto(struct sock *sk, struct sk_psock *psock, bool restore)
 	}
 
 	if (restore) {
+#if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
+		/*
+		 * Settle the copied_seq rollback for the now-discarded
+		 * ingress_msg data so it cannot trail the receive queue
+		 */
+		if (sk_psock_test_state(psock, SK_PSOCK_RX_STRP_ENABLED) &&
+		    before(tcp_sk(sk)->copied_seq, psock->copied_seq))
+			WRITE_ONCE(tcp_sk(sk)->copied_seq, psock->copied_seq);
+#endif
 		if (inet_csk_has_ulp(sk)) {
 			/* TLS does not have an unhash proto in SW cases,
 			 * but we need to ensure we stop using the sock_map
-- 
2.43.0

[PATCH bpf 2/2] selftests/bpf: test sockmap strparser recover with undelivered ingress

[Sechang Lim]

Park SK_PASS data on a stream-parser socket's ingress_msg queue, drop the
socket from the sockmap without reading it, then check the native stack
still delivers data queued afterwards. Without the fix copied_seq is left
behind sk_receive_queue and tcp_recvmsg_locked() warns instead of
delivering.

Signed-off-by: Sechang Lim <rhkrqnwk98@gmail.com>
---
 .../selftests/bpf/prog_tests/sockmap_basic.c  | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index cb3229711f93..86b584f5491e 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -1292,6 +1292,63 @@ static int wait_for_fionread(int fd, int expected, unsigned int timeout_ms)
 	return avail;
 }
 
+static void test_sockmap_strp_recover_undelivered(void)
+{
+	struct test_sockmap_pass_prog *skel = NULL;
+	int c0 = -1, p0 = -1, c1 = -1, p1 = -1;
+	char buf[10] = "0123456789", rcv[11];
+	int err, map, verdict, parser, sent, recvd, avail, zero = 0;
+
+	skel = test_sockmap_pass_prog__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "open_and_load"))
+		return;
+
+	if (create_socket_pairs(AF_INET, SOCK_STREAM, &c0, &c1, &p0, &p1))
+		goto out;
+
+	map = bpf_map__fd(skel->maps.sock_map_rx);
+
+	verdict = bpf_program__fd(skel->progs.prog_skb_verdict);
+	err = bpf_prog_attach(verdict, map, BPF_SK_SKB_STREAM_VERDICT, 0);
+	if (!ASSERT_OK(err, "bpf_prog_attach"))
+		goto out;
+
+	parser = bpf_program__fd(skel->progs.prog_skb_verdict_ingress_strp);
+	err = bpf_prog_attach(parser, map, BPF_SK_SKB_STREAM_PARSER, 0);
+	if (!ASSERT_OK(err, "bpf_prog_attach"))
+		goto out;
+
+	err = bpf_map_update_elem(map, &zero, &p1, BPF_ANY);
+	if (!ASSERT_OK(err, "bpf_map_update(p1)"))
+		goto out;
+
+	sent = xsend(c1, buf, sizeof(buf), 0);
+	if (!ASSERT_EQ(sent, sizeof(buf), "xsend(c1) bpf"))
+		goto out;
+
+	avail = wait_for_fionread(p1, sizeof(buf), 1000);
+	if (!ASSERT_EQ(avail, sizeof(buf), "fionread"))
+		goto out;
+
+	err = bpf_map_delete_elem(map, &zero);
+	if (!ASSERT_OK(err, "map_delete(p1)"))
+		goto out;
+
+	sent = xsend(c1, buf, sizeof(buf), 0);
+	if (!ASSERT_EQ(sent, sizeof(buf), "xsend(c1) native"))
+		goto out;
+	recvd = recv_timeout(p1, rcv, sizeof(buf), MSG_DONTWAIT, 1);
+	ASSERT_EQ(recvd, sent, "recv(p1) native after drop");
+
+out:
+	close(c0);
+	close(p0);
+	close(c1);
+	close(p1);
+
+	test_sockmap_pass_prog__destroy(skel);
+}
+
 /* it is used to send data to via native stack and BPF redirecting */
 static void test_sockmap_multi_channels(int sotype)
 {
@@ -1447,6 +1504,8 @@ void test_sockmap_basic(void)
 		test_sockmap_copied_seq(false);
 	if (test__start_subtest("sockmap recover with strp"))
 		test_sockmap_copied_seq(true);
+	if (test__start_subtest("sockmap strp recover undelivered"))
+		test_sockmap_strp_recover_undelivered();
 	if (test__start_subtest("sockmap tcp multi channels"))
 		test_sockmap_multi_channels(SOCK_STREAM);
 	if (test__start_subtest("sockmap udp multi channels"))
-- 
2.43.0