bpf: fix wrong copied_seq calculation and add tests

PATCH Superseded Jiayuan Chen 3 participants 8 messages Jan 9, 2025 – Jan 14, 2025

Subsystems (8)

Networking Network drivers Documentation Kselftest +4 more subsystems

lore

Share & tools

lore

Jiayuan Chen

author

2025-01-09 09:46 UTC #0 lore raw

v5 cover bpf: fix wrong copied_seq calculation and add tests

A previous commit described in this topic
http://lore.kernel.org/bpf/[email protected]
directly updated 'sk->copied_seq' in the tcp_eat_skb() function when the action of a BPF program was SK_REDIRECT. For other actions, like SK_PASS, the update logic for 'sk->copied_seq' was moved to tcp_bpf_recvmsg_parser() to ensure the accuracy of the 'fionread' feature.

That commit works for a single stream_verdict scenario, as it also modified 'sk_data_ready->sk_psock_verdict_data_ready->tcp_read_skb' to remove updating 'sk->copied_seq'.

However, for programs where both stream_parser and stream_verdict are active(strparser purpose), tcp_read_sock() was used instead of tcp_read_skb() (sk_data_ready->strp_data_ready->tcp_read_sock) tcp_read_sock() now still update 'sk->copied_seq', leading to duplicated updates.

In summary, for strparser + SK_PASS, copied_seq is redundantly calculated in both tcp_read_sock() and tcp_bpf_recvmsg_parser().

The issue causes incorrect copied_seq calculations, which prevent correct data reads from the recv() interface in user-land.

Also we added test cases for bpf + strparser and separated them from sockmap_basic, as strparser has more encapsulation and parsing capabilities compared to sockmap.

Fixes e5c6de5fa025 ("bpf, sockmap: Incorrectly handling copied_seq")

---
V3 -> v5:
https://lore.kernel.org/bpf/[email protected]/
Avoid introducing new proto_ops. (Daniel Borkmann). Add more edge test cases for strparser + bpf. Fix patchwork fail of test cases code.
Fix psock fetch without rcu lock.

V1 -> V3:
https://lore.kernel.org/bpf/[email protected]/
Fix patchwork fail by adding Fixes tag.
Save skb data offset for ENOMEM. (John Fastabend)
---

Jiayuan Chen (3):
bpf: fix wrong copied_seq calculation
selftests/bpf: add strparser test for bpf
bpf, strparser, docs: Add new callback for bpf

 Documentation/networking/strparser.rst        |  11 +-
 include/linux/skmsg.h                         |   2 +
 include/net/strparser.h                       |   2 +
 include/net/tcp.h                             |   3 +
 net/core/skmsg.c                              |  51 ++
 net/ipv4/tcp.c                                |  31 +-
 net/strparser/strparser.c                     |  11 +-
 .../selftests/bpf/prog_tests/sockmap_basic.c  |  53 --
 .../selftests/bpf/prog_tests/sockmap_strp.c   | 452 ++++++++++++++++++
 .../selftests/bpf/progs/test_sockmap_strp.c   |  53 ++
 10 files changed, 608 insertions(+), 61 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/sockmap_strp.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_strp.c

Signature · 1 line

2.43.5

Jiayuan Chen

author

2025-01-09 09:46 UTC #1 lore raw

PATCH v5 1/3 bpf: fix wrong copied_seq calculation

'sk->copied_seq' was updated in the tcp_eat_skb() function when the action of a BPF program was SK_REDIRECT. For other actions, like SK_PASS, the update logic for 'sk->copied_seq' was moved to tcp_bpf_recvmsg_parser() to ensure the accuracy of the 'fionread' feature.

It works for a single stream_verdict scenario, as it also modified 'sk_data_ready->sk_psock_verdict_data_ready->tcp_read_skb' to remove updating 'sk->copied_seq'.

In summary, for strparser + SK_PASS, copied_seq is redundantly calculated in both tcp_read_sock() and tcp_bpf_recvmsg_parser().

The issue causes incorrect copied_seq calculations, which prevent correct data reads from the recv() interface in user-land.

We do not want to add new proto_ops to implement a new version of tcp_read_sock, as this would introduce code complexity [1].

[1]: https://lore.kernel.org/bpf/[email protected]

Fixes e5c6de5fa025 ("bpf, sockmap: Incorrectly handling copied_seq")

Co-developed-by Jakub Sitnicki <[email protected]>

Signed-off-by Jakub Sitnicki <[email protected]>

Signed-off-by Jiayuan Chen <[email protected]>

---
 include/linux/skmsg.h     |  2 ++
 include/net/strparser.h   |  2 ++
 include/net/tcp.h         |  3 +++
 net/core/skmsg.c          | 51 +++++++++++++++++++++++++++++++++++++++
 net/ipv4/tcp.c            | 31 ++++++++++++++++++++----
 net/strparser/strparser.c | 11 +++++++--
 6 files changed, 93 insertions(+), 7 deletions(-)

include/linux/skmsg.h +2 -0

index 2cbe0c22a32f..ed1fdb0aa044 100644
@@ -85,6 +85,8 @@ struct sk_psock {
 	struct sock			*sk_redir;
 	u32				apply_bytes;
 	u32				cork_bytes;
+	u32				copied_seq;
+	u32				ingress_bytes;
 	u32				eval;
 	bool				redir_ingress; /* undefined if sk_redir is null */
 	struct sk_msg			*cork;

include/net/strparser.h +2 -0

index 41e2ce9e9e10..0a83010b3a64 100644
@@ -43,6 +43,8 @@ struct strparser;
 struct strp_callbacks {
 	int (*parse_msg)(struct strparser *strp, struct sk_buff *skb);
 	void (*rcv_msg)(struct strparser *strp, struct sk_buff *skb);
+	int (*read_sock)(struct strparser *strp, read_descriptor_t *desc,
+			 sk_read_actor_t recv_actor);
 	int (*read_sock_done)(struct strparser *strp, int err);
 	void (*abort_parser)(struct strparser *strp, int err);
 	void (*lock)(struct strparser *strp);

include/net/tcp.h +3 -0

index e9b37b76e894..36db593984eb 100644
@@ -729,6 +729,9 @@ void tcp_get_info(struct sock *, struct tcp_info *);
 /* Read 'sendfile()'-style from a TCP socket */
 int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
 		  sk_read_actor_t recv_actor);
+int tcp_read_sock_noack(struct sock *sk, read_descriptor_t *desc,
+			sk_read_actor_t recv_actor, u32 noack,
+			u32 *copied_seq);
 int tcp_read_skb(struct sock *sk, skb_read_actor_t recv_actor);
 struct sk_buff *tcp_recv_skb(struct sock *sk, u32 seq, u32 *off);
 void tcp_read_done(struct sock *sk, size_t len);

net/core/skmsg.c +51 -0

75 lines not on this page · raw message

net/ipv4/tcp.c +26 -5

index 0d704bda6c41..32de16077ca7 100644
@@ -1565,12 +1565,13 @@ EXPORT_SYMBOL(tcp_recv_skb);
  *	  or for 'peeking' the socket using this routine
  *	  (although both would be easy to implement).
  */
-int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
-		  sk_read_actor_t recv_actor)
+static int __tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
+			   sk_read_actor_t recv_actor, u32 noack,
+			   u32 *copied_seq)
 {
 	struct sk_buff *skb;
 	struct tcp_sock *tp = tcp_sk(sk);
-	u32 seq = tp->copied_seq;
+	u32 seq = *copied_seq;
 	u32 offset;
 	int copied = 0;

@@ -1624,9 +1625,12 @@ int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
 		tcp_eat_recv_skb(sk, skb);
 		if (!desc->count)
 			break;
-		WRITE_ONCE(tp->copied_seq, seq);
+		WRITE_ONCE(*copied_seq, seq);
 	}
-	WRITE_ONCE(tp->copied_seq, seq);
+	WRITE_ONCE(*copied_seq, seq);
+
+	if (noack)
+		goto out;
 
 	tcp_rcv_space_adjust(sk);

@@ -1635,10 +1639,27 @@ int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
 		tcp_recv_skb(sk, seq, &offset);
 		tcp_cleanup_rbuf(sk, copied);
 	}
+out:
 	return copied;
 }
+
+int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
+		  sk_read_actor_t recv_actor)
+{
+	return __tcp_read_sock(sk, desc, recv_actor,
+			       0, &tcp_sk(sk)->copied_seq);
+}
 EXPORT_SYMBOL(tcp_read_sock);
 
+int tcp_read_sock_noack(struct sock *sk, read_descriptor_t *desc,
+			sk_read_actor_t recv_actor, u32 noack,
+			u32 *copied_seq)
+{
+	return __tcp_read_sock(sk, desc, recv_actor,
+			       noack, copied_seq);
+}
+EXPORT_SYMBOL(tcp_read_sock_noack);
+
 int tcp_read_skb(struct sock *sk, skb_read_actor_t recv_actor)
 {
 	struct sk_buff *skb;

net/strparser/strparser.c +9 -2

index 8299ceb3e373..95696f42647e 100644
@@ -347,7 +347,10 @@ static int strp_read_sock(struct strparser *strp)
 	struct socket *sock = strp->sk->sk_socket;
 	read_descriptor_t desc;
 
-	if (unlikely(!sock || !sock->ops || !sock->ops->read_sock))
+	if (unlikely(!sock || !sock->ops))
+		return -EBUSY;
+
+	if (unlikely(!strp->cb.read_sock && !sock->ops->read_sock))
 		return -EBUSY;
 
 	desc.arg.data = strp;

@@ -355,7 +358,10 @@ static int strp_read_sock(struct strparser *strp)
 	desc.count = 1; /* give more than one skb per call */
 
 	/* sk should be locked here, so okay to do read_sock */
-	sock->ops->read_sock(strp->sk, &desc, strp_recv);
+	if (strp->cb.read_sock)
+		strp->cb.read_sock(strp, &desc, strp_recv);
+	else
+		sock->ops->read_sock(strp->sk, &desc, strp_recv);
 
 	desc.error = strp->cb.read_sock_done(strp, desc.error);

@@ -468,6 +474,7 @@ int strp_init(struct strparser *strp, struct sock *sk,
 	strp->cb.unlock = cb->unlock ? : strp_sock_unlock;
 	strp->cb.rcv_msg = cb->rcv_msg;
 	strp->cb.parse_msg = cb->parse_msg;
+	strp->cb.read_sock = cb->read_sock;
 	strp->cb.read_sock_done = cb->read_sock_done ? : default_read_sock_done;
 	strp->cb.abort_parser = cb->abort_parser ? : strp_abort_strp;

Signature · 1 line

2.43.5

Jakub Kicinski

maintainer

2025-01-14 00:04 UTC #2 lore raw

Re: [PATCH bpf v5 1/3] bpf: fix wrong copied_seq calculation

Quoted 5 lines · On Thu, 9 Jan 2025 17:43:59 +0800 Jiayuan Chen wrote:

However, for programs where both stream_parser and stream_verdict are active(strparser purpose), tcp_read_sock() was used instead of tcp_read_skb() (sk_data_ready->strp_data_ready->tcp_read_sock) tcp_read_sock() now still update 'sk->copied_seq', leading to duplicated updates.

To state the obvious feels like the abstraction between TCP and psock has broken down pretty severely at this stage. You're modifying TCP and straight up calling TCP functions from skmsg.c :(

net/ipv4/tcp.c int tcp_read_sock(struct sock *sk, read_descriptor_t *desc, · line 1661

Quoted 8 lines

net/ipv4/tcp.c +8 -0

+int tcp_read_sock_noack(struct sock *sk, read_descriptor_t *desc,
+			sk_read_actor_t recv_actor, u32 noack,
+			u32 *copied_seq)
+{
+	return __tcp_read_sock(sk, desc, recv_actor,
+			       noack, copied_seq);
+}
+EXPORT_SYMBOL(tcp_read_sock_noack);

Pretty sure you don't have to export this. skmsg can't be a module.

Jiayuan Chen

author

2025-01-14 06:37 UTC #3 lore raw

Re: [PATCH bpf v5 1/3] bpf: fix wrong copied_seq calculation

Quoted 11 lines · Jakub Kicinski

Quoted 5 lines · On Thu, 9 Jan 2025 17:43:59 +0800 Jiayuan Chen wrote:
However, for programs where both stream_parser and stream_verdict are active(strparser purpose), tcp_read_sock() was used instead of tcp_read_skb() (sk_data_ready->strp_data_ready->tcp_read_sock) tcp_read_sock() now still update 'sk->copied_seq', leading to duplicated updates.
To state the obvious feels like the abstraction between TCP and psock has broken down pretty severely at this stage. You're modifying TCP and straight up calling TCP functions from skmsg.c :(

You are right!

How about we construct code like this:

sk_psock_strp_read_sock(strp)    skmsg.c
    tcp_bpf_strp_read_sock(sk)   tcp_bpf.c
        tcp_read_sock_noack(sk)  tcp.c

In skmsg.c we just register read_sock handler for strparser, then move core code into tcp_bpf.c. I believe it makes more sense than before as there already exist some psock with tcp operation(especially ops handler) implemented in tcp_bpf.c.

Quoted 10 lines

net/ipv4/tcp.c int tcp_read_sock(struct sock *sk, read_descriptor_t *desc, · line 1661
Quoted 8 lines
net/ipv4/tcp.c +8 -0
+int tcp_read_sock_noack(struct sock *sk, read_descriptor_t *desc,
+			sk_read_actor_t recv_actor, u32 noack,
+			u32 *copied_seq)
+{
+	return __tcp_read_sock(sk, desc, recv_actor,
+			       noack, copied_seq);
+}
+EXPORT_SYMBOL(tcp_read_sock_noack);
Pretty sure you don't have to export this. skmsg can't be a module.

ok, i will remove it.

Jakub Kicinski

maintainer

2025-01-14 18:01 UTC #4 lore raw

Re: [PATCH bpf v5 1/3] bpf: fix wrong copied_seq calculation

Quoted 8 lines · On Tue, 14 Jan 2025 14:35:34 +0800 Jiayuan Chen wrote:

Quoted 4 lines
To state the obvious feels like the abstraction between TCP and psock has broken down pretty severely at this stage. You're modifying TCP and straight up calling TCP functions from skmsg.c :(
You are right!
How about we construct code like this:

Quoted 8 lines

sk_psock_strp_read_sock(strp)    skmsg.c
    tcp_bpf_strp_read_sock(sk)   tcp_bpf.c
        tcp_read_sock_noack(sk)  tcp.c
In skmsg.c we just register read_sock handler for strparser, then move core code into tcp_bpf.c. I believe it makes more sense than before as there already exist some psock with tcp operation(especially ops handler) implemented in tcp_bpf.c.

Yes, that's slightly better, thanks

Jakub Sitnicki

maintainer

2025-01-14 11:27 UTC #5 lore raw

Re: [PATCH bpf v5 1/3] bpf: fix wrong copied_seq calculation

In reply to Jiayuan Chen

Quoted 24 lines · Jiayuan Chen

'sk->copied_seq' was updated in the tcp_eat_skb() function when the action of a BPF program was SK_REDIRECT. For other actions, like SK_PASS, the update logic for 'sk->copied_seq' was moved to tcp_bpf_recvmsg_parser() to ensure the accuracy of the 'fionread' feature.
It works for a single stream_verdict scenario, as it also modified 'sk_data_ready->sk_psock_verdict_data_ready->tcp_read_skb' to remove updating 'sk->copied_seq'.
However, for programs where both stream_parser and stream_verdict are active(strparser purpose), tcp_read_sock() was used instead of tcp_read_skb() (sk_data_ready->strp_data_ready->tcp_read_sock) tcp_read_sock() now still update 'sk->copied_seq', leading to duplicated updates.
In summary, for strparser + SK_PASS, copied_seq is redundantly calculated in both tcp_read_sock() and tcp_bpf_recvmsg_parser().
The issue causes incorrect copied_seq calculations, which prevent correct data reads from the recv() interface in user-land.
We do not want to add new proto_ops to implement a new version of tcp_read_sock, as this would introduce code complexity [1].

Quoted 6 lines

[1]: https://lore.kernel.org/bpf/[email protected]
Fixes e5c6de5fa025 ("bpf, sockmap: Incorrectly handling copied_seq")
Co-developed-by Jakub Sitnicki <[email protected]>
Signed-off-by Jakub Sitnicki <[email protected]>
Signed-off-by Jiayuan Chen <[email protected]>
---

No need to put me down as an author.
If you want you can me with a Suggested-by.

Also, please remove my Signed-off-by.
I will review these patches, but I haven't authored them.

Jiayuan Chen

author

2025-01-09 09:46 UTC #6 lore raw

PATCH v5 2/3 selftests/bpf: add strparser test for bpf

In reply to Jiayuan Chen

Add test cases for bpf + strparser and separated them from sockmap_basic, as strparser has more encapsulation and parsing capabilities compared to sockmap.

Signed-off-by Jiayuan Chen <[email protected]>

---
 .../selftests/bpf/prog_tests/sockmap_basic.c  |  53 --
 .../selftests/bpf/prog_tests/sockmap_strp.c   | 452 ++++++++++++++++++
 .../selftests/bpf/progs/test_sockmap_strp.c   |  53 ++
 3 files changed, 505 insertions(+), 53 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/sockmap_strp.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_strp.c

tools/testing/selftests/bpf/prog_tests/sockmap_basic.c +0 ~~-53~~

index 884ad87783d5..244057f49970 100644
@@ -531,57 +531,6 @@ static void test_sockmap_skb_verdict_shutdown(void)
 	test_sockmap_pass_prog__destroy(skel);
 }
 
-static void test_sockmap_stream_pass(void)
-{
-	int zero = 0, sent, recvd;
-	int verdict, parser;
-	int err, map;
-	int c = -1, p = -1;
-	struct test_sockmap_pass_prog *pass = NULL;
-	char snd[256] = "0123456789";
-	char rcv[256] = "0";
-
-	pass = test_sockmap_pass_prog__open_and_load();
-	verdict = bpf_program__fd(pass->progs.prog_skb_verdict);
-	parser = bpf_program__fd(pass->progs.prog_skb_parser);
-	map = bpf_map__fd(pass->maps.sock_map_rx);
-
-	err = bpf_prog_attach(parser, map, BPF_SK_SKB_STREAM_PARSER, 0);
-	if (!ASSERT_OK(err, "bpf_prog_attach stream parser"))
-		goto out;
-
-	err = bpf_prog_attach(verdict, map, BPF_SK_SKB_STREAM_VERDICT, 0);
-	if (!ASSERT_OK(err, "bpf_prog_attach stream verdict"))
-		goto out;
-
-	err = create_pair(AF_INET, SOCK_STREAM, &c, &p);
-	if (err)
-		goto out;
-
-	/* sk_data_ready of 'p' will be replaced by strparser handler */
-	err = bpf_map_update_elem(map, &zero, &p, BPF_NOEXIST);
-	if (!ASSERT_OK(err, "bpf_map_update_elem(p)"))
-		goto out_close;
-
-	/*
-	 * as 'prog_skb_parser' return the original skb len and
-	 * 'prog_skb_verdict' return SK_PASS, the kernel will just
-	 * pass it through to original socket 'p'
-	 */
-	sent = xsend(c, snd, sizeof(snd), 0);
-	ASSERT_EQ(sent, sizeof(snd), "xsend(c)");
-
-	recvd = recv_timeout(p, rcv, sizeof(rcv), SOCK_NONBLOCK,
-			     IO_TIMEOUT_SEC);
-	ASSERT_EQ(recvd, sizeof(rcv), "recv_timeout(p)");
-
-out_close:
-	close(c);
-	close(p);
-
-out:
-	test_sockmap_pass_prog__destroy(pass);
-}
 
 static void test_sockmap_skb_verdict_fionread(bool pass_prog)
 {

@@ -1101,8 +1050,6 @@ void test_sockmap_basic(void)
 		test_sockmap_progs_query(BPF_SK_SKB_VERDICT);
 	if (test__start_subtest("sockmap skb_verdict shutdown"))
 		test_sockmap_skb_verdict_shutdown();
-	if (test__start_subtest("sockmap stream parser and verdict pass"))
-		test_sockmap_stream_pass();
 	if (test__start_subtest("sockmap skb_verdict fionread"))
 		test_sockmap_skb_verdict_fionread(true);
 	if (test__start_subtest("sockmap skb_verdict fionread on drop"))

tools/testing/selftests/bpf/prog_tests/sockmap_strp.c new file +452 -0

452 lines not on this page · raw message

tools/testing/selftests/bpf/progs/test_sockmap_strp.c new file +53 -0

53 lines not on this page · raw message

Signature · 1 line

2.43.5

Jiayuan Chen

author

2025-01-09 09:47 UTC #7 lore raw

PATCH v5 3/3 bpf, strparser, docs: Add new callback for bpf

In reply to Jiayuan Chen

sockmap with strparser need customized read operations to fix copied_seq error.

Signed-off-by Jiayuan Chen <[email protected]>

---
 Documentation/networking/strparser.rst | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Documentation/networking/strparser.rst +10 -1

index 6cab1f74ae05..e41c18eee2f4 100644
@@ -112,7 +112,7 @@ Functions
 Callbacks
 =========
 
-There are six callbacks:
+There are seven callbacks:
 
     ::

@@ -182,6 +182,15 @@ There are six callbacks:
     the length of the message. skb->len - offset may be greater
     then full_len since strparser does not trim the skb.
 
+    ::
+
+	int (*read_sock)(struct strparser *strp, read_descriptor_t *desc,
+                     sk_read_actor_t recv_actor);
+
+    read_sock is called when the user specify it, allowing for customized
+    read operations. If the callback is not set (NULL in strp_init) native
+    read_sock operation of the socket is used.
+
     ::
 
 	int (*read_sock_done)(struct strparser *strp, int err);

Signature · 1 line

2.43.5