From: Cong Wang <hidden> Date: 2021-03-02 10:38:36
From: Cong Wang <redacted>
We have thousands of services connected to a daemon on every host
via AF_UNIX dgram sockets, after they are moved into VM, we have to
add a proxy to forward these communications from VM to host, because
rewriting thousands of them is not practical. This proxy uses an
AF_UNIX socket connected to services and a UDP socket to connect to
the host. It is inefficient because data is copied between kernel
space and user space twice, and we can not use splice() which only
supports TCP. Therefore, we want to use sockmap to do the splicing
without going to user-space at all (after the initial setup).
Currently sockmap only fully supports TCP, UDP is partially supported
as it is only allowed to add into sockmap. This patchset, as the second
part of the original large patchset, extends sockmap with:
1) cross-protocol support with BPF_SK_SKB_VERDICT; 2) full UDP support.
On the high level, ->sendmsg_locked() and ->read_sock() are required
for each protocol to support sockmap redirection, and in order to do
sock proto update, a new ops ->update_proto() is introduced, which is
also required to implement. A BPF ->recvmsg() is also needed to replace
the original ->recvmsg() to retrieve skmsg. Please see each patch for
more details.
To see the big picture, the original patchset is available here:
https://github.com/congwang/linux/tree/sockmap
this patchset is also available:
https://github.com/congwang/linux/tree/sockmap2
---
v2: separate from the original large patchset
rebase to the latest bpf-next
split UDP test case
move inet_csk_has_ulp() check to tcp_bpf.c
clean up udp_read_sock()
Cong Wang (9):
sock_map: introduce BPF_SK_SKB_VERDICT
sock: introduce sk_prot->update_proto()
udp: implement ->sendmsg_locked()
udp: implement ->read_sock() for sockmap
udp: add ->read_sock() and ->sendmsg_locked() to ipv6
skmsg: extract __tcp_bpf_recvmsg() and tcp_bpf_wait_data()
udp: implement udp_bpf_recvmsg() for sockmap
sock_map: update sock type checks for UDP
selftests/bpf: add a test case for udp sockmap
include/linux/skmsg.h | 25 ++--
include/net/ipv6.h | 1 +
include/net/sock.h | 3 +
include/net/tcp.h | 3 +-
include/net/udp.h | 4 +
include/uapi/linux/bpf.h | 1 +
kernel/bpf/syscall.c | 1 +
net/core/skmsg.c | 113 +++++++++++++-
net/core/sock_map.c | 52 ++++---
net/ipv4/af_inet.c | 2 +
net/ipv4/tcp_bpf.c | 129 +++-------------
net/ipv4/tcp_ipv4.c | 3 +
net/ipv4/udp.c | 68 ++++++++-
net/ipv4/udp_bpf.c | 78 +++++++++-
net/ipv6/af_inet6.c | 2 +
net/ipv6/tcp_ipv6.c | 3 +
net/ipv6/udp.c | 30 +++-
net/tls/tls_sw.c | 4 +-
tools/bpf/bpftool/common.c | 1 +
tools/bpf/bpftool/prog.c | 1 +
tools/include/uapi/linux/bpf.h | 1 +
.../selftests/bpf/prog_tests/sockmap_listen.c | 140 ++++++++++++++++++
.../selftests/bpf/progs/test_sockmap_listen.c | 22 +++
23 files changed, 517 insertions(+), 170 deletions(-)
--
2.25.1
From: Cong Wang <hidden> Date: 2021-03-02 10:38:36
From: Cong Wang <redacted>
UDP already has udp_sendmsg() which takes lock_sock() inside.
We have to build ->sendmsg_locked() on top of it, by adding
a new parameter for whether the sock has been locked.
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Lorenz Bauer <redacted>
Signed-off-by: Cong Wang <redacted>
---
include/net/udp.h | 1 +
net/ipv4/af_inet.c | 1 +
net/ipv4/udp.c | 30 +++++++++++++++++++++++-------
3 files changed, 25 insertions(+), 7 deletions(-)
@@ -1241,11 +1244,13 @@ int udp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)gotoout;}-lock_sock(sk);+if(!locked)+lock_sock(sk);if(unlikely(up->pending)){/* The socket is already corked while preparing it. *//* ... which is an evident application bug. --ANK */-release_sock(sk);+if(!locked)+release_sock(sk);net_dbg_ratelimited("socket already corked\n");err=-EINVAL;
From: Cong Wang <hidden> Date: 2021-03-02 10:38:36
From: Cong Wang <redacted>
Reusing BPF_SK_SKB_STREAM_VERDICT is possible but its name is
confusing and more importantly we still want to distinguish them
from user-space. So we can just reuse the stream verdict code but
introduce a new type of eBPF program, skb_verdict. Users are not
allowed to set stream_verdict and skb_verdict at the same time.
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Lorenz Bauer <redacted>
Signed-off-by: Cong Wang <redacted>
---
include/linux/skmsg.h | 3 +++
include/uapi/linux/bpf.h | 1 +
kernel/bpf/syscall.c | 1 +
net/core/skmsg.c | 4 +++-
net/core/sock_map.c | 23 ++++++++++++++++++++++-
tools/bpf/bpftool/common.c | 1 +
tools/bpf/bpftool/prog.c | 1 +
tools/include/uapi/linux/bpf.h | 1 +
8 files changed, 33 insertions(+), 2 deletions(-)
From: Cong Wang <hidden> Date: 2021-03-02 10:38:37
From: Cong Wang <redacted>
Similarly, udpv6_sendmsg() takes lock_sock() inside too,
we have to build ->sendmsg_locked() on top of it.
For ->read_sock(), we can just use udp_read_sock().
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Lorenz Bauer <redacted>
Signed-off-by: Cong Wang <redacted>
---
include/net/ipv6.h | 1 +
net/ipv4/udp.c | 1 +
net/ipv6/af_inet6.c | 2 ++
net/ipv6/udp.c | 27 +++++++++++++++++++++------
4 files changed, 25 insertions(+), 6 deletions(-)
@@ -1533,11 +1535,13 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)gotoout;}-lock_sock(sk);+if(!locked)+lock_sock(sk);if(unlikely(up->pending)){/* The socket is already corked while preparing it. *//* ... which is an evident application bug. --ANK */-release_sock(sk);+if(!locked)+release_sock(sk);net_dbg_ratelimited("udp cork app bug 2\n");err=-EINVAL;
From: Cong Wang <hidden> Date: 2021-03-02 10:38:37
From: Cong Wang <redacted>
Although these two functions are only used by TCP, they are not
specific to TCP at all, both operate on skmsg and ingress_msg,
so fit in net/core/skmsg.c very well.
And we will need them for non-TCP, so rename and move them to
skmsg.c and export them to modules.
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Lorenz Bauer <redacted>
Signed-off-by: Cong Wang <redacted>
---
include/linux/skmsg.h | 4 ++
include/net/tcp.h | 2 -
net/core/skmsg.c | 104 +++++++++++++++++++++++++++++++++++++++++
net/ipv4/tcp_bpf.c | 106 +-----------------------------------------
net/tls/tls_sw.c | 4 +-
5 files changed, 112 insertions(+), 108 deletions(-)
@@ -399,6 +399,110 @@ int sk_msg_memcopy_from_iter(struct sock *sk, struct iov_iter *from,}EXPORT_SYMBOL_GPL(sk_msg_memcopy_from_iter);+intsk_msg_wait_data(structsock*sk,structsk_psock*psock,intflags,+longtimeo,int*err)+{+DEFINE_WAIT_FUNC(wait,woken_wake_function);+intret=0;++if(sk->sk_shutdown&RCV_SHUTDOWN)+return1;++if(!timeo)+returnret;++add_wait_queue(sk_sleep(sk),&wait);+sk_set_bit(SOCKWQ_ASYNC_WAITDATA,sk);+ret=sk_wait_event(sk,&timeo,+!list_empty(&psock->ingress_msg)||+!skb_queue_empty(&sk->sk_receive_queue),&wait);+sk_clear_bit(SOCKWQ_ASYNC_WAITDATA,sk);+remove_wait_queue(sk_sleep(sk),&wait);+returnret;+}+EXPORT_SYMBOL_GPL(sk_msg_wait_data);++/* Receive sk_msg from psock->ingress_msg to @msg. */+intsk_msg_recvmsg(structsock*sk,structsk_psock*psock,structmsghdr*msg,+intlen,intflags)+{+structiov_iter*iter=&msg->msg_iter;+intpeek=flags&MSG_PEEK;+structsk_msg*msg_rx;+inti,copied=0;++msg_rx=list_first_entry_or_null(&psock->ingress_msg,+structsk_msg,list);++while(copied!=len){+structscatterlist*sge;++if(unlikely(!msg_rx))+break;++i=msg_rx->sg.start;+do{+structpage*page;+intcopy;++sge=sk_msg_elem(msg_rx,i);+copy=sge->length;+page=sg_page(sge);+if(copied+copy>len)+copy=len-copied;+copy=copy_page_to_iter(page,sge->offset,copy,iter);+if(!copy)+returncopied?copied:-EFAULT;++copied+=copy;+if(likely(!peek)){+sge->offset+=copy;+sge->length-=copy;+if(!msg_rx->skb)+sk_mem_uncharge(sk,copy);+msg_rx->sg.size-=copy;++if(!sge->length){+sk_msg_iter_var_next(i);+if(!msg_rx->skb)+put_page(page);+}+}else{+/* Lets not optimize peek case if copy_page_to_iter+*didn'tcopytheentirelengthletsjustbreak.+*/+if(copy!=sge->length)+returncopied;+sk_msg_iter_var_next(i);+}++if(copied==len)+break;+}while(i!=msg_rx->sg.end);++if(unlikely(peek)){+if(msg_rx==list_last_entry(&psock->ingress_msg,+structsk_msg,list))+break;+msg_rx=list_next_entry(msg_rx,list);+continue;+}++msg_rx->sg.start=i;+if(!sge->length&&msg_rx->sg.start==msg_rx->sg.end){+list_del(&msg_rx->list);+if(msg_rx->skb)+consume_skb(msg_rx->skb);+kfree(msg_rx);+}+msg_rx=list_first_entry_or_null(&psock->ingress_msg,+structsk_msg,list);+}++returncopied;+}+EXPORT_SYMBOL_GPL(sk_msg_recvmsg);+staticstructsk_msg*sk_psock_create_ingress_msg(structsock*sk,structsk_buff*skb){
@@ -10,86 +10,6 @@#include<net/inet_common.h>#include<net/tls.h>-int__tcp_bpf_recvmsg(structsock*sk,structsk_psock*psock,-structmsghdr*msg,intlen,intflags)-{-structiov_iter*iter=&msg->msg_iter;-intpeek=flags&MSG_PEEK;-structsk_msg*msg_rx;-inti,copied=0;--msg_rx=list_first_entry_or_null(&psock->ingress_msg,-structsk_msg,list);--while(copied!=len){-structscatterlist*sge;--if(unlikely(!msg_rx))-break;--i=msg_rx->sg.start;-do{-structpage*page;-intcopy;--sge=sk_msg_elem(msg_rx,i);-copy=sge->length;-page=sg_page(sge);-if(copied+copy>len)-copy=len-copied;-copy=copy_page_to_iter(page,sge->offset,copy,iter);-if(!copy)-returncopied?copied:-EFAULT;--copied+=copy;-if(likely(!peek)){-sge->offset+=copy;-sge->length-=copy;-if(!msg_rx->skb)-sk_mem_uncharge(sk,copy);-msg_rx->sg.size-=copy;--if(!sge->length){-sk_msg_iter_var_next(i);-if(!msg_rx->skb)-put_page(page);-}-}else{-/* Lets not optimize peek case if copy_page_to_iter-*didn'tcopytheentirelengthletsjustbreak.-*/-if(copy!=sge->length)-returncopied;-sk_msg_iter_var_next(i);-}--if(copied==len)-break;-}while(i!=msg_rx->sg.end);--if(unlikely(peek)){-if(msg_rx==list_last_entry(&psock->ingress_msg,-structsk_msg,list))-break;-msg_rx=list_next_entry(msg_rx,list);-continue;-}--msg_rx->sg.start=i;-if(!sge->length&&msg_rx->sg.start==msg_rx->sg.end){-list_del(&msg_rx->list);-if(msg_rx->skb)-consume_skb(msg_rx->skb);-kfree(msg_rx);-}-msg_rx=list_first_entry_or_null(&psock->ingress_msg,-structsk_msg,list);-}--returncopied;-}-EXPORT_SYMBOL_GPL(__tcp_bpf_recvmsg);-staticintbpf_tcp_ingress(structsock*sk,structsk_psock*psock,structsk_msg*msg,u32apply_bytes,intflags){
From: Cong Wang <hidden> Date: 2021-03-02 10:38:37
From: Cong Wang <redacted>
Currently sockmap calls into each protocol to update the struct
proto and replace it. This certainly won't work when the protocol
is implemented as a module, for example, AF_UNIX.
Introduce a new ops sk->sk_prot->update_proto(), so each protocol
can implement its own way to replace the struct proto. This also
helps get rid of symbol dependencies on CONFIG_INET.
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Lorenz Bauer <redacted>
Signed-off-by: Cong Wang <redacted>
---
include/linux/skmsg.h | 18 +++---------------
include/net/sock.h | 3 +++
include/net/tcp.h | 1 +
include/net/udp.h | 1 +
net/core/skmsg.c | 5 -----
net/core/sock_map.c | 24 ++++--------------------
net/ipv4/tcp_bpf.c | 23 ++++++++++++++++++++---
net/ipv4/tcp_ipv4.c | 3 +++
net/ipv4/udp.c | 3 +++
net/ipv4/udp_bpf.c | 14 ++++++++++++--
net/ipv6/tcp_ipv6.c | 3 +++
net/ipv6/udp.c | 3 +++
12 files changed, 56 insertions(+), 45 deletions(-)
@@ -1184,6 +1184,9 @@ struct proto {void(*unhash)(structsock*sk);void(*rehash)(structsock*sk);int(*get_port)(structsock*sk,unsignedshortsnum);+#ifdef CONFIG_BPF_SYSCALL+int(*update_proto)(structsock*sk,boolrestore);+#endif/* Keeping track of sockets in use */#ifdef CONFIG_PROC_FS
@@ -601,19 +601,36 @@ static int tcp_bpf_assert_proto_ops(struct proto *ops)ops->sendpage==tcp_sendpage?0:-ENOTSUPP;}-structproto*tcp_bpf_get_proto(structsock*sk,structsk_psock*psock)+inttcp_bpf_update_proto(structsock*sk,boolrestore){+structsk_psock*psock=sk_psock(sk);intfamily=sk->sk_family==AF_INET6?TCP_BPF_IPV6:TCP_BPF_IPV4;intconfig=psock->progs.msg_parser?TCP_BPF_TX:TCP_BPF_BASE;+if(restore){+if(inet_csk_has_ulp(sk)){+tcp_update_ulp(sk,psock->sk_proto,psock->saved_write_space);+}else{+sk->sk_write_space=psock->saved_write_space;+/* Pairs with lockless read in sk_clone_lock() */+WRITE_ONCE(sk->sk_prot,psock->sk_proto);+}+return0;+}++if(inet_csk_has_ulp(sk))+return-EINVAL;+if(sk->sk_family==AF_INET6){if(tcp_bpf_assert_proto_ops(psock->sk_proto))-returnERR_PTR(-EINVAL);+return-EINVAL;tcp_bpf_check_v6_needs_rebuild(psock->sk_proto);}-return&tcp_bpf_prots[family][config];+/* Pairs with lockless read in sk_clone_lock() */+WRITE_ONCE(sk->sk_prot,&tcp_bpf_prots[family][config]);+return0;}/* If a child got cloned from a listening socket that had tcp_bpf
@@ -41,12 +41,22 @@ static int __init udp_bpf_v4_build_proto(void)}core_initcall(udp_bpf_v4_build_proto);-structproto*udp_bpf_get_proto(structsock*sk,structsk_psock*psock)+intudp_bpf_update_proto(structsock*sk,boolrestore){intfamily=sk->sk_family==AF_INET?UDP_BPF_IPV4:UDP_BPF_IPV6;+structsk_psock*psock=sk_psock(sk);++if(restore){+sk->sk_write_space=psock->saved_write_space;+/* Pairs with lockless read in sk_clone_lock() */+WRITE_ONCE(sk->sk_prot,psock->sk_proto);+return0;+}if(sk->sk_family==AF_INET6)udp_bpf_check_v6_needs_rebuild(psock->sk_proto);-return&udp_bpf_prots[family];+/* Pairs with lockless read in sk_clone_lock() */+WRITE_ONCE(sk->sk_prot,&udp_bpf_prots[family]);+return0;}
From: Cong Wang <hidden> Date: 2021-03-02 10:38:37
From: Cong Wang <redacted>
We have to implement udp_bpf_recvmsg() to replace the ->recvmsg()
to retrieve skmsg from ingress_msg.
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Lorenz Bauer <redacted>
Signed-off-by: Cong Wang <redacted>
---
net/ipv4/udp_bpf.c | 64 +++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 63 insertions(+), 1 deletion(-)
@@ -330,6 +330,8 @@ struct sock *__udp6_lib_lookup(struct net *net,structsk_buff*skb);structsock*udp6_lib_lookup_skb(conststructsk_buff*skb,__be16sport,__be16dport);+intudp_read_sock(structsock*sk,read_descriptor_t*desc,+sk_read_actor_trecv_actor);/* UDP uses skb->dev_scratch to cache as much information as possible and avoid*possiblymultiplecachemissondequeue()
From: Cong Wang <hidden> Date: 2021-03-02 10:38:38
From: Cong Wang <redacted>
Add a test case to ensure redirection between two UDP sockets work.
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Lorenz Bauer <redacted>
Signed-off-by: Cong Wang <redacted>
---
.../selftests/bpf/prog_tests/sockmap_listen.c | 140 ++++++++++++++++++
.../selftests/bpf/progs/test_sockmap_listen.c | 22 +++
2 files changed, 162 insertions(+)
From: Cong Wang <hidden> Date: 2021-03-02 10:38:38
From: Cong Wang <redacted>
Now UDP supports sockmap and redirection, we can safely update
the sock type checks for it accordingly.
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Lorenz Bauer <redacted>
Signed-off-by: Cong Wang <redacted>
---
net/core/sock_map.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
@@ -1831,6 +1831,7 @@ int udp_read_sock(struct sock *sk, read_descriptor_t *desc,returncopied;}+EXPORT_SYMBOL(udp_read_sock);
Should this be in the previous commit?
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
On Tue, 2 Mar 2021 at 02:38, Cong Wang [off-list ref] wrote:
From: Cong Wang <redacted>
Add a test case to ensure redirection between two UDP sockets work.
I basically don't understand how splicing works, but watching from the
sidelines makes me think it'd be good to have more thorough tests.
tools/testing/selftests/bpf/test_sockmap.c has quite elaborate tests
for the TCP part, it'd be nice to get similar tests going for UDP. For
example:
* sendfile?
* sendmmsg
* Something Jakub mentioned: what happens when a connected, spliced
socket is disconnected via connect(AF_UNSPEC)? Seems like we don't
hook sk_prot->disconnect anywhere.
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
Not related to your patch set, but why do an extra restore of
sk_prot->unhash here? At this point sk->sk_prot is one of our tcp_bpf
/ udp_bpf protos, so overwriting that seems wrong?
@@ -1184,6 +1184,9 @@ struct proto {void(*unhash)(structsock*sk);void(*rehash)(structsock*sk);int(*get_port)(structsock*sk,unsignedshortsnum);+#ifdef CONFIG_BPF_SYSCALL+int(*update_proto)(structsock*sk,boolrestore);
Kind of a nit, but this name suggests that the callback is a lot more
generic than it really is. The only thing you can use it for is to
prep the socket to be sockmap ready since we hardwire sockmap_unhash,
etc. It's also not at all clear that this only works if sk has an
sk_psock associated with it. Calling it without one would crash the
kernel since the update_proto functions don't check for !sk_psock.
Might as well call it install_sockmap_hooks or something and have a
valid sk_psock be passed in to the callback. Additionally, I'd prefer
if the function returned a struct proto * like it does at the moment.
That way we keep sk->sk_prot manipulation confined to the sockmap code
and don't have to copy paste it into every proto.
I think reads / writes from sk_prot need READ_ONCE / WRITE_ONCE. We've
not been diligent about this so far, but I think it makes sense to be
careful in new code.
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
@@ -1831,6 +1831,7 @@ int udp_read_sock(struct sock *sk, read_descriptor_t *desc,returncopied;}+EXPORT_SYMBOL(udp_read_sock);
Should this be in the previous commit?
No, exporting this symbol is unnecessary until a module starts to
use it, which is IPv6 module in this patch. So, it is perfectly fine to
export it here.
Thanks.
From: Cong Wang <hidden> Date: 2021-03-03 03:56:14
On Tue, Mar 2, 2021 at 8:32 AM Lorenz Bauer [off-list ref] wrote:
On Tue, 2 Mar 2021 at 02:38, Cong Wang [off-list ref] wrote:
quoted
From: Cong Wang <redacted>
Add a test case to ensure redirection between two UDP sockets work.
I basically don't understand how splicing works, but watching from the
sidelines makes me think it'd be good to have more thorough tests.
tools/testing/selftests/bpf/test_sockmap.c has quite elaborate tests
for the TCP part, it'd be nice to get similar tests going for UDP. For
Sure, TCP supports more than just BPF_SK_SKB_VERDICT, hence
why it must have more tests than UDP. ;)
example:
* sendfile?
* sendmmsg
Does UDP support any of these? I don't think so, at least not in my
patchset.
* Something Jakub mentioned: what happens when a connected, spliced
socket is disconnected via connect(AF_UNSPEC)? Seems like we don't
hook sk_prot->disconnect anywhere.
Not related to your patch set, but why do an extra restore of
sk_prot->unhash here? At this point sk->sk_prot is one of our tcp_bpf
/ udp_bpf protos, so overwriting that seems wrong?
Good catch. It seems you are right, but I need a double check. And
yes, it is completely unrelated to my patch, as the current code has
the same problem.
@@ -1184,6 +1184,9 @@ struct proto {void(*unhash)(structsock*sk);void(*rehash)(structsock*sk);int(*get_port)(structsock*sk,unsignedshortsnum);+#ifdef CONFIG_BPF_SYSCALL+int(*update_proto)(structsock*sk,boolrestore);
Kind of a nit, but this name suggests that the callback is a lot more
generic than it really is. The only thing you can use it for is to
prep the socket to be sockmap ready since we hardwire sockmap_unhash,
etc. It's also not at all clear that this only works if sk has an
sk_psock associated with it. Calling it without one would crash the
kernel since the update_proto functions don't check for !sk_psock.
Might as well call it install_sockmap_hooks or something and have a
valid sk_psock be passed in to the callback. Additionally, I'd prefer
For the name, sure, I am always open to better names. Not sure if
'install_sockmap_hooks' is a good name, I also want to express we
are overriding sk_prot. How about 'psock_update_sk_prot'?
if the function returned a struct proto * like it does at the moment.
That way we keep sk->sk_prot manipulation confined to the sockmap code
and don't have to copy paste it into every proto.
Well, TCP seems too special to do this, as it could call tcp_update_ulp()
to update the proto.
I think reads / writes from sk_prot need READ_ONCE / WRITE_ONCE. We've
not been diligent about this so far, but I think it makes sense to be
careful in new code.
Hmm, there are many places not using READ_ONCE/WRITE_ONCE,
for a quick example:
void sock_map_unhash(struct sock *sk)
{
void (*saved_unhash)(struct sock *sk);
struct sk_psock *psock;
rcu_read_lock();
psock = sk_psock(sk);
if (unlikely(!psock)) {
rcu_read_unlock();
if (sk->sk_prot->unhash)
sk->sk_prot->unhash(sk);
return;
}
saved_unhash = psock->saved_unhash;
sock_map_remove_links(sk, psock);
rcu_read_unlock();
saved_unhash(sk);
}
Thanks.
@@ -330,6 +330,8 @@ struct sock *__udp6_lib_lookup(struct net *net,structsk_buff*skb);structsock*udp6_lib_lookup_skb(conststructsk_buff*skb,__be16sport,__be16dport);+intudp_read_sock(structsock*sk,read_descriptor_t*desc,+sk_read_actor_trecv_actor);/* UDP uses skb->dev_scratch to cache as much information as possible and avoid*possiblymultiplecachemissondequeue()
From: Yonghong Song <hidden> Date: 2021-03-03 22:55:56
On 3/1/21 6:37 PM, Cong Wang wrote:
quoted hunk
From: Cong Wang <redacted>
Now UDP supports sockmap and redirection, we can safely update
the sock type checks for it accordingly.
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Lorenz Bauer <redacted>
Signed-off-by: Cong Wang <redacted>
---
net/core/sock_map.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Not a networking expert, a dump question. Here we tested
whether sk_is_tcp(sk) or not, if not we compare
sk->sk_state == TCP_ESTABLISHED, could this be
always false? Mostly I missed something, some comments
here will be good.
On Tue, 2 Mar 2021 at 18:23, Cong Wang [off-list ref] wrote:
quoted
if the function returned a struct proto * like it does at the moment.
That way we keep sk->sk_prot manipulation confined to the sockmap code
and don't have to copy paste it into every proto.
Well, TCP seems too special to do this, as it could call tcp_update_ulp()
to update the proto.
I had a quick look, tcp_bpf_update_proto is the only caller of tcp_update_ulp,
which in turn is the only caller of icsk_ulp_ops->update, which in turn is only
implemented as tls_update in tls_main.c. Turns out that tls_update
has another one of these calls:
} else {
/* Pairs with lockless read in sk_clone_lock(). */
WRITE_ONCE(sk->sk_prot, p);
sk->sk_write_space = write_space;
}
Maybe it looks familiar? :o) I think it would be a worthwhile change.
I think reads / writes from sk_prot need READ_ONCE / WRITE_ONCE. We've
not been diligent about this so far, but I think it makes sense to be
careful in new code.
Hmm, there are many places not using READ_ONCE/WRITE_ONCE,
for a quick example:
I know! I'll defer to John and Jakub.
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
On Tue, 2 Mar 2021 at 18:05, Cong Wang [off-list ref] wrote:
On Tue, Mar 2, 2021 at 8:32 AM Lorenz Bauer [off-list ref] wrote:
quoted
On Tue, 2 Mar 2021 at 02:38, Cong Wang [off-list ref] wrote:
quoted
From: Cong Wang <redacted>
Add a test case to ensure redirection between two UDP sockets work.
I basically don't understand how splicing works, but watching from the
sidelines makes me think it'd be good to have more thorough tests.
tools/testing/selftests/bpf/test_sockmap.c has quite elaborate tests
for the TCP part, it'd be nice to get similar tests going for UDP. For
Sure, TCP supports more than just BPF_SK_SKB_VERDICT, hence
why it must have more tests than UDP. ;)
quoted
example:
* sendfile?
* sendmmsg
Does UDP support any of these? I don't think so, at least not in my
patchset.
I have no idea, thanks for checking :)
quoted
* Something Jakub mentioned: what happens when a connected, spliced
socket is disconnected via connect(AF_UNSPEC)? Seems like we don't
hook sk_prot->disconnect anywhere.
But we hook ->unhash(), right?
I wasn't aware that ->disconnect calls unhash, thanks!
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
From: Cong Wang <hidden> Date: 2021-03-03 22:57:33
On Tue, Mar 2, 2021 at 10:37 PM Yonghong Song [off-list ref] wrote:
On 3/1/21 6:37 PM, Cong Wang wrote:
quoted
From: Cong Wang <redacted>
Now UDP supports sockmap and redirection, we can safely update
the sock type checks for it accordingly.
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Lorenz Bauer <redacted>
Signed-off-by: Cong Wang <redacted>
---
net/core/sock_map.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Not a networking expert, a dump question. Here we tested
whether sk_is_tcp(sk) or not, if not we compare
sk->sk_state == TCP_ESTABLISHED, could this be
always false? Mostly I missed something, some comments
here will be good.
No, dgram sockets also use TCP_ESTABLISHED as a valid
state. I know its name looks confusing, but it is already widely
used in networking:
net/appletalk/ddp.c: sk->sk_state = TCP_ESTABLISHED;
net/appletalk/ddp.c: if (sk->sk_state != TCP_ESTABLISHED)
net/appletalk/ddp.c: if (sk->sk_state != TCP_ESTABLISHED)
net/ax25/af_ax25.c: sk->sk_state = TCP_ESTABLISHED;
net/ax25/af_ax25.c: case TCP_ESTABLISHED: /* connection
established */
net/ax25/af_ax25.c: if (sk->sk_state == TCP_ESTABLISHED &&
sk->sk_type == SOCK_SEQPACKET) {
net/ax25/af_ax25.c: sk->sk_state = TCP_ESTABLISHED;
net/ax25/af_ax25.c: if (sk->sk_state != TCP_ESTABLISHED && (flags
& O_NONBLOCK)) {
net/ax25/af_ax25.c: if (sk->sk_state != TCP_ESTABLISHED) {
net/ax25/af_ax25.c: if (sk->sk_state != TCP_ESTABLISHED) {
net/ax25/af_ax25.c: if (sk->sk_state != TCP_ESTABLISHED) {
net/ax25/af_ax25.c: if (sk->sk_state != TCP_ESTABLISHED) {
net/ax25/af_ax25.c: if (sk->sk_type == SOCK_SEQPACKET &&
sk->sk_state != TCP_ESTABLISHED) {
net/ax25/ax25_ds_in.c: ax25->sk->sk_state = TCP_ESTABLISHED;
net/ax25/ax25_in.c: make->sk_state = TCP_ESTABLISHED;
net/ax25/ax25_std_in.c: ax25->sk->sk_state =
TCP_ESTABLISHED;
net/caif/caif_socket.c: CAIF_CONNECTED = TCP_ESTABLISHED,
net/ceph/messenger.c: case TCP_ESTABLISHED:
net/ceph/messenger.c: dout("%s TCP_ESTABLISHED\n", __func__);
net/core/datagram.c: !(sk->sk_state == TCP_ESTABLISHED ||
sk->sk_state == TCP_LISTEN))
...
Hence, I believe it is okay to use it as it is, otherwise we would need
to comment on every use of it. ;)
Thanks.
From: Cong Wang <hidden> Date: 2021-03-03 22:57:34
On Wed, Mar 3, 2021 at 1:35 AM Lorenz Bauer [off-list ref] wrote:
On Tue, 2 Mar 2021 at 18:23, Cong Wang [off-list ref] wrote:
quoted
quoted
if the function returned a struct proto * like it does at the moment.
That way we keep sk->sk_prot manipulation confined to the sockmap code
and don't have to copy paste it into every proto.
Well, TCP seems too special to do this, as it could call tcp_update_ulp()
to update the proto.
I had a quick look, tcp_bpf_update_proto is the only caller of tcp_update_ulp,
which in turn is the only caller of icsk_ulp_ops->update, which in turn is only
implemented as tls_update in tls_main.c. Turns out that tls_update
has another one of these calls:
} else {
/* Pairs with lockless read in sk_clone_lock(). */
WRITE_ONCE(sk->sk_prot, p);
sk->sk_write_space = write_space;
}
Maybe it looks familiar? :o) I think it would be a worthwhile change.
Yeah, I am not surprised we can change tcp_update_ulp() too, but
why should I bother kTLS when I do not have to? What you suggest
could at most save us a bit of code size, not a big gain. So, I'd keep
its return value as it is, unless you see any other benefits.
BTW, I will rename it to 'psock_update_sk_prot', please let me know
if you have any better names.
Thanks.
From: Yonghong Song <hidden> Date: 2021-03-03 22:57:34
On 3/3/21 10:02 AM, Cong Wang wrote:
On Tue, Mar 2, 2021 at 10:37 PM Yonghong Song [off-list ref] wrote:
quoted
On 3/1/21 6:37 PM, Cong Wang wrote:
quoted
From: Cong Wang <redacted>
Now UDP supports sockmap and redirection, we can safely update
the sock type checks for it accordingly.
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Lorenz Bauer <redacted>
Signed-off-by: Cong Wang <redacted>
---
net/core/sock_map.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Not a networking expert, a dump question. Here we tested
whether sk_is_tcp(sk) or not, if not we compare
sk->sk_state == TCP_ESTABLISHED, could this be
always false? Mostly I missed something, some comments
here will be good.
No, dgram sockets also use TCP_ESTABLISHED as a valid
state. I know its name looks confusing, but it is already widely
used in networking:
net/appletalk/ddp.c: sk->sk_state = TCP_ESTABLISHED;
net/appletalk/ddp.c: if (sk->sk_state != TCP_ESTABLISHED)
net/appletalk/ddp.c: if (sk->sk_state != TCP_ESTABLISHED)
net/ax25/af_ax25.c: sk->sk_state = TCP_ESTABLISHED;
net/ax25/af_ax25.c: case TCP_ESTABLISHED: /* connection
established */
net/ax25/af_ax25.c: if (sk->sk_state == TCP_ESTABLISHED &&
sk->sk_type == SOCK_SEQPACKET) {
net/ax25/af_ax25.c: sk->sk_state = TCP_ESTABLISHED;
net/ax25/af_ax25.c: if (sk->sk_state != TCP_ESTABLISHED && (flags
& O_NONBLOCK)) {
net/ax25/af_ax25.c: if (sk->sk_state != TCP_ESTABLISHED) {
net/ax25/af_ax25.c: if (sk->sk_state != TCP_ESTABLISHED) {
net/ax25/af_ax25.c: if (sk->sk_state != TCP_ESTABLISHED) {
net/ax25/af_ax25.c: if (sk->sk_state != TCP_ESTABLISHED) {
net/ax25/af_ax25.c: if (sk->sk_type == SOCK_SEQPACKET &&
sk->sk_state != TCP_ESTABLISHED) {
net/ax25/ax25_ds_in.c: ax25->sk->sk_state = TCP_ESTABLISHED;
net/ax25/ax25_in.c: make->sk_state = TCP_ESTABLISHED;
net/ax25/ax25_std_in.c: ax25->sk->sk_state =
TCP_ESTABLISHED;
net/caif/caif_socket.c: CAIF_CONNECTED = TCP_ESTABLISHED,
net/ceph/messenger.c: case TCP_ESTABLISHED:
net/ceph/messenger.c: dout("%s TCP_ESTABLISHED\n", __func__);
net/core/datagram.c: !(sk->sk_state == TCP_ESTABLISHED ||
sk->sk_state == TCP_LISTEN))
...
Hence, I believe it is okay to use it as it is, otherwise we would need
to comment on every use of it. ;)
On Wed, 3 Mar 2021 at 18:21, Cong Wang [off-list ref] wrote:
Yeah, I am not surprised we can change tcp_update_ulp() too, but
why should I bother kTLS when I do not have to? What you suggest
could at most save us a bit of code size, not a big gain. So, I'd keep
its return value as it is, unless you see any other benefits.
I think the end result is code that is easier to understand and
therefore maintain. Keep it as it is if you prefer.
BTW, I will rename it to 'psock_update_sk_prot', please let me know
if you have any better names.
SGTM.
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
Not related to your patch set, but why do an extra restore of
sk_prot->unhash here? At this point sk->sk_prot is one of our tcp_bpf
/ udp_bpf protos, so overwriting that seems wrong?
Good catch. It seems you are right, but I need a double check. And
yes, it is completely unrelated to my patch, as the current code has
the same problem.
Looking at this again. I noticed
commit 4da6a196f93b1af7612340e8c1ad8ce71e18f955
Author: John Fastabend [off-list ref]
Date: Sat Jan 11 06:11:59 2020 +0000
bpf: Sockmap/tls, during free we may call tcp_bpf_unhash() in loop
intentionally fixed a bug in kTLS with overwriting this ->unhash.
I agree with you that it should not be updated for sockmap case,
however I don't know what to do with kTLS case, it seems the bug the
above commit fixed still exists if we just revert it.
Anyway, this should be targeted for -bpf as a bug fix, so it does not
belong to this patchset.
Thanks.
Not related to your patch set, but why do an extra restore of
sk_prot->unhash here? At this point sk->sk_prot is one of our tcp_bpf
/ udp_bpf protos, so overwriting that seems wrong?
"extra"? restore_proto should only be called when the psock ref count
is zero and we need to transition back to the original socks proto
handlers. To trigger this we can simply delete a sock from the map.
In the case where we are deleting the psock overwriting the tcp_bpf
protos is exactly what we want.?
quoted
Good catch. It seems you are right, but I need a double check. And
yes, it is completely unrelated to my patch, as the current code has
the same problem.
Looking at this again. I noticed
commit 4da6a196f93b1af7612340e8c1ad8ce71e18f955
Author: John Fastabend [off-list ref]
Date: Sat Jan 11 06:11:59 2020 +0000
bpf: Sockmap/tls, during free we may call tcp_bpf_unhash() in loop
intentionally fixed a bug in kTLS with overwriting this ->unhash.
I agree with you that it should not be updated for sockmap case,
however I don't know what to do with kTLS case, it seems the bug the
above commit fixed still exists if we just revert it.
Anyway, this should be targeted for -bpf as a bug fix, so it does not
belong to this patchset.
Thanks.
Hi,
I'm missing the error case here. The restore logic happens when the refcnt
hits 0 on the psock, indicating its time to garbage collect the psock.
sk_psock_put
if (refcount_dec_and_test(&psock->refcnt))
sk_psock_drop(sk, psock);
sk_psock_restore_proto(sk, psock)
sk->sk_prot->unhash = psock->saved_unhash
When sockets are initialized via sk_psock_init() we opulate the unhash field
psock->saved_unhash = prot->unhash;
So we need to unwind this otherwise a future unhash() call would not call
the original protos unhash handler.
Care to give me some more context on what the bug is?
Thanks,
John
Not related to your patch set, but why do an extra restore of
sk_prot->unhash here? At this point sk->sk_prot is one of our tcp_bpf
/ udp_bpf protos, so overwriting that seems wrong?
"extra"? restore_proto should only be called when the psock ref count
is zero and we need to transition back to the original socks proto
handlers. To trigger this we can simply delete a sock from the map.
In the case where we are deleting the psock overwriting the tcp_bpf
protos is exactly what we want.?
Why do you want to overwrite tcp_bpf_prots->unhash? Overwriting
tcp_bpf_prots is correct, but overwriting tcp_bpf_prots->unhash is not.
Because once you overwrite it, the next time you use it to replace
sk->sk_prot, it would be a different one rather than sock_map_unhash():
// tcp_bpf_prots->unhash == sock_map_unhash
sk_psock_restore_proto();
// Now tcp_bpf_prots->unhash is inet_unhash
...
sk_psock_update_proto();
// sk->sk_proto is now tcp_bpf_prots again,
// so its ->unhash now is inet_unhash
// but it should be sock_map_unhash here
Thanks.
Not related to your patch set, but why do an extra restore of
sk_prot->unhash here? At this point sk->sk_prot is one of our tcp_bpf
/ udp_bpf protos, so overwriting that seems wrong?
"extra"? restore_proto should only be called when the psock ref count
is zero and we need to transition back to the original socks proto
handlers. To trigger this we can simply delete a sock from the map.
In the case where we are deleting the psock overwriting the tcp_bpf
protos is exactly what we want.?
Why do you want to overwrite tcp_bpf_prots->unhash? Overwriting
tcp_bpf_prots is correct, but overwriting tcp_bpf_prots->unhash is not.
Because once you overwrite it, the next time you use it to replace
sk->sk_prot, it would be a different one rather than sock_map_unhash():
// tcp_bpf_prots->unhash == sock_map_unhash
sk_psock_restore_proto();
// Now tcp_bpf_prots->unhash is inet_unhash
...
sk_psock_update_proto();
// sk->sk_proto is now tcp_bpf_prots again,
// so its ->unhash now is inet_unhash
// but it should be sock_map_unhash here
Right, we can fix this on the TLS side. I'll push a fix shortly.
Not related to your patch set, but why do an extra restore of
sk_prot->unhash here? At this point sk->sk_prot is one of our tcp_bpf
/ udp_bpf protos, so overwriting that seems wrong?
"extra"? restore_proto should only be called when the psock ref count
is zero and we need to transition back to the original socks proto
handlers. To trigger this we can simply delete a sock from the map.
In the case where we are deleting the psock overwriting the tcp_bpf
protos is exactly what we want.?
Why do you want to overwrite tcp_bpf_prots->unhash? Overwriting
tcp_bpf_prots is correct, but overwriting tcp_bpf_prots->unhash is not.
Because once you overwrite it, the next time you use it to replace
sk->sk_prot, it would be a different one rather than sock_map_unhash():
// tcp_bpf_prots->unhash == sock_map_unhash
sk_psock_restore_proto();
// Now tcp_bpf_prots->unhash is inet_unhash
...
sk_psock_update_proto();
// sk->sk_proto is now tcp_bpf_prots again,
// so its ->unhash now is inet_unhash
// but it should be sock_map_unhash here
Right, we can fix this on the TLS side. I'll push a fix shortly.
Are you still working on this? If kTLS still needs it, then we can
have something like this:
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-03-10 06:34:37
Cong Wang wrote:
On Fri, Mar 5, 2021 at 5:55 PM John Fastabend [off-list ref] wrote:
quoted
[...]
quoted
quoted
// tcp_bpf_prots->unhash == sock_map_unhash
sk_psock_restore_proto();
// Now tcp_bpf_prots->unhash is inet_unhash
...
sk_psock_update_proto();
// sk->sk_proto is now tcp_bpf_prots again,
// so its ->unhash now is inet_unhash
// but it should be sock_map_unhash here
Right, we can fix this on the TLS side. I'll push a fix shortly.
Are you still working on this? If kTLS still needs it, then we can
have something like this:
Testing a fix now I will flush it out tomorrow. The below is not
really correct either it just moves the issue so it only impacts
TLS.