From: Cong Wang <hidden> Date: 2021-03-28 20:21:20
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, ->read_sock() is required for each protocol to support
sockmap redirection, and in order to do sock proto update, a new ops
->psock_update_sk_prot() is introduced, which is also required. And the
BPF ->recvmsg() is also needed to replace the original ->recvmsg() to
retrieve skmsg. To make life easier, we have to get rid of lock_sock()
in sk_psock_handle_skb(), otherwise we would have to implement
->sendmsg_locked() on top of ->sendmsg(), which is ugly.
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
---
v7: use work_mutex to protect psock->work
return err in udp_read_sock()
add patch 6/13
clean up test case
v6: get rid of sk_psock_zap_ingress()
add rcu work patch
v5: use INDIRECT_CALL_2() for function pointers
use ingress_lock to fix a race condition found by Jacub
rename two helper functions
v4: get rid of lock_sock() in sk_psock_handle_skb()
get rid of udp_sendmsg_locked()
remove an empty line
update cover letter
v3: export tcp/udp_update_proto()
rename sk->sk_prot->psock_update_sk_prot()
improve changelogs
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 (13):
skmsg: lock ingress_skb when purging
skmsg: introduce a spinlock to protect ingress_msg
net: introduce skb_send_sock() for sock_map
skmsg: avoid lock_sock() in sk_psock_backlog()
skmsg: use rcu work for destroying psock
skmsg: use GFP_KERNEL in sk_psock_create_ingress_msg()
sock_map: introduce BPF_SK_SKB_VERDICT
sock: introduce sk->sk_prot->psock_update_sk_prot()
udp: implement ->read_sock() for sockmap
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/skbuff.h | 1 +
include/linux/skmsg.h | 77 ++++++--
include/net/sock.h | 3 +
include/net/tcp.h | 3 +-
include/net/udp.h | 3 +
include/uapi/linux/bpf.h | 1 +
kernel/bpf/syscall.c | 1 +
net/core/skbuff.c | 55 +++++-
net/core/skmsg.c | 177 ++++++++++++++----
net/core/sock_map.c | 53 +++---
net/ipv4/af_inet.c | 1 +
net/ipv4/tcp_bpf.c | 130 +++----------
net/ipv4/tcp_ipv4.c | 3 +
net/ipv4/udp.c | 38 ++++
net/ipv4/udp_bpf.c | 79 +++++++-
net/ipv6/af_inet6.c | 1 +
net/ipv6/tcp_ipv6.c | 3 +
net/ipv6/udp.c | 3 +
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 | 136 ++++++++++++++
.../selftests/bpf/progs/test_sockmap_listen.c | 22 +++
24 files changed, 601 insertions(+), 196 deletions(-)
--
2.25.1
From: Cong Wang <hidden> Date: 2021-03-28 20:21:20
From: Cong Wang <redacted>
We only have skb_send_sock_locked() which requires callers
to use lock_sock(). Introduce a variant skb_send_sock()
which locks on its own, callers do not need to lock it
any more. This will save us from adding a ->sendmsg_locked
for each protocol.
To reuse the code, pass function pointers to __skb_send_sock()
and build skb_send_sock() and skb_send_sock_locked() on top.
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/skbuff.h | 1 +
net/core/skbuff.c | 55 ++++++++++++++++++++++++++++++++++++------
2 files changed, 49 insertions(+), 7 deletions(-)
@@ -2500,9 +2500,32 @@ int skb_splice_bits(struct sk_buff *skb, struct sock *sk, unsigned int offset,}EXPORT_SYMBOL_GPL(skb_splice_bits);-/* Send skb data on a socket. Socket must be locked. */-intskb_send_sock_locked(structsock*sk,structsk_buff*skb,intoffset,-intlen)+staticintsendmsg_unlocked(structsock*sk,structmsghdr*msg,+structkvec*vec,size_tnum,size_tsize)+{+structsocket*sock=sk->sk_socket;++if(!sock)+return-EINVAL;+returnkernel_sendmsg(sock,msg,vec,num,size);+}++staticintsendpage_unlocked(structsock*sk,structpage*page,intoffset,+size_tsize,intflags)+{+structsocket*sock=sk->sk_socket;++if(!sock)+return-EINVAL;+returnkernel_sendpage(sock,page,offset,size,flags);+}++typedefint(*sendmsg_func)(structsock*sk,structmsghdr*msg,+structkvec*vec,size_tnum,size_tsize);+typedefint(*sendpage_func)(structsock*sk,structpage*page,intoffset,+size_tsize,intflags);+staticint__skb_send_sock(structsock*sk,structsk_buff*skb,intoffset,+intlen,sendmsg_funcsendmsg,sendpage_funcsendpage){unsignedintorig_len=len;structsk_buff*head=skb;
@@ -2522,7 +2545,8 @@ int skb_send_sock_locked(struct sock *sk, struct sk_buff *skb, int offset,memset(&msg,0,sizeof(msg));msg.msg_flags=MSG_DONTWAIT;-ret=kernel_sendmsg_locked(sk,&msg,&kv,1,slen);+ret=INDIRECT_CALL_2(sendmsg,kernel_sendmsg_locked,+sendmsg_unlocked,sk,&msg,&kv,1,slen);if(ret<=0)gotoerror;
@@ -2553,9 +2577,11 @@ int skb_send_sock_locked(struct sock *sk, struct sk_buff *skb, int offset,slen=min_t(size_t,len,skb_frag_size(frag)-offset);while(slen){-ret=kernel_sendpage_locked(sk,skb_frag_page(frag),-skb_frag_off(frag)+offset,-slen,MSG_DONTWAIT);+ret=INDIRECT_CALL_2(sendpage,kernel_sendpage_locked,+sendpage_unlocked,sk,+skb_frag_page(frag),+skb_frag_off(frag)+offset,+slen,MSG_DONTWAIT);if(ret<=0)gotoerror;
@@ -2587,8 +2613,23 @@ int skb_send_sock_locked(struct sock *sk, struct sk_buff *skb, int offset,error:returnorig_len==len?ret:orig_len-len;}++/* Send skb data on a socket. Socket must be locked. */+intskb_send_sock_locked(structsock*sk,structsk_buff*skb,intoffset,+intlen)+{+return__skb_send_sock(sk,skb,offset,len,kernel_sendmsg_locked,+kernel_sendpage_locked);+}EXPORT_SYMBOL_GPL(skb_send_sock_locked);+/* Send skb data on a socket. Socket must be unlocked. */+intskb_send_sock(structsock*sk,structsk_buff*skb,intoffset,intlen)+{+return__skb_send_sock(sk,skb,offset,len,sendmsg_unlocked,+sendpage_unlocked);+}+/***skb_store_bits-storebitsfromkernelbuffertoskb*@skb:destinationbuffer
From: Cong Wang <hidden> Date: 2021-03-28 20:21:20
From: Cong Wang <redacted>
Currently we rely on lock_sock to protect ingress_msg,
it is too big for this, we can actually just use a spinlock
to protect this list like protecting other skb queues.
__tcp_bpf_recvmsg() is still special because of peeking,
it still has to use lock_sock.
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Lorenz Bauer <redacted>
Acked-by: Jakub Sitnicki <jakub@cloudflare.com>
Signed-off-by: Cong Wang <redacted>
---
include/linux/skmsg.h | 46 +++++++++++++++++++++++++++++++++++++++++++
net/core/skmsg.c | 3 +++
net/ipv4/tcp_bpf.c | 18 ++++++-----------
3 files changed, 55 insertions(+), 12 deletions(-)
From: Cong Wang <hidden> Date: 2021-03-28 20:21:20
From: Cong Wang <redacted>
Currently we purge the ingress_skb queue only when psock
refcnt goes down to 0, so locking the queue is not necessary,
but in order to be called during ->close, we have to lock it
here.
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Lorenz Bauer <redacted>
Acked-by: Jakub Sitnicki <jakub@cloudflare.com>
Signed-off-by: Cong Wang <redacted>
---
net/core/skmsg.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Cong Wang <hidden> Date: 2021-03-28 20:21:20
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 | 2 ++
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, 32 insertions(+), 2 deletions(-)
From: Cong Wang <hidden> Date: 2021-03-28 20:21:20
From: Cong Wang <redacted>
The RCU callback sk_psock_destroy() only queues work psock->gc,
so we can just switch to rcu work to simplify the code.
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Lorenz Bauer <redacted>
Acked-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Cong Wang <redacted>
---
include/linux/skmsg.h | 5 +----
net/core/skmsg.c | 17 +++++------------
2 files changed, 6 insertions(+), 16 deletions(-)
From: Cong Wang <hidden> Date: 2021-03-28 20:21:20
From: Cong Wang <redacted>
This function is only called in process context.
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/skmsg.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Cong Wang <hidden> Date: 2021-03-28 20:21:20
From: Cong Wang <redacted>
We do not have to lock the sock to avoid losing sk_socket,
instead we can purge all the ingress queues when we close
the socket. Sending or receiving packets after orphaning
socket makes no sense.
We do purge these queues when psock refcnt reaches zero but
here we want to purge them explicitly in sock_map_close().
There are also some nasty race conditions on testing bit
SK_PSOCK_TX_ENABLED and queuing/canceling the psock work,
we can expand psock->ingress_lock a bit to protect them too.
As noticed by John, we still have to lock the psock->work,
because the same work item could be running concurrently on
different CPU's.
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 | 2 ++
net/core/skmsg.c | 50 +++++++++++++++++++++++++++++--------------
net/core/sock_map.c | 1 +
3 files changed, 37 insertions(+), 16 deletions(-)
From: Cong Wang <hidden> Date: 2021-03-28 20:21:20
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 | 98 +++++++++++++++++++++++++++++++++++++++++
net/ipv4/tcp_bpf.c | 100 +-----------------------------------------
net/tls/tls_sw.c | 4 +-
5 files changed, 106 insertions(+), 102 deletions(-)
@@ -399,6 +399,104 @@ 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=sk_psock_peek_msg(psock);+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)){+msg_rx=sk_psock_next_msg(psock,msg_rx);+if(!msg_rx)+break;+continue;+}++msg_rx->sg.start=i;+if(!sge->length&&msg_rx->sg.start==msg_rx->sg.end){+msg_rx=sk_psock_dequeue_msg(psock);+kfree_sk_msg(msg_rx);+}+msg_rx=sk_psock_peek_msg(psock);+}++returncopied;+}+EXPORT_SYMBOL_GPL(sk_msg_recvmsg);+staticstructsk_msg*sk_psock_create_ingress_msg(structsock*sk,structsk_buff*skb){
@@ -10,80 +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=sk_psock_peek_msg(psock);-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)){-msg_rx=sk_psock_next_msg(psock,msg_rx);-if(!msg_rx)-break;-continue;-}--msg_rx->sg.start=i;-if(!sge->length&&msg_rx->sg.start==msg_rx->sg.end){-msg_rx=sk_psock_dequeue_msg(psock);-kfree_sk_msg(msg_rx);-}-msg_rx=sk_psock_peek_msg(psock);-}--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-28 20:21:21
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 | 136 ++++++++++++++++++
.../selftests/bpf/progs/test_sockmap_listen.c | 22 +++
2 files changed, 158 insertions(+)
From: Cong Wang <hidden> Date: 2021-03-28 20:21:21
From: Cong Wang <redacted>
This is similar to tcp_read_sock(), except we do not need
to worry about connections, we just need to retrieve skb
from UDP receive queue.
Note, the return value of ->read_sock() is unused in
sk_psock_verdict_data_ready().
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 | 2 ++
net/ipv4/af_inet.c | 1 +
net/ipv4/udp.c | 35 +++++++++++++++++++++++++++++++++++
net/ipv6/af_inet6.c | 1 +
4 files changed, 39 insertions(+)
@@ -329,6 +329,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-28 20:21:21
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(-)
From: Cong Wang <hidden> Date: 2021-03-28 20:21:21
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->psock_update_sk_prot(), 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 | 24 +++++++++++++++++++++---
net/ipv4/tcp_ipv4.c | 3 +++
net/ipv4/udp.c | 3 +++
net/ipv4/udp_bpf.c | 15 +++++++++++++--
net/ipv6/tcp_ipv6.c | 3 +++
net/ipv6/udp.c | 3 +++
12 files changed, 58 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(*psock_update_sk_prot)(structsock*sk,boolrestore);+#endif/* Keeping track of sockets in use */#ifdef CONFIG_PROC_FS
@@ -595,20 +595,38 @@ 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;}+EXPORT_SYMBOL_GPL(tcp_bpf_update_proto);/* If a child got cloned from a listening socket that had tcp_bpf*protocolcallbacksinstalled,weneedtorestorethecallbacksto
@@ -41,12 +41,23 @@ 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;}+EXPORT_SYMBOL_GPL(udp_bpf_update_proto);
From: Cong Wang <hidden> Date: 2021-03-28 20:21:21
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(-)
On Sun, Mar 28, 2021 at 01:20:00PM -0700, Cong Wang wrote:
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, ->read_sock() is required for each protocol to support
sockmap redirection, and in order to do sock proto update, a new ops
->psock_update_sk_prot() is introduced, which is also required. And the
BPF ->recvmsg() is also needed to replace the original ->recvmsg() to
retrieve skmsg. To make life easier, we have to get rid of lock_sock()
in sk_psock_handle_skb(), otherwise we would have to implement
->sendmsg_locked() on top of ->sendmsg(), which is ugly.
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
---
v7: use work_mutex to protect psock->work
return err in udp_read_sock()
add patch 6/13
clean up test case
The feature looks great to me.
I think the selftest is a bit light in terms of coverage, but it's acceptable.
I'd like to see the final Acks from John/Daniel and Jakub/Lorenz before merging.
Folks,
please prioritize the review of these patches.
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-03-29 15:04:17
Alexei Starovoitov wrote:
On Sun, Mar 28, 2021 at 01:20:00PM -0700, Cong Wang wrote:
quoted
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, ->read_sock() is required for each protocol to support
sockmap redirection, and in order to do sock proto update, a new ops
->psock_update_sk_prot() is introduced, which is also required. And the
BPF ->recvmsg() is also needed to replace the original ->recvmsg() to
retrieve skmsg. To make life easier, we have to get rid of lock_sock()
in sk_psock_handle_skb(), otherwise we would have to implement
->sendmsg_locked() on top of ->sendmsg(), which is ugly.
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
---
v7: use work_mutex to protect psock->work
return err in udp_read_sock()
add patch 6/13
clean up test case
The feature looks great to me.
I think the selftest is a bit light in terms of coverage, but it's acceptable.
+1
I'd like to see the final Acks from John/Daniel and Jakub/Lorenz before merging.
Folks,
please prioritize the review of these patches.
This is getting really close I'll take another pass over it today. Thanks
From: Cong Wang <hidden> Date: 2021-03-29 16:58:50
On Mon, Mar 29, 2021 at 8:03 AM John Fastabend [off-list ref] wrote:
Alexei Starovoitov wrote:
quoted
On Sun, Mar 28, 2021 at 01:20:00PM -0700, Cong Wang wrote:
quoted
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, ->read_sock() is required for each protocol to support
sockmap redirection, and in order to do sock proto update, a new ops
->psock_update_sk_prot() is introduced, which is also required. And the
BPF ->recvmsg() is also needed to replace the original ->recvmsg() to
retrieve skmsg. To make life easier, we have to get rid of lock_sock()
in sk_psock_handle_skb(), otherwise we would have to implement
->sendmsg_locked() on top of ->sendmsg(), which is ugly.
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
---
v7: use work_mutex to protect psock->work
return err in udp_read_sock()
add patch 6/13
clean up test case
The feature looks great to me.
I think the selftest is a bit light in terms of coverage, but it's acceptable.
+1
Well, the first half of this patchset still focuses on the existing code, which
is already covered by existing test cases. The second half adds
BPF_SK_SKB_VERDICT and UDP support, which are already covered by
my new test case. And apparently UDP will never support other sockmap
programs like TCP, for example, BPF_SK_SKB_STREAM_PARSER, hence
it of course has much less test cases than TCP. Cross-protocol test case
will be added in the next patchset when AF_UNIX comes in.
If I miss anything, please be specific. Just saying the test case is light does
not help me to understand what I need to add.
Thanks.
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-03-29 19:12:20
Cong Wang wrote:
From: Cong Wang <redacted>
Currently we rely on lock_sock to protect ingress_msg,
it is too big for this, we can actually just use a spinlock
to protect this list like protecting other skb queues.
__tcp_bpf_recvmsg() is still special because of peeking,
it still has to use lock_sock.
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Lorenz Bauer <redacted>
Acked-by: Jakub Sitnicki <jakub@cloudflare.com>
Signed-off-by: Cong Wang <redacted>
---
Acked-by: John Fastabend <john.fastabend@gmail.com>
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-03-29 19:42:26
Cong Wang wrote:
From: Cong Wang <redacted>
We do not have to lock the sock to avoid losing sk_socket,
instead we can purge all the ingress queues when we close
the socket. Sending or receiving packets after orphaning
socket makes no sense.
We do purge these queues when psock refcnt reaches zero but
here we want to purge them explicitly in sock_map_close().
There are also some nasty race conditions on testing bit
SK_PSOCK_TX_ENABLED and queuing/canceling the psock work,
we can expand psock->ingress_lock a bit to protect them too.
As noticed by John, we still have to lock the psock->work,
because the same work item could be running concurrently on
different CPU's.
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>
---
Acked-by: John Fastabend <john.fastabend@gmail.com>
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-03-29 19:44:03
Cong Wang wrote:
From: Cong Wang <redacted>
The RCU callback sk_psock_destroy() only queues work psock->gc,
so we can just switch to rcu work to simplify the code.
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Lorenz Bauer <redacted>
Acked-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Cong Wang <redacted>
---
LGTM
Acked-by: John Fastabend <john.fastabend@gmail.com>
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-03-29 19:45:40
Cong Wang wrote:
quoted hunk
From: Cong Wang <redacted>
This function is only called in process context.
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/skmsg.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-03-29 20:10:57
Cong Wang wrote:
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>
---
Trying to think through this case. User attachs skb_verdict program
to map, then updates map with a bunch of TCP sockets. The above
code will run the skb_verdict program with the TCP socket as far as
I can tell.
This is OK because there really is no difference, other than by name,
between a skb_verdict and a stream_verdict program? Do we want something
to block adding TCP sockets to maps with stream_verdict programs? It
feels a bit odd in its current state to me.
quoted hunk
if (likely(prog)) {
skb_dst_drop(skb);
skb_bpf_redirect_clear(skb);
Do we need another test here,
(skb_verdict && READ_ONCE(psock->progs.stream_verdict)
this way we return EBUSY and avoid having both stream_verdict and
skb_verdict attached on the same map?
From commit msg:
"Users are not allowed to set stream_verdict and skb_verdict at
the same time."
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-03-29 20:55:08
Cong Wang wrote:
quoted hunk
From: Cong Wang <redacted>
This is similar to tcp_read_sock(), except we do not need
to worry about connections, we just need to retrieve skb
from UDP receive queue.
Note, the return value of ->read_sock() is unused in
sk_psock_verdict_data_ready().
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 | 2 ++
net/ipv4/af_inet.c | 1 +
net/ipv4/udp.c | 35 +++++++++++++++++++++++++++++++++++
net/ipv6/af_inet6.c | 1 +
4 files changed, 39 insertions(+)
@@ -329,6 +329,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()
Should this be
int offset = sk_peek_offset()?
MSG_PEEK should work from recv side, at least it does on TCP side. If
its handled in some following patch a comment would be nice. I was
just reading udp_recvmsg() so maybe its not needed.
+ struct sk_buff *skb;
+
+ skb = __skb_recv_udp(sk, 0, 1, &offset, &err);
+ if (!skb)
+ return err;
+ if (offset < skb->len) {
+ size_t len;
+ int used;
+
+ len = skb->len - offset;
+ used = recv_actor(desc, skb, offset, len);
+ if (used <= 0) {
+ if (!copied)
+ copied = used;
+ break;
+ } else if (used <= len) {
+ copied += used;
+ offset += used;
The while loop is going to zero this? What are we trying to do
here with offset?
quoted hunk
+ }
+ }
+ if (!desc->count)
+ break;
+ }
+
+ return copied;
+}
+EXPORT_SYMBOL(udp_read_sock);
+
/*
* This should be easy, if there is something there we
* return it, otherwise we block.
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-03-29 23:11:51
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(-)
From: Cong Wang <hidden> Date: 2021-03-30 01:27:49
On Mon, Mar 29, 2021 at 1:10 PM John Fastabend [off-list ref] wrote:
Cong Wang wrote:
quoted
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>
---
Trying to think through this case. User attachs skb_verdict program
to map, then updates map with a bunch of TCP sockets. The above
code will run the skb_verdict program with the TCP socket as far as
I can tell.
This is OK because there really is no difference, other than by name,
between a skb_verdict and a stream_verdict program? Do we want something
to block adding TCP sockets to maps with stream_verdict programs? It
feels a bit odd in its current state to me.
Yes, it should work too. skb_verdict only extends stream_verdict beyond
TCP, it does not prohibit TCP.
quoted
if (likely(prog)) {
skb_dst_drop(skb);
skb_bpf_redirect_clear(skb);
Do we need another test here,
(skb_verdict && READ_ONCE(psock->progs.stream_verdict)
this way we return EBUSY and avoid having both stream_verdict and
skb_verdict attached on the same map?
Yes, good catch, we do need a check here. And I will see if I can add a small
test case for this too.
Thanks.
From: Cong Wang <hidden> Date: 2021-03-30 05:39:56
On Mon, Mar 29, 2021 at 1:54 PM John Fastabend [off-list ref] wrote:
Cong Wang wrote:
quoted
From: Cong Wang <redacted>
This is similar to tcp_read_sock(), except we do not need
to worry about connections, we just need to retrieve skb
from UDP receive queue.
Note, the return value of ->read_sock() is unused in
sk_psock_verdict_data_ready().
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 | 2 ++
net/ipv4/af_inet.c | 1 +
net/ipv4/udp.c | 35 +++++++++++++++++++++++++++++++++++
net/ipv6/af_inet6.c | 1 +
4 files changed, 39 insertions(+)
@@ -329,6 +329,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()
What are you really suggesting? sk_peek_offset() is just 0 unless
we have MSG_PEEK here and we don't, because we really want to
dequeue the skb rather than peeking it.
Are you suggesting we should do peeking? I am afraid we can't.
Please be specific, guessing your mind is not an effective way to
address your reviews.
MSG_PEEK should work from recv side, at least it does on TCP side. If
its handled in some following patch a comment would be nice. I was
just reading udp_recvmsg() so maybe its not needed.
Please explain why do we need peeking in sockmap? At very least
it has nothing to do with my patchset.
I do not know why you want to use TCP as a "standard" here, TCP
also supports splice(), UDP still doesn't even with ->read_sock().
Of course they are very different.
quoted
+ struct sk_buff *skb;
+
+ skb = __skb_recv_udp(sk, 0, 1, &offset, &err);
+ if (!skb)
+ return err;
+ if (offset < skb->len) {
+ size_t len;
+ int used;
+
+ len = skb->len - offset;
+ used = recv_actor(desc, skb, offset, len);
+ if (used <= 0) {
+ if (!copied)
+ copied = used;
+ break;
+ } else if (used <= len) {
+ copied += used;
+ offset += used;
The while loop is going to zero this? What are we trying to do
here with offset?
offset only matters for MSG_PEEK and we do not support peeking
in sockmap case, hence it is unnecessary here. I "use" it here just
to make the code as complete as possible.
To further answer your question, it is set to 0 when we return a
valid skb on line 201 inside __skb_try_recv_from_queue(), as
"_off" is set to 0 and won't change unless we have MSG_PEEK.
173 bool peek_at_off = false;
174 struct sk_buff *skb;
175 int _off = 0;
176
177 if (unlikely(flags & MSG_PEEK && *off >= 0)) {
178 peek_at_off = true;
179 _off = *off;
180 }
181
182 *last = queue->prev;
183 skb_queue_walk(queue, skb) {
184 if (flags & MSG_PEEK) {
185 if (peek_at_off && _off >= skb->len &&
186 (_off || skb->peeked)) {
187 _off -= skb->len;
188 continue;
189 }
190 if (!skb->len) {
191 skb = skb_set_peeked(skb);
192 if (IS_ERR(skb)) {
193 *err = PTR_ERR(skb);
194 return NULL;
195 }
196 }
197 refcount_inc(&skb->users);
198 } else {
199 __skb_unlink(skb, queue);
200 }
201 *off = _off;
202 return skb;
Of course, when we return NULL, we return immediately without
using offset:
1794 skb = __skb_recv_udp(sk, 0, 1, &offset, &err);
1795 if (!skb)
1796 return err;
This should not be hard to figure out. Hope it is clear now.
Thanks.
From: Cong Wang <hidden> Date: 2021-03-30 05:48:17
On Mon, Mar 29, 2021 at 4:10 PM John Fastabend [off-list ref] wrote:
I think its a bit odd for TCP_ESTABLISHED to work with !tcp, but
thats not your invention so LGTM.
It has been there for many years, so why it is suddenly a problem with
my patchset? More importantly, why don't you change it by yourself
as it looks odd to you? Please go ahead to do whatever you want,
your patches are always welcome.
Thanks.
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-03-30 06:24:35
Cong Wang wrote:
On Mon, Mar 29, 2021 at 1:54 PM John Fastabend [off-list ref] wrote:
quoted
Cong Wang wrote:
quoted
From: Cong Wang <redacted>
This is similar to tcp_read_sock(), except we do not need
to worry about connections, we just need to retrieve skb
from UDP receive queue.
Note, the return value of ->read_sock() is unused in
sk_psock_verdict_data_ready().
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>
---
[...]
quoted
quoted
}
EXPORT_SYMBOL(__skb_recv_udp);
+int udp_read_sock(struct sock *sk, read_descriptor_t *desc,
+ sk_read_actor_t recv_actor)
+{
+ int copied = 0;
+
+ while (1) {
+ int offset = 0, err;
Should this be
int offset = sk_peek_offset()?
What are you really suggesting? sk_peek_offset() is just 0 unless
we have MSG_PEEK here and we don't, because we really want to
dequeue the skb rather than peeking it.
Are you suggesting we should do peeking? I am afraid we can't.
Please be specific, guessing your mind is not an effective way to
address your reviews.
I was only asking for further details because the offset addition
below struck me as odd.
quoted
MSG_PEEK should work from recv side, at least it does on TCP side. If
its handled in some following patch a comment would be nice. I was
just reading udp_recvmsg() so maybe its not needed.
Please explain why do we need peeking in sockmap? At very least
it has nothing to do with my patchset.
We need MSG_PEEK to work from application side. From sockmap
side I agree its not needed.
I do not know why you want to use TCP as a "standard" here, TCP
also supports splice(), UDP still doesn't even with ->read_sock().
Of course they are very different.
Not claiming any "standard" here only that user application needs
to work correctly if it passes MSG_PEEK.
quoted
quoted
+ struct sk_buff *skb;
+
+ skb = __skb_recv_udp(sk, 0, 1, &offset, &err);
+ if (!skb)
+ return err;
+ if (offset < skb->len) {
+ size_t len;
+ int used;
+
+ len = skb->len - offset;
+ used = recv_actor(desc, skb, offset, len);
+ if (used <= 0) {
+ if (!copied)
+ copied = used;
+ break;
+ } else if (used <= len) {
+ copied += used;
+ offset += used;
The while loop is going to zero this? What are we trying to do
here with offset?
offset only matters for MSG_PEEK and we do not support peeking
in sockmap case, hence it is unnecessary here. I "use" it here just
to make the code as complete as possible.
huh? If its not used the addition is just confusing. Can we drop it?
To further answer your question, it is set to 0 when we return a
valid skb on line 201 inside __skb_try_recv_from_queue(), as
"_off" is set to 0 and won't change unless we have MSG_PEEK.
173 bool peek_at_off = false;
174 struct sk_buff *skb;
175 int _off = 0;
176
177 if (unlikely(flags & MSG_PEEK && *off >= 0)) {
178 peek_at_off = true;
179 _off = *off;
180 }
181
182 *last = queue->prev;
183 skb_queue_walk(queue, skb) {
184 if (flags & MSG_PEEK) {
185 if (peek_at_off && _off >= skb->len &&
186 (_off || skb->peeked)) {
187 _off -= skb->len;
188 continue;
189 }
190 if (!skb->len) {
191 skb = skb_set_peeked(skb);
192 if (IS_ERR(skb)) {
193 *err = PTR_ERR(skb);
194 return NULL;
195 }
196 }
197 refcount_inc(&skb->users);
198 } else {
199 __skb_unlink(skb, queue);
200 }
201 *off = _off;
202 return skb;
Of course, when we return NULL, we return immediately without
using offset:
1794 skb = __skb_recv_udp(sk, 0, 1, &offset, &err);
1795 if (!skb)
1796 return err;
This should not be hard to figure out. Hope it is clear now.
Yes, but tracking offset only to clear it a couple lines later
is confusing.
From: Cong Wang <hidden> Date: 2021-03-30 06:37:24
On Mon, Mar 29, 2021 at 11:23 PM John Fastabend
[off-list ref] wrote:
Cong Wang wrote:
quoted
On Mon, Mar 29, 2021 at 1:54 PM John Fastabend [off-list ref] wrote:
quoted
Cong Wang wrote:
quoted
From: Cong Wang <redacted>
This is similar to tcp_read_sock(), except we do not need
to worry about connections, we just need to retrieve skb
from UDP receive queue.
Note, the return value of ->read_sock() is unused in
sk_psock_verdict_data_ready().
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>
---
[...]
quoted
quoted
quoted
}
EXPORT_SYMBOL(__skb_recv_udp);
+int udp_read_sock(struct sock *sk, read_descriptor_t *desc,
+ sk_read_actor_t recv_actor)
+{
+ int copied = 0;
+
+ while (1) {
+ int offset = 0, err;
Should this be
int offset = sk_peek_offset()?
What are you really suggesting? sk_peek_offset() is just 0 unless
we have MSG_PEEK here and we don't, because we really want to
dequeue the skb rather than peeking it.
Are you suggesting we should do peeking? I am afraid we can't.
Please be specific, guessing your mind is not an effective way to
address your reviews.
I was only asking for further details because the offset addition
below struck me as odd.
quoted
quoted
MSG_PEEK should work from recv side, at least it does on TCP side. If
its handled in some following patch a comment would be nice. I was
just reading udp_recvmsg() so maybe its not needed.
Please explain why do we need peeking in sockmap? At very least
it has nothing to do with my patchset.
We need MSG_PEEK to work from application side. From sockmap
side I agree its not needed.
How does the application reach udp_read_sock()? UDP does not support
splice() as I already mentioned, as ->splice_read() is still missing.
quoted
I do not know why you want to use TCP as a "standard" here, TCP
also supports splice(), UDP still doesn't even with ->read_sock().
Of course they are very different.
Not claiming any "standard" here only that user application needs
to work correctly if it passes MSG_PEEK.
I do not see how an application could pass any msg flag to
udp_read_sock().
quoted
quoted
quoted
+ struct sk_buff *skb;
+
+ skb = __skb_recv_udp(sk, 0, 1, &offset, &err);
+ if (!skb)
+ return err;
+ if (offset < skb->len) {
+ size_t len;
+ int used;
+
+ len = skb->len - offset;
+ used = recv_actor(desc, skb, offset, len);
+ if (used <= 0) {
+ if (!copied)
+ copied = used;
+ break;
+ } else if (used <= len) {
+ copied += used;
+ offset += used;
The while loop is going to zero this? What are we trying to do
here with offset?
offset only matters for MSG_PEEK and we do not support peeking
in sockmap case, hence it is unnecessary here. I "use" it here just
to make the code as complete as possible.
huh? If its not used the addition is just confusing. Can we drop it?
If you mean dropping this single line of code, yes. If you mean
dropping 'offset' completely, no, as both __skb_recv_udp() and
recv_actor() still need it. If you mean I should re-write
__skb_recv_udp() and recv_actor() just to drop 'offset', I am afraid
that is too much with too little gain.
quoted
To further answer your question, it is set to 0 when we return a
valid skb on line 201 inside __skb_try_recv_from_queue(), as
"_off" is set to 0 and won't change unless we have MSG_PEEK.
173 bool peek_at_off = false;
174 struct sk_buff *skb;
175 int _off = 0;
176
177 if (unlikely(flags & MSG_PEEK && *off >= 0)) {
178 peek_at_off = true;
179 _off = *off;
180 }
181
182 *last = queue->prev;
183 skb_queue_walk(queue, skb) {
184 if (flags & MSG_PEEK) {
185 if (peek_at_off && _off >= skb->len &&
186 (_off || skb->peeked)) {
187 _off -= skb->len;
188 continue;
189 }
190 if (!skb->len) {
191 skb = skb_set_peeked(skb);
192 if (IS_ERR(skb)) {
193 *err = PTR_ERR(skb);
194 return NULL;
195 }
196 }
197 refcount_inc(&skb->users);
198 } else {
199 __skb_unlink(skb, queue);
200 }
201 *off = _off;
202 return skb;
Of course, when we return NULL, we return immediately without
using offset:
1794 skb = __skb_recv_udp(sk, 0, 1, &offset, &err);
1795 if (!skb)
1796 return err;
This should not be hard to figure out. Hope it is clear now.
Yes, but tracking offset only to clear it a couple lines later
is confusing.
Yeah, but that's __skb_recv_udp()'s fault, not mine. We can refactor
__skb_recv_udp() a bit for !MSG_PEEK case, but I do not see
much gain here.
Thanks.
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-03-30 06:46:09
Cong Wang wrote:
On Mon, Mar 29, 2021 at 11:23 PM John Fastabend
[off-list ref] wrote:
quoted
Cong Wang wrote:
quoted
On Mon, Mar 29, 2021 at 1:54 PM John Fastabend [off-list ref] wrote:
quoted
Cong Wang wrote:
quoted
From: Cong Wang <redacted>
This is similar to tcp_read_sock(), except we do not need
to worry about connections, we just need to retrieve skb
from UDP receive queue.
Note, the return value of ->read_sock() is unused in
sk_psock_verdict_data_ready().
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>
---
[...]
quoted
quoted
quoted
}
EXPORT_SYMBOL(__skb_recv_udp);
+int udp_read_sock(struct sock *sk, read_descriptor_t *desc,
+ sk_read_actor_t recv_actor)
+{
+ int copied = 0;
+
+ while (1) {
+ int offset = 0, err;
Should this be
int offset = sk_peek_offset()?
What are you really suggesting? sk_peek_offset() is just 0 unless
we have MSG_PEEK here and we don't, because we really want to
dequeue the skb rather than peeking it.
Are you suggesting we should do peeking? I am afraid we can't.
Please be specific, guessing your mind is not an effective way to
address your reviews.
I was only asking for further details because the offset addition
below struck me as odd.
quoted
quoted
MSG_PEEK should work from recv side, at least it does on TCP side. If
its handled in some following patch a comment would be nice. I was
just reading udp_recvmsg() so maybe its not needed.
Please explain why do we need peeking in sockmap? At very least
it has nothing to do with my patchset.
We need MSG_PEEK to work from application side. From sockmap
side I agree its not needed.
How does the application reach udp_read_sock()? UDP does not support
splice() as I already mentioned, as ->splice_read() is still missing.
It doesn't. All I was trying to say is if an application calls
recvmsg(..., MSG_PEEK) it should work correctly. It wasn't a
comment about this specific patch.
quoted
quoted
I do not know why you want to use TCP as a "standard" here, TCP
also supports splice(), UDP still doesn't even with ->read_sock().
Of course they are very different.
Not claiming any "standard" here only that user application needs
to work correctly if it passes MSG_PEEK.
I do not see how an application could pass any msg flag to
udp_read_sock().
Agree.
quoted
quoted
quoted
quoted
+ struct sk_buff *skb;
+
+ skb = __skb_recv_udp(sk, 0, 1, &offset, &err);
+ if (!skb)
+ return err;
+ if (offset < skb->len) {
+ size_t len;
+ int used;
+
+ len = skb->len - offset;
+ used = recv_actor(desc, skb, offset, len);
+ if (used <= 0) {
+ if (!copied)
+ copied = used;
+ break;
+ } else if (used <= len) {
+ copied += used;
+ offset += used;
The while loop is going to zero this? What are we trying to do
here with offset?
offset only matters for MSG_PEEK and we do not support peeking
in sockmap case, hence it is unnecessary here. I "use" it here just
to make the code as complete as possible.
huh? If its not used the addition is just confusing. Can we drop it?
If you mean dropping this single line of code, yes. If you mean
dropping 'offset' completely, no, as both __skb_recv_udp() and
recv_actor() still need it. If you mean I should re-write
__skb_recv_udp() and recv_actor() just to drop 'offset', I am afraid
that is too much with too little gain.
All I'm saying is drop the single line of code above. This specific
one
'offset += used'
And add a comment in the commit msg that just says peeking is not
supported. I think we need at least one more respin of the patches
anyways to address a different small comment so should be easy.
quoted
quoted
To further answer your question, it is set to 0 when we return a
valid skb on line 201 inside __skb_try_recv_from_queue(), as
"_off" is set to 0 and won't change unless we have MSG_PEEK.
173 bool peek_at_off = false;
174 struct sk_buff *skb;
175 int _off = 0;
176
177 if (unlikely(flags & MSG_PEEK && *off >= 0)) {
178 peek_at_off = true;
179 _off = *off;
180 }
181
182 *last = queue->prev;
183 skb_queue_walk(queue, skb) {
184 if (flags & MSG_PEEK) {
185 if (peek_at_off && _off >= skb->len &&
186 (_off || skb->peeked)) {
187 _off -= skb->len;
188 continue;
189 }
190 if (!skb->len) {
191 skb = skb_set_peeked(skb);
192 if (IS_ERR(skb)) {
193 *err = PTR_ERR(skb);
194 return NULL;
195 }
196 }
197 refcount_inc(&skb->users);
198 } else {
199 __skb_unlink(skb, queue);
200 }
201 *off = _off;
202 return skb;
Of course, when we return NULL, we return immediately without
using offset:
1794 skb = __skb_recv_udp(sk, 0, 1, &offset, &err);
1795 if (!skb)
1796 return err;
This should not be hard to figure out. Hope it is clear now.
Yes, but tracking offset only to clear it a couple lines later
is confusing.
Yeah, but that's __skb_recv_udp()'s fault, not mine. We can refactor
__skb_recv_udp() a bit for !MSG_PEEK case, but I do not see
much gain here.
No don't bother here. I don't see much gain in doing that either. If
you want do it in another series not this one.