bpf: fix wrong copied_seq calculation
Subsystems (8)
v4 cover bpf: fix wrong copied_seq calculation
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.
---
V3 -> v4:
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.
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 | 42 ++ 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, 599 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
PATCH v4 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'.
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].
[1]: https://lore.kernel.org/bpf/[email protected]
--- include/linux/skmsg.h | 2 ++ include/net/strparser.h | 2 ++ include/net/tcp.h | 3 +++ net/core/skmsg.c | 42 +++++++++++++++++++++++++++++++++++++++ net/ipv4/tcp.c | 31 ++++++++++++++++++++++++----- net/strparser/strparser.c | 11 ++++++++-- 6 files changed, 84 insertions(+), 7 deletions(-)
include/linux/skmsg.h +2 -0
@@ -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
@@ -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
@@ -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 +42 -0
66 lines not on this page · raw message
net/ipv4/tcp.c +26 -5
@@ -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
@@ -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
PATCH v4 2/3 selftests/bpf: add strparser test for bpf
In reply to Jiayuan ChenAdd test cases for bpf + strparser and separated them from sockmap_basic, as strparser has more encapsulation and parsing capabilities compared to sockmap.
--- .../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
@@ -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
PATCH v4 3/3 bpf, strparser, docs: Add new callback for bpf
In reply to Jiayuan Chensockmap with strparser need customized read operations to fix copied_seq error.
--- Documentation/networking/strparser.rst | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
Documentation/networking/strparser.rst +10 -1
@@ -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