bpf: fix wrong copied_seq calculation and add tests
Subsystems (9)
v8 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.
---
V8 -> V7:
https://lore.kernel.org/bpf/[email protected]/
Avoid using add read_sock to psock. (Jakub Sitnicki)
Avoid using warpper function to check whether strparser is supported.
V3 -> V7:
https://lore.kernel.org/bpf/[email protected]/
https://lore.kernel.org/bpf/[email protected]/
Avoid introducing new proto_ops. (Jakub Sitnicki). Add more edge test cases for strparser + bpf. Fix patchwork fail of test cases code.
Fix psock fetch without rcu lock.
Move code of modifying to tcp_bpf.c.
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 (5):
strparser: add read_sock callback
bpf: fix wrong copied_seq calculation
bpf: disable non stream socket for strparser
selftests/bpf: fix invalid flag of recv()
selftests/bpf: add strparser test for bpf
Documentation/networking/strparser.rst | 9 +- include/linux/skmsg.h | 2 + include/net/strparser.h | 2 + include/net/tcp.h | 8 + net/core/skmsg.c | 7 + net/core/sock_map.c | 5 +- net/ipv4/tcp.c | 29 +- net/ipv4/tcp_bpf.c | 42 ++ net/strparser/strparser.c | 11 +- .../selftests/bpf/prog_tests/sockmap_basic.c | 59 +-- .../selftests/bpf/prog_tests/sockmap_strp.c | 452 ++++++++++++++++++ .../selftests/bpf/progs/test_sockmap_strp.c | 53 ++ 12 files changed, 614 insertions(+), 65 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 v8 1/5 strparser: add read_sock callback
Added a new read_sock handler, allowing users to customize read operations instead of relying on the native socket's read_sock.
--- Documentation/networking/strparser.rst | 9 ++++++++- include/net/strparser.h | 2 ++ net/strparser/strparser.c | 11 +++++++++-- 3 files changed, 19 insertions(+), 3 deletions(-)
Documentation/networking/strparser.rst +8 -1
@@ -112,7 +112,7 @@ Functions Callbacks ========= -There are six callbacks: +There are seven callbacks: ::
@@ -182,6 +182,13 @@ 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); + + The read_sock callback is used by strparser instead of + sock->ops->read_sock, if provided. :: int (*read_sock_done)(struct strparser *strp, int err);
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);
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
Re: [PATCH bpf v8 1/5] strparser: add read_sock callback
Quoted 5 lines · Jiayuan Chen
Added a new
read_sockhandler, allowing users to customize read operations instead of relying on the native socket'sread_sock.Signed-off-by Jiayuan Chen <[email protected]>---
PATCH v8 2/5 bpf: fix wrong copied_seq calculation
In reply to 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].
We add new callback for strparser for customized read operation.
[1]: https://lore.kernel.org/bpf/[email protected]
--- include/linux/skmsg.h | 2 ++ include/net/tcp.h | 8 ++++++++ net/core/skmsg.c | 7 +++++++ net/ipv4/tcp.c | 29 ++++++++++++++++++++++++----- net/ipv4/tcp_bpf.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 83 insertions(+), 5 deletions(-)
include/linux/skmsg.h +2 -0
@@ -91,6 +91,8 @@ struct sk_psock {struct sk_psock_progs progs;#if IS_ENABLED (CONFIG_BPF_STREAM_PARSER )struct strparser strp; +u32 copied_seq; +u32 ingress_bytes;#endif struct sk_buff_head ingress_skb;struct list_head ingress_msg;
include/net/tcp.h +8 -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,bool 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);
@@ -2599,6 +2602,11 @@ struct sk_psock;#ifdef CONFIG_BPF_SYSCALL int tcp_bpf_update_proto (struct sock *sk,struct sk_psock *psock,bool restore);void tcp_bpf_clone (const struct sock *sk,struct sock *newsk); +#ifdef CONFIG_BPF_STREAM_PARSER +struct strparser ; +int tcp_bpf_strp_read_sock (struct strparser *strp,read_descriptor_t *desc, +sk_read_actor_t recv_actor); +#endif /* CONFIG_BPF_STREAM_PARSER */ #endif /* CONFIG_BPF_SYSCALL */ #ifdef CONFIG_INET
net/core/skmsg.c +7 -0
@@ -549,6 +549,9 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb,return num_sge; } +#if IS_ENABLED (CONFIG_BPF_STREAM_PARSER ) + psock->ingress_bytes += len; +#endif copied = len; msg->sg.start =0 ; msg->sg.size = copied;
@@ -1144,6 +1147,10 @@ int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock)if (!ret)sk_psock_set_state (psock,SK_PSOCK_RX_STRP_ENABLED ); +if (sk_is_tcp (sk)) { + psock->strp.cb.read_sock = tcp_bpf_strp_read_sock; + psock->copied_seq =tcp_sk (sk)->copied_seq; + }return ret; }
net/ipv4/tcp.c +24 -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,bool 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,25 @@ 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, false, + &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,bool noack, +u32 *copied_seq) +{ +return __tcp_read_sock (sk, desc, recv_actor, noack, copied_seq); +} +int tcp_read_skb (struct sock *sk,skb_read_actor_t recv_actor) {struct sk_buff *skb;
net/ipv4/tcp_bpf.c +42 -0
48 lines not on this page · raw message
Signature · 1 line
2.43.5
Jakub Sitnicki
maintainerRe: [PATCH bpf v8 2/5] bpf: fix wrong copied_seq calculation
Quoted 11 lines · Jiayuan Chen
'
sk->copied_seq' was updated in thetcp_eat_skb()function when the action of a BPF program wasSK_REDIRECT. For other actions, likeSK_PASS, the update logic for 'sk->copied_seq' was moved totcp_bpf_recvmsg_parser()to ensure the accuracy of the 'fionread' feature.It works for a single
stream_verdictscenario, 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_parserandstream_verdictare active(strparser purpose),tcp_read_sock()was used instead of
Nit: missing space, "active (strparser purpose)"
^
Quoted 1 line
tcp_read_skb()(sk_data_ready->strp_data_ready->tcp_read_sock)
Nit: missing period, "… (sk_data_ready->strp_data_ready->tcp_read_sock)."
^
Quoted 1 line
tcp_read_sock()now still update 'sk->copied_seq', leading to duplicated
Nit: grammar "still updates"
^
Please run commit descriptions through a language tool or an LLM, if possible. This makes reviewing easier.
Quoted 162 lines
updates.
In summary, for strparser +
SK_PASS,copied_seqis redundantly calculated in bothtcp_read_sock()andtcp_bpf_recvmsg_parser().The issue causes incorrect
copied_seqcalculations, which prevent correct data reads from therecv()interface in user-land.We do not want to add new
proto_opsto implement a new version oftcp_read_sock, as this would introduce code complexity [1].We add new callback for strparser for customized read operation.
[1]: https://lore.kernel.org/bpf/[email protected]
Fixes e5c6de5fa025 ("bpf, sockmap: Incorrectly handling copied_seq")Suggested-by Jakub Sitnicki <[email protected]>Signed-off-by Jiayuan Chen <[email protected]>--- include/linux/skmsg.h | 2 ++ include/net/tcp.h | 8 ++++++++ net/core/skmsg.c | 7 +++++++ net/ipv4/tcp.c | 29 ++++++++++++++++++++++++----- net/ipv4/tcp_bpf.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 83 insertions(+), 5 deletions(-)include/linux/skmsg.h +2
-0@@ -91,6 +91,8 @@ struct sk_psock {struct sk_psock_progs progs;#if IS_ENABLED (CONFIG_BPF_STREAM_PARSER )struct strparser strp; +u32 copied_seq; +u32 ingress_bytes;#endif struct sk_buff_head ingress_skb;struct list_head ingress_msg;include/net/tcp.h +8
-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,bool 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);@@ -2599,6 +2602,11 @@ struct sk_psock;#ifdef CONFIG_BPF_SYSCALL int tcp_bpf_update_proto (struct sock *sk,struct sk_psock *psock,bool restore);void tcp_bpf_clone (const struct sock *sk,struct sock *newsk); +#ifdef CONFIG_BPF_STREAM_PARSER +struct strparser ; +int tcp_bpf_strp_read_sock (struct strparser *strp,read_descriptor_t *desc, +sk_read_actor_t recv_actor); +#endif /* CONFIG_BPF_STREAM_PARSER */ #endif /* CONFIG_BPF_SYSCALL */ #ifdef CONFIG_INET net/core/skmsg.c +7
-0@@ -549,6 +549,9 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb,return num_sge; } +#if IS_ENABLED (CONFIG_BPF_STREAM_PARSER ) + psock->ingress_bytes += len; +#endif copied = len; msg->sg.start =0 ; msg->sg.size = copied;@@ -1144,6 +1147,10 @@ int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock)if (!ret)sk_psock_set_state (psock,SK_PSOCK_RX_STRP_ENABLED ); +if (sk_is_tcp (sk)) { + psock->strp.cb.read_sock = tcp_bpf_strp_read_sock; + psock->copied_seq =tcp_sk (sk)->copied_seq; + }return ret; }net/ipv4/tcp.c +24
-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,bool 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,25 @@ 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, false, + &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,bool noack, +u32 *copied_seq) +{ +return __tcp_read_sock (sk, desc, recv_actor, noack, copied_seq); +} +int tcp_read_skb (struct sock *sk,skb_read_actor_t recv_actor) {struct sk_buff *skb;net/ipv4/tcp_bpf.c +3
-0@@ -646,6 +646,48 @@ static int tcp_bpf_assert_proto_ops(struct proto *ops) ops->sendmsg == tcp_sendmsg ?0 : -ENOTSUPP ; } +#if IS_ENABLED (CONFIG_BPF_STREAM_PARSER ) +int tcp_bpf_strp_read_sock (struct strparser *strp,read_descriptor_t *desc, +sk_read_actor_t recv_actor)
net/ipv4/tcp_bpf.c static int tcp_bpf_assert_proto_ops(struct proto *ops) · line 661
Quoted 10 lines
net/ipv4/tcp_bpf.c +10
-0+{ +struct sock *sk = strp->sk; +struct sk_psock *psock; +struct tcp_sock *tp; +int copied =0 ; + + tp =tcp_sk (sk); +rcu_read_lock (); + psock =sk_psock (sk); +if (WARN_ON (!psock)) {
WARN_ON_ONCE, please. We don't want to flood dmesg.
net/ipv4/tcp_bpf.c static int tcp_bpf_assert_proto_ops(struct proto *ops) · line 672
Quoted 11 lines
net/ipv4/tcp_bpf.c +11
-0+ desc->error = -EINVAL ; + goto out; + } + + psock->ingress_bytes =0 ; +/* We could easily add copied_seq and noack into desc then call +* ops->read_sock without calling symbol directly. But unfortunately +* most descriptors used by other modules are not inited with zero. +* Also it not work by replacing ops->read_sock without introducing +* new ops as ops itself is located in rodata segment. +*/
Above commment explains an implementation decision and belongs in the patch description, not in the code. It does not help with understanding the code itself.
net/ipv4/tcp_bpf.c static int tcp_bpf_assert_proto_ops(struct proto *ops) · line 677
Quoted 5 lines
net/ipv4/tcp_bpf.c +5
-0+ copied =tcp_read_sock_noack (sk, desc, recv_actor, true, + &psock->copied_seq); +if (copied <0 ) + goto out; +/* recv_actor may redirect skb to another socket(SK_REDIRECT) or
Nit: missing space, "socket (SK_REDIRECT)"
net/ipv4/tcp_bpf.c static int tcp_bpf_assert_proto_ops(struct proto *ops) · line 678
Quoted 1 line
net/ipv4/tcp_bpf.c +1
-0+* just put skb into ingress queue of current socket(SK_PASS).
Nit: missing space, "socket (SK_PASS)"
net/ipv4/tcp_bpf.c static int tcp_bpf_assert_proto_ops(struct proto *ops) · line 679
Quoted 1 line
net/ipv4/tcp_bpf.c +1
-0+* For SK_REDIRECT, we need 'ack' the frame immediately but for
Nit: grammar, "we need to"
Nit: style, "to ack" is an accepted form of "to acknowlege", no need for quotes around it.
net/ipv4/tcp_bpf.c static int tcp_bpf_assert_proto_ops(struct proto *ops) · line 680
Quoted 1 line
net/ipv4/tcp_bpf.c +1
-0+* SK_PASS, the 'ack' was delay to tcp_bpf_recvmsg_parser()
Nit: grammar, "we want to delay the ack until tcp_bpf_recvmsg_parser()"
Quoted 13 lines
net/ipv4/tcp_bpf.c +10
-0+*/ + tp->copied_seq = psock->copied_seq - psock->ingress_bytes; +tcp_rcv_space_adjust (sk); +__tcp_cleanup_rbuf (sk, copied - psock->ingress_bytes); +out: +rcu_read_unlock (); + return copied; +} +#endif /* CONFIG_BPF_STREAM_PARSER */ +int tcp_bpf_update_proto (struct sock *sk,struct sk_psock *psock,bool restore) {int family = sk->sk_family ==AF_INET6 ?TCP_BPF_IPV6 :TCP_BPF_IPV4 ;
Re: [PATCH bpf v8 2/5] bpf: fix wrong copied_seq calculation
Quoted 19 lines · Jakub Sitnicki
Quoted 11 lines · Jiayuan Chen
'
sk->copied_seq' was updated in thetcp_eat_skb()function when the action of a BPF program wasSK_REDIRECT. For other actions, likeSK_PASS, the update logic for 'sk->copied_seq' was moved totcp_bpf_recvmsg_parser()to ensure the accuracy of the 'fionread' feature.It works for a single
stream_verdictscenario, 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_parserandstream_verdictare active(strparser purpose),tcp_read_sock()was used instead ofNit: missing space, "active (strparser purpose)"
^Quoted 1 line
tcp_read_skb()(sk_data_ready->strp_data_ready->tcp_read_sock)Nit: missing period, "… (
sk_data_ready->strp_data_ready->tcp_read_sock)."
Quoted 9 lines
^Quoted 1 line
tcp_read_sock()now still update 'sk->copied_seq', leading to duplicatedNit: grammar "still updates"
^Please run commit descriptions through a language tool or an LLM, if possible. This makes reviewing easier.
Thanks Jakub, I'll review all commit messages and code comments, and also use LLLM for grammar checks.
PATCH v8 3/5 bpf: disable non stream socket for strparser
In reply to Jiayuan ChenCurrently, only TCP supports strparser, but sockmap doesn't intercept non-TCP to attach strparser. For example, with UDP, although the read/write handlers are replaced, strparser is not executed due to the lack of read_sock operation.
Furthermore, in udp_bpf_recvmsg(), it checks whether psock has data, and if not, it falls back to the native UDP read interface, making UDP + strparser appear to read correctly. According to it's commit history, the behavior is unexpected.
Moreover, since UDP lacks the concept of streams, we intercept it directly.
--- net/core/sock_map.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
net/core/sock_map.c +4 -1
@@ -303,7 +303,10 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)write_lock_bh (&sk->sk_callback_lock);if (stream_parser && stream_verdict && !psock->saved_data_ready) { - ret =sk_psock_init_strp (sk, psock); +if (sk_is_tcp (sk)) + ret =sk_psock_init_strp (sk, psock); +else + ret = -EOPNOTSUPP ;if (ret) {write_unlock_bh (&sk->sk_callback_lock);sk_psock_put (sk, psock);
Signature · 1 line
2.43.5
Re: [PATCH bpf v8 3/5] bpf: disable non stream socket for strparser
Quoted 8 lines · Jiayuan Chen
Currently, only TCP supports strparser, but sockmap doesn't intercept non-TCP to attach strparser. For example, with UDP, although the read/write handlers are replaced, strparser is not executed due to the lack of
read_sockoperation.Furthermore, in
udp_bpf_recvmsg(), it checks whether psock has data, and if not, it falls back to the native UDP read interface, making UDP + strparser appear to read correctly. According to it's commit
Nit: typo, "its"
Quoted 16 lines
history, the behavior is unexpected.
Moreover, since UDP lacks the concept of streams, we intercept it directly.
Signed-off-by Jiayuan Chen <[email protected]>--- net/core/sock_map.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)net/core/sock_map.c +0
-0@@ -303,7 +303,10 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
net/core/sock_map.c static int sock_map_link(struct bpf_map *map, struct sock *sk) · line 312
Quoted 10 lines
net/core/sock_map.c +4
-1write_lock_bh (&sk->sk_callback_lock);if (stream_parser && stream_verdict && !psock->saved_data_ready) { - ret =sk_psock_init_strp (sk, psock); +if (sk_is_tcp (sk)) + ret =sk_psock_init_strp (sk, psock); +else + ret = -EOPNOTSUPP ;if (ret) {write_unlock_bh (&sk->sk_callback_lock);sk_psock_put (sk, psock);
PATCH v8 4/5 selftests/bpf: fix invalid flag of recv()
In reply to Jiayuan ChenSOCK_NONBLOCK flag is only effective during socket creation, not during recv. Use MSG_DONTWAIT instead.
--- tools/testing/selftests/bpf/prog_tests/sockmap_basic.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
tools/testing/selftests/bpf/prog_tests/sockmap_basic.c +3 -3
@@ -522,8 +522,8 @@ static void test_sockmap_skb_verdict_shutdown(void)if (!ASSERT_EQ (err,1 ,"epoll_wait(fd)" )) goto out_close; - n =recv (c1, &b,1 ,SOCK_NONBLOCK ); -ASSERT_EQ (n,0 ,"recv_timeout(fin)" ); + n =recv (c1, &b,1 ,MSG_DONTWAIT ); +ASSERT_EQ (n,0 ,"recv(fin)" ); out_close:close (c1);close (p1);
@@ -628,7 +628,7 @@ static void test_sockmap_skb_verdict_fionread(bool pass_prog)ASSERT_EQ (avail, expected,"ioctl(FIONREAD)" );/* On DROP test there will be no data to read */ if (pass_prog) { - recvd =recv_timeout (c1, &buf,sizeof (buf),SOCK_NONBLOCK ,IO_TIMEOUT_SEC ); + recvd =recv_timeout (c1, &buf,sizeof (buf),MSG_DONTWAIT ,IO_TIMEOUT_SEC );ASSERT_EQ (recvd,sizeof (buf),"recv_timeout(c0)" ); }
Signature · 1 line
2.43.5
Re: [PATCH bpf v8 4/5] selftests/bpf: fix invalid flag of recv()
Quoted 5 lines · Jiayuan Chen
SOCK_NONBLOCKflag is only effective during socket creation, not during recv. UseMSG_DONTWAITinstead.Signed-off-by Jiayuan Chen <[email protected]>---
Good catch.
PATCH v8 5/5 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
Jakub Sitnicki
maintainerRe: [PATCH bpf v8 5/5] selftests/bpf: add strparser test for bpf
Thanks for expanding tests.
Quoted 90 lines · 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.ctools/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 +0
-0
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c line 7
Quoted 7 lines
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c new file +7
-0@@ -0,0 +1,452 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <error.h> +#include <netinet/tcp.h> +#include <test_progs.h> +#include "sockmap_helpers.h" +#include "test_skmsg_load_helpers.skel.h" +#include "test_sockmap_strp.skel.h"
Nit: add new line to separate cpp defines visually
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c line 10
Quoted 3 lines
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c new file +3
-0+#define STRP_PKT_HEAD_LEN 4 +#define STRP_PKT_BODY_LEN 6 +#define STRP_PKT_FULL_LEN (STRP_PKT_HEAD_LEN + STRP_PKT_BODY_LEN)
Nit: add new line to constants visually
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c line 14
Quoted 4 lines
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c new file +4
-0+static const char packet[STRP_PKT_FULL_LEN ] ="head+body\0" ; +static const int test_packet_num =100 ; + +/* current implementation of tcp_bpf_recvmsg_parser() invoke
Nit: grammar, "invoke*s*"
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c line 15
Quoted 1 line
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c new file +1
-0+* data_ready with sk held if skb exist in sk_receive_queue.
Nit: grammar, "exist*s*"
Quoted 5 lines
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c new file +5
-0+* Then for data_ready implementation of strparser, it will +* delay the read operation if sk was held and EAGAIN is returned. +*/ +static int sockmap_strp_consume_pre_data (int p) +{
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c line 30
Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c new file +10
-0+int recvd; +bool retried = false; +char rcv[10 ]; + +retry: + errno =0 ; + recvd =recv_timeout (p, rcv,sizeof (rcv),0 ,1 ); +if (recvd <0 && errno ==EAGAIN && retried == false) { +/* On the first call, EAGAIN will certainly be returned. +* Waiting 1 second is pretty enough wait workqueue finish.
Nit: style, "Wait for workqueue to finish."
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c line 37
Quoted 7 lines
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c new file +7
-0+*/ +sleep (1 ); + retried = true; + goto retry; + } + + if (!ASSERT_EQ(recvd, STRP_PKT_FULL_LEN, "recv_timeout(pre-data)") ||
Meaningful error message like "recv error or truncated data" would be better. ASSERT_EQ / CHECK macros print the function name, so "(pre-data)" tag is redundant.
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c line 39
Quoted 2 lines
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c new file +2
-0+ !ASSERT_OK (memcmp (packet, rcv,STRP_PKT_FULL_LEN ), + "memcmp pre-data"))
Suggested error message: "data mismatch".
Quoted 199 lines
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c new file +199
-0+return -1 ; +return 0 ; +} + +static struct test_sockmap_strp *sockmap_strp_init (int *out_map,bool pass, +bool need_parser) +{ +struct test_sockmap_strp *strp =NULL ; +int verdict, parser; +int err; + + strp =test_sockmap_strp__open_and_load (); + *out_map =bpf_map__fd (strp->maps.sock_map); + +if (need_parser) + parser =bpf_program__fd (strp->progs.prog_skb_parser_partial); +else + parser =bpf_program__fd (strp->progs.prog_skb_parser); + +if (pass) + verdict =bpf_program__fd (strp->progs.prog_skb_verdict_pass); +else + verdict =bpf_program__fd (strp->progs.prog_skb_verdict); + + err =bpf_prog_attach (parser, *out_map,BPF_SK_SKB_STREAM_PARSER ,0 ); +if (!ASSERT_OK (err,"bpf_prog_attach stream parser" )) + goto err; + + err =bpf_prog_attach (verdict, *out_map,BPF_SK_SKB_STREAM_VERDICT ,0 ); +if (!ASSERT_OK (err,"bpf_prog_attach stream verdict" )) + goto err; + +return strp; +err: +test_sockmap_strp__destroy (strp); +return NULL ; +} + +/* Dispatch packets to different socket by packet size: +* +* ------ ------ +* | pkt4 || pkt1 |... > remote socket +* ------ ------ / ------ ------ +* | pkt8 | pkt7 |... +* ------ ------ \ ------ ------ +* | pkt3 || pkt2 |... > local socket +* ------ ------ +*/ +static void test_sockmap_strp_dispatch_pkt (int family,int sotype) +{ +int i, j, zero =0 , one =1 , recvd; +int err, map; +int c0 =-1 , p0 =-1 , c1 =-1 , p1 =-1 ; +struct test_sockmap_strp *strp =NULL ; +int test_cnt =6 ; +char rcv[10 ]; +struct { +char data[7 ]; +int data_len; +int send_cnt; +int *receiver; + } send_dir[2 ] = { +/* data expected to deliver to local */ + {"llllll" ,6 ,0 , &p0}, +/* data expected to deliver to remote */ + {"rrrrr" ,5 ,0 , &c1} + }; + + strp =sockmap_strp_init (&map, false, false); +if (!ASSERT_TRUE (strp,"sockmap_strp_init" )) +return ; + + err =create_socket_pairs (family, sotype, &c0, &c1, &p0, &p1); +if (!ASSERT_OK (err,"create_socket_pairs()" )) + goto out; + + err =bpf_map_update_elem (map, &zero, &p0,BPF_NOEXIST ); +if (!ASSERT_OK (err,"bpf_map_update_elem(p0)" )) + goto out_close; + + err =bpf_map_update_elem (map, &one, &p1,BPF_NOEXIST ); +if (!ASSERT_OK (err,"bpf_map_update_elem(p1)" )) + goto out_close; + + err =setsockopt (c1,IPPROTO_TCP ,TCP_NODELAY , &zero,sizeof (zero)); +if (!ASSERT_OK (err,"setsockopt(TCP_NODELAY)" )) + goto out_close; + +/* deliver data with data size greater than 5 to local */ + strp->data->verdict_max_size =5 ; + +for (i =0 ; i < test_cnt; i++) { +int d = i %2 ; + +xsend (c0, send_dir[d].data, send_dir[d].data_len,0 ); + send_dir[d].send_cnt++; + } + +for (i =0 ; i <2 ; i++) { +for (j =0 ; j < send_dir[i].send_cnt; j++) { +int expected = send_dir[i].data_len; + + recvd =recv_timeout (*send_dir[i].receiver, rcv, + expected,MSG_DONTWAIT , +IO_TIMEOUT_SEC ); +if (!ASSERT_EQ (recvd, expected,"recv_timeout()" )) + goto out_close; +if (!ASSERT_OK (memcmp (send_dir[i].data, rcv, recvd), +"memcmp(rcv)" )) + goto out_close; + } + } +out_close: +close (c0); +close (c1); +close (p0); +close (p1); +out: +test_sockmap_strp__destroy (strp); +} + +/* We have multiple packets in one skb +* ------------ ------------ ------------ +* | packet1 | packet2 | ... +* ------------ ------------ ------------ +*/ +static void test_sockmap_strp_multiple_pkt (int family,int sotype) +{ +int i, zero =0 ; +int sent, recvd, total; +int err, map; +int c =-1 , p =-1 ; +struct test_sockmap_strp *strp =NULL ; +char *snd =NULL , *rcv =NULL ; + + strp =sockmap_strp_init (&map, true, true); +if (!ASSERT_TRUE (strp,"sockmap_strp_init" )) +return ; + + err =create_pair (family, sotype, &c, &p); +if (err) + goto out; + + err =bpf_map_update_elem (map, &zero, &p,BPF_NOEXIST ); +if (!ASSERT_OK (err,"bpf_map_update_elem(zero, p)" )) + goto out_close; + +/* construct multiple packets in one buffer */ + total = test_packet_num *STRP_PKT_FULL_LEN ; + snd =malloc (total); + rcv =malloc (total +1 ); +if (!ASSERT_TRUE (snd,"malloc(snd)" ) || + !ASSERT_TRUE (rcv,"malloc(rcv)" )) + goto out_close; + +for (i =0 ; i < test_packet_num; i++) { +memcpy (snd + i *STRP_PKT_FULL_LEN , + packet,STRP_PKT_FULL_LEN ); + } + + sent =xsend (c, snd, total,0 ); +if (!ASSERT_EQ (sent, total,"xsend(c)" )) + goto out_close; + +/* try to recv one more byte to avoid truncation check */ + recvd =recv_timeout (p, rcv, total +1 ,MSG_DONTWAIT ,IO_TIMEOUT_SEC ); +if (!ASSERT_EQ (recvd, total,"recv(rcv)" )) + goto out_close; + +/* we sent TCP segment with multiple encapsulation +* then check whether packets are handled correctly +*/ +if (!ASSERT_OK (memcmp (snd, rcv, total),"memcmp(snd, rcv)" )) + goto out_close; + +out_close: +close (c); +close (p); +if (snd) +free (snd); +if (rcv) +free (rcv); +out: +test_sockmap_strp__destroy (strp); +} + +/* Test strparser with partial read */ +static void test_sockmap_strp_partial_read (int family,int sotype) +{ +int zero =0 , recvd, off; +int err, map; +int c =-1 , p =-1 ; +struct test_sockmap_strp *strp =NULL ; +char rcv[STRP_PKT_FULL_LEN +1 ] ="0" ; + + strp =sockmap_strp_init (&map, true, true); +if (!ASSERT_TRUE (strp,"sockmap_strp_init" )) +return ; +
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c line 248
Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c new file +10
-0+ err =create_pair (family, sotype, &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(zero, p)" )) + goto out_close; + +/* 1.1 send partial head, 1 byte header left*/
Nit: missing space before comment-close tag, "left */".
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c line 252
Quoted 4 lines
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c new file +4
-0+ off =STRP_PKT_HEAD_LEN -1 ; +xsend (c, packet, off,0 ); + recvd =recv_timeout (p, rcv,sizeof (rcv),MSG_DONTWAIT ,1 ); +if (!ASSERT_EQ (-1 , recvd,"insufficient head, should no data recvd" ))
"partial head sent, expected no data"
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c line 258
Quoted 6 lines
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c new file +6
-0+ goto out_close; + +/* 1.2 send remaining head and body */ +xsend (c, packet + off,STRP_PKT_FULL_LEN - off,0 ); + recvd =recv_timeout (p, rcv,sizeof (rcv),MSG_DONTWAIT ,IO_TIMEOUT_SEC ); +if (!ASSERT_EQ (recvd,STRP_PKT_FULL_LEN ,"should full data recvd" ))
"expected full data"
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c line 269
Quoted 11 lines
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c new file +11
-0+ goto out_close; + +/* 2.1 send partial head, 1 byte header left */ + off =STRP_PKT_HEAD_LEN -1 ; +xsend (c, packet, off,0 ); + +/* 2.2 send remaining head and partial body, 1 byte body left */ +xsend (c, packet + off,STRP_PKT_FULL_LEN - off -1 ,0 ); + off =STRP_PKT_FULL_LEN -1 ; + recvd =recv_timeout (p, rcv,sizeof (rcv),MSG_DONTWAIT ,1 ); +if (!ASSERT_EQ (-1 , recvd,"insufficient body, should no data read" ))
"partial body sent, expected no data"
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c line 275
Quoted 6 lines
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c new file +6
-0+ goto out_close; + +/* 2.3 send remaining body */ +xsend (c, packet + off,STRP_PKT_FULL_LEN - off,0 ); + recvd =recv_timeout (p, rcv,sizeof (rcv),MSG_DONTWAIT ,IO_TIMEOUT_SEC ); +if (!ASSERT_EQ (recvd,STRP_PKT_FULL_LEN ,"should full data recvd" ))
"expected full data"
Quoted 236 lines
tools/testing/selftests/bpf/prog_tests/sockmap_strp.c new file +177
-0+ goto out_close; + +out_close: +close (c); +close (p); + +out: +test_sockmap_strp__destroy (strp); +} + +/* Test simple socket read/write with strparser + FIONREAD */ +static void test_sockmap_strp_pass (int family,int sotype,bool fionread) +{ +int zero =0 , pkt_size =STRP_PKT_FULL_LEN , sent, recvd, avail; +int err, map; +int c =-1 , p =-1 ; +int test_cnt =10 , i; +struct test_sockmap_strp *strp =NULL ; +char rcv[STRP_PKT_FULL_LEN +1 ] ="0" ; + + strp =sockmap_strp_init (&map, true, true); +if (!ASSERT_TRUE (strp,"sockmap_strp_init" )) +return ; + + err =create_pair (family, sotype, &c, &p); +if (err) + goto out; + +/* inject some data before bpf process, it should be read +* correctly because we check sk_receive_queue in +* tcp_bpf_recvmsg_parser() +*/ + sent =xsend (c, packet, pkt_size,0 ); +if (!ASSERT_EQ (sent, pkt_size,"xsend(pre-data)" )) + goto out_close; + +/* 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; + +/* consume previous data we injected */ +if (sockmap_strp_consume_pre_data (p)) + goto out_close; + +/* Previously, we encountered issues such as deadlocks and +* sequence errors that resulted in the inability to read +* continuously. Therefore, we perform multiple iterations +* of testing here. +*/ +for (i =0 ; i < test_cnt; i++) { + sent =xsend (c, packet, pkt_size,0 ); +if (!ASSERT_EQ (sent, pkt_size,"xsend(c)" )) + goto out_close; + + recvd =recv_timeout (p, rcv,sizeof (rcv),MSG_DONTWAIT , +IO_TIMEOUT_SEC ); +if (!ASSERT_EQ (recvd, pkt_size,"recv_timeout(p)" ) || + !ASSERT_OK (memcmp (packet, rcv, pkt_size), +"memcmp" )) + goto out_close; + } + +if (fionread) { + sent =xsend (c, packet, pkt_size,0 ); +if (!ASSERT_EQ (sent, pkt_size,"second xsend(c)" )) + goto out_close; + + err =ioctl (p,FIONREAD , &avail); +if (!ASSERT_OK (err,"ioctl(FIONREAD) error" ) || + !ASSERT_EQ (avail, pkt_size,"ioctl(FIONREAD)" )) + goto out_close; + + recvd =recv_timeout (p, rcv,sizeof (rcv),MSG_DONTWAIT , +IO_TIMEOUT_SEC ); +if (!ASSERT_EQ (recvd, pkt_size,"second recv_timeout(p)" ) || + !ASSERT_OK (memcmp (packet, rcv, pkt_size), +"second memcmp" )) + goto out_close; + } + +out_close: +close (c); +close (p); + +out: +test_sockmap_strp__destroy (strp); +} + +/* Test strparser with verdict mode */ +static void test_sockmap_strp_verdict (int family,int sotype) +{ +int zero =0 , one =1 , sent, recvd, off; +int err, map; +int c0 =-1 , p0 =-1 , c1 =-1 , p1 =-1 ; +struct test_sockmap_strp *strp =NULL ; +char rcv[STRP_PKT_FULL_LEN +1 ] ="0" ; + + strp =sockmap_strp_init (&map, false, true); +if (!ASSERT_TRUE (strp,"sockmap_strp_init" )) +return ; + +/* We simulate a reverse proxy server. +* When p0 receives data from c0, we forward it to c1. +* From c1's perspective, it will consider this data +* as being sent by p1. +*/ + err =create_socket_pairs (family, sotype, &c0, &c1, &p0, &p1); +if (!ASSERT_OK (err,"create_socket_pairs()" )) + goto out; + + err =bpf_map_update_elem (map, &zero, &p0,BPF_NOEXIST ); +if (!ASSERT_OK (err,"bpf_map_update_elem(p0)" )) + goto out_close; + + err =bpf_map_update_elem (map, &one, &p1,BPF_NOEXIST ); +if (!ASSERT_OK (err,"bpf_map_update_elem(c1)" )) + goto out_close; + + sent =xsend (c0, packet,STRP_PKT_FULL_LEN ,0 ); +if (!ASSERT_EQ (sent,STRP_PKT_FULL_LEN ,"xsend(c0)" )) + goto out_close; + + recvd =recv_timeout (c1, rcv,sizeof (rcv),MSG_DONTWAIT , +IO_TIMEOUT_SEC ); +if (!ASSERT_EQ (recvd,STRP_PKT_FULL_LEN ,"recv_timeout(p1)" ) || + !ASSERT_OK (memcmp (packet, rcv,STRP_PKT_FULL_LEN ), +"received data does not match the sent data" )) + goto out_close; + +/* send again to ensure the stream is functioning correctly. */ + sent =xsend (c0, packet,STRP_PKT_FULL_LEN ,0 ); +if (!ASSERT_EQ (sent,STRP_PKT_FULL_LEN ,"second xsend(c0)" )) + goto out_close; + +/* partial read */ + off =STRP_PKT_FULL_LEN /2 ; + recvd =recv_timeout (c1, rcv, off,MSG_DONTWAIT , +IO_TIMEOUT_SEC ); + recvd +=recv_timeout (c1, rcv + off,sizeof (rcv) - off,MSG_DONTWAIT , +IO_TIMEOUT_SEC ); + +if (!ASSERT_EQ (recvd,STRP_PKT_FULL_LEN ,"partial recv_timeout(c1)" ) || + !ASSERT_OK (memcmp (packet, rcv,STRP_PKT_FULL_LEN ), +"partial received data does not match the sent data" )) + goto out_close; + +out_close: +close (c0); +close (c1); +close (p0); +close (p1); +out: +test_sockmap_strp__destroy (strp); +} + +void test_sockmap_strp (void ) +{ +if (test__start_subtest ("sockmap strp tcp pass" )) +test_sockmap_strp_pass (AF_INET ,SOCK_STREAM , false); +if (test__start_subtest ("sockmap strp tcp v6 pass" )) +test_sockmap_strp_pass (AF_INET6 ,SOCK_STREAM , false); +if (test__start_subtest ("sockmap strp tcp pass fionread" )) +test_sockmap_strp_pass (AF_INET ,SOCK_STREAM , true); +if (test__start_subtest ("sockmap strp tcp v6 pass fionread" )) +test_sockmap_strp_pass (AF_INET6 ,SOCK_STREAM , true); +if (test__start_subtest ("sockmap strp tcp verdict" )) +test_sockmap_strp_verdict (AF_INET ,SOCK_STREAM ); +if (test__start_subtest ("sockmap strp tcp v6 verdict" )) +test_sockmap_strp_verdict (AF_INET6 ,SOCK_STREAM ); +if (test__start_subtest ("sockmap strp tcp partial read" )) +test_sockmap_strp_partial_read (AF_INET ,SOCK_STREAM ); +if (test__start_subtest ("sockmap strp tcp multiple packets" )) +test_sockmap_strp_multiple_pkt (AF_INET ,SOCK_STREAM ); +if (test__start_subtest ("sockmap strp tcp dispatch" )) +test_sockmap_strp_dispatch_pkt (AF_INET ,SOCK_STREAM ); +}tools/testing/selftests/bpf/progs/test_sockmap_strp.c new file +53
-0@@ -0,0 +1,53 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <linux/bpf.h> +#include <bpf/bpf_helpers.h> +#include <bpf/bpf_endian.h> +int verdict_max_size =10000 ; +struct { + __uint(type, BPF_MAP_TYPE_SOCKMAP); + __uint(max_entries, 20); + __type(key, int); + __type(value, int); +} sock_map SEC(".maps"); + +SEC("sk_skb/stream_verdict") +int prog_skb_verdict (struct __sk_buff *skb) +{ +__u32 one =1 ; + +if (skb->len > verdict_max_size) +return SK_PASS ; + +return bpf_sk_redirect_map (skb, &sock_map, one,0 ); +} + +SEC("sk_skb/stream_verdict") +int prog_skb_verdict_pass (struct __sk_buff *skb) +{ +return SK_PASS ; +} + +SEC("sk_skb/stream_parser") +int prog_skb_parser (struct __sk_buff *skb) +{ +return skb->len; +} + +SEC("sk_skb/stream_parser") +int prog_skb_parser_partial (struct __sk_buff *skb) +{ +/* agreement with the test program on a 4-byte size header +* and 6-byte body. +*/ +if (skb->len <4 ) { +/* need more header to determine full length */ +return 0 ; + } +/* return full length decoded from header. +* the return value may be larger than skb->len which +* means framework must wait body coming. +*/ +return 10 ; +} + +char _license[] SEC("license") = "GPL";