From: Cong Wang <hidden> Date: 2021-03-31 02:33:43
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
---
v8: get rid of 'offset' in udp_read_sock()
add checks for skb_verdict/stream_verdict conflict
add two cleanup patches for sock_map_link()
add a new test case
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 (16):
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: simplify sock_map_link() a bit
sock_map: kill sock_map_link_no_progs()
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
selftests/bpf: add a test case for loading BPF_SK_SKB_VERDICT
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 | 118 ++++++------
net/ipv4/af_inet.c | 1 +
net/ipv4/tcp_bpf.c | 130 +++----------
net/ipv4/tcp_ipv4.c | 3 +
net/ipv4/udp.c | 32 ++++
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_basic.c | 40 ++++
.../selftests/bpf/prog_tests/sockmap_listen.c | 136 ++++++++++++++
.../selftests/bpf/progs/test_sockmap_listen.c | 22 +++
.../progs/test_sockmap_skb_verdict_attach.c | 18 ++
26 files changed, 677 insertions(+), 237 deletions(-)
create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_skb_verdict_attach.c
--
2.25.1
From: Cong Wang <hidden> Date: 2021-03-31 02:33:44
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-31 02:33:44
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: Daniel Borkmann <daniel@iogearbox.net>
Cc: Lorenz Bauer <redacted>
Acked-by: Jakub Sitnicki <jakub@cloudflare.com>
Acked-by: John Fastabend <john.fastabend@gmail.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-31 02:33:44
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-31 02:33:44
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-31 02:33:44
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: 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 | 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-31 02:33:44
From: Cong Wang <redacted>
This function is only called in process context.
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>
---
net/core/skmsg.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Cong Wang <hidden> Date: 2021-03-31 02:33:44
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 attach stream_verdict and skb_verdict programs to the
same map.
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 | 28 ++++++++++++++++++++++++++++
tools/bpf/bpftool/common.c | 1 +
tools/bpf/bpftool/prog.c | 1 +
tools/include/uapi/linux/bpf.h | 1 +
8 files changed, 38 insertions(+), 1 deletion(-)
From: Cong Wang <hidden> Date: 2021-03-31 02:33:44
From: Cong Wang <redacted>
sock_map_link() passes down map progs, but it is confusing
to see both map progs and psock progs. Make the map progs
more obvious by retrieving it directly with sock_map_progs()
inside sock_map_link(). Now it is aligned with
sock_map_link_no_progs() too.
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Lorenz Bauer <redacted>
Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Cong Wang <redacted>
---
net/core/sock_map.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
From: Cong Wang <hidden> Date: 2021-03-31 02:33:45
From: Cong Wang <redacted>
Now we can fold sock_map_link_no_progs() into sock_map_link()
and get rid of sock_map_link_no_progs().
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Lorenz Bauer <redacted>
Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Cong Wang <redacted>
---
net/core/sock_map.c | 55 +++++++++++++--------------------------------
1 file changed, 15 insertions(+), 40 deletions(-)
@@ -225,13 +225,24 @@ static struct sk_psock *sock_map_psock_get_checked(struct sock *sk)returnpsock;}+staticboolsock_map_redirect_allowed(conststructsock*sk);+staticintsock_map_link(structbpf_map*map,structsock*sk){-structbpf_prog*msg_parser,*stream_parser,*stream_verdict;structsk_psock_progs*progs=sock_map_progs(map);+structbpf_prog*stream_verdict=NULL;+structbpf_prog*stream_parser=NULL;+structbpf_prog*msg_parser=NULL;structsk_psock*psock;intret;+/* Only sockets we can redirect into/from in BPF need to hold+*refstoparser/verdictprogsandhavetheirsk_data_ready+*andsk_write_spacecallbacksoverridden.+*/+if(!sock_map_redirect_allowed(sk))+gotono_progs;+stream_verdict=READ_ONCE(progs->stream_verdict);if(stream_verdict){stream_verdict=bpf_prog_inc_not_zero(stream_verdict);
@@ -488,14 +477,7 @@ static int sock_map_update_common(struct bpf_map *map, u32 idx,if(!link)return-ENOMEM;-/* Only sockets we can redirect into/from in BPF need to hold-*refstoparser/verdictprogsandhavetheirsk_data_ready-*andsk_write_spacecallbacksoverridden.-*/-if(sock_map_redirect_allowed(sk))-ret=sock_map_link(map,sk);-else-ret=sock_map_link_no_progs(map,sk);+ret=sock_map_link(map,sk);if(ret<0)gotoout_free;
@@ -1000,14 +982,7 @@ static int sock_hash_update_common(struct bpf_map *map, void *key,if(!link)return-ENOMEM;-/* Only sockets we can redirect into/from in BPF need to hold-*refstoparser/verdictprogsandhavetheirsk_data_ready-*andsk_write_spacecallbacksoverridden.-*/-if(sock_map_redirect_allowed(sk))-ret=sock_map_link(map,sk);-else-ret=sock_map_link_no_progs(map,sk);+ret=sock_map_link(map,sk);if(ret<0)gotoout_free;
From: Cong Wang <hidden> Date: 2021-03-31 02:33:45
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-31 02:33:45
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-31 02:33:45
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(), and UDP still does not
support splice() due to lack of ->splice_read(), so users
can not reach udp_read_sock() directly.
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 | 29 +++++++++++++++++++++++++++++
net/ipv6/af_inet6.c | 1 +
4 files changed, 33 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-31 02:33:45
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-31 02:33:45
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-31 02:33:46
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-31 02:33:46
From: Cong Wang <redacted>
This adds a test case to ensure BPF_SK_SKB_VERDICT and
BPF_SK_STREAM_VERDICT will never be attached 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>
---
.../selftests/bpf/prog_tests/sockmap_basic.c | 40 +++++++++++++++++++
.../progs/test_sockmap_skb_verdict_attach.c | 18 +++++++++
2 files changed, 58 insertions(+)
create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_skb_verdict_attach.c
@@ -7,6 +7,7 @@#include"test_skmsg_load_helpers.skel.h"#include"test_sockmap_update.skel.h"#include"test_sockmap_invalid_update.skel.h"+#include"test_sockmap_skb_verdict_attach.skel.h"#include"bpf_iter_sockmap.skel.h"#define TCP_REPAIR 19 /* TCP sock is under repair right now */
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-03-31 22:01:02
Cong Wang wrote:
quoted hunk
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: John Fastabend <john.fastabend@gmail.com> Date: 2021-04-01 05:49:24
Cong Wang wrote:
From: Cong Wang <redacted>
sock_map_link() passes down map progs, but it is confusing
to see both map progs and psock progs. Make the map progs
more obvious by retrieving it directly with sock_map_progs()
inside sock_map_link(). Now it is aligned with
sock_map_link_no_progs() too.
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Lorenz Bauer <redacted>
Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Cong Wang <redacted>
---
Acked-by: John Fastabend <john.fastabend@gmail.com>
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-04-01 05:53:34
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 attach stream_verdict and skb_verdict programs to the
same map.
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>
---
Looks good.
Acked-by: John Fastabend <john.fastabend@gmail.com>
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-04-01 06:01:44
Cong Wang wrote:
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(), and UDP still does not
support splice() due to lack of ->splice_read(), so users
can not reach udp_read_sock() directly.
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>
---
Thanks this is easier to read IMO. One nit below.
Acked-by: John Fastabend <john.fastabend@gmail.com>
[...]
+int udp_read_sock(struct sock *sk, read_descriptor_t *desc,
+ sk_read_actor_t recv_actor)
+{
+ int copied = 0;
+
+ while (1) {
+ struct sk_buff *skb;
+ int err, used;
+
+ skb = skb_recv_udp(sk, 0, 1, &err);
+ if (!skb)
+ return err;
+ used = recv_actor(desc, skb, 0, skb->len);
+ if (used <= 0) {
+ if (!copied)
+ copied = used;
+ break;
+ } else if (used <= skb->len) {
+ copied += used;
+ }
This 'else if' is always true if above is false right? Would be
impler and clearer IMO as,
if (used <= 0) {
if (!copied)
copied = used;
break;
}
copied += used;
I don't see anyway for used to be great than skb->len.
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-04-01 06:03:19
Cong Wang wrote:
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(-)
Acked-by: John Fastabend <john.fastabend@gmail.com>
From: Jakub Sitnicki <jakub@cloudflare.com> Date: 2021-04-01 08:11:25
On Wed, Mar 31, 2021 at 04:32 AM CEST, Cong Wang wrote:
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>
---
Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-04-01 17:46:09
Cong Wang wrote:
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>
Acked-by: John Fastabend <john.fastabend@gmail.com>
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-04-01 17:51:58
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
---
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-04-01 18:08:52
Cong Wang wrote:
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>
Acked-by: John Fastabend <john.fastabend@gmail.com>
On Thu, Apr 1, 2021 at 10:52 AM John Fastabend [off-list ref] wrote:
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
---
This LGTM, thanks for doing this Cong.
Applied. Thanks everyone.
Cong, please follow up with minor cleanup that John requested.
From: Jakub Sitnicki <jakub@cloudflare.com> Date: 2021-04-02 10:16:42
On Wed, Mar 31, 2021 at 04:32 AM CEST, Cong Wang wrote:
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>
---
@@ -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() */
Just to clarify. UDP sockets don't get cloned, so the above comment
apply.
From: Cong Wang <hidden> Date: 2021-04-03 05:08:51
On Wed, Mar 31, 2021 at 11:01 PM John Fastabend
[off-list ref] wrote:
This 'else if' is always true if above is false right? Would be
impler and clearer IMO as,
if (used <= 0) {
if (!copied)
copied = used;
break;
}
copied += used;
I don't see anyway for used to be great than skb->len.
Yes, slightly better. Please feel free to submit a patch by yourself,
like always your patches are welcome.
Please also remember to submit a patch to address the name
TCP_ESTABLISHED, or literally any code you feel uncomfortable
with. I am actually comfortable with what they are, hence not
motivated to make a change.
BTW, please try to group your reviews in one round, it is
completely a waste of time to address your review one during
each update.
On my side, I need to adjust the cover letter, rebase the
whole patchset, and manually add your ACK's. On your side,
you have to read this again and again. On other people side,
they just see more than a dozen patches flooding in the mailing
list again and again. In the end, everyone's time is wasted, this
can be avoided if you just try to group as many reviews as possible
together. I certainly do not mind waiting for more time just to get
more reviews in one round.
And please do not give any ACK unless you are comfortable with
the whole patchset, because otherwise I have to add it manually.
It is not too late to give one single ACK to the whole patchset once
you are comfortable with everything. This would save some traffic
in the mailing list too.
Thanks!
On Fri, Apr 2, 2021 at 10:12 PM Cong Wang [off-list ref] wrote:
On Wed, Mar 31, 2021 at 11:01 PM John Fastabend
[off-list ref] wrote:
quoted
This 'else if' is always true if above is false right? Would be
impler and clearer IMO as,
if (used <= 0) {
if (!copied)
copied = used;
break;
}
copied += used;
I don't see anyway for used to be great than skb->len.
Yes, slightly better. Please feel free to submit a patch by yourself,
like always your patches are welcome.
Please submit a follow up patch as John requested
or I'm reverting your set.
From: Eric Dumazet <hidden> Date: 2021-04-05 08:25:33
On 3/31/21 4:32 AM, Cong Wang wrote:
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.
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-04-06 18:12:21
Eric Dumazet wrote:
On 3/31/21 4:32 AM, Cong Wang wrote:
quoted
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.
sk_psock() is using rcu_dereference_sk_user_data()
First caller of this is here,
sock_{hash|map}_update_common <- has a WARN_ON_ONCE(!rcu_read_lock_held);
sock_map_link()
sock_map_init_proto()
psock_update_sk_prot(sk, false)
And the other does this,
sk_psock_put()
sk_psock_drop()
sk_psock_restore_proto
psock_update_sk_prot(sk, true)
But we can get here through many callers and it sure doesn't look like its
all safe. For example one case,
.sendmsg
tcp_bpf_sendmsg
psock = sk_psock_get(sk)
sk_psock_put(sk, psock) <- this doesn't have the RCU held
quoted
int family = sk->sk_family == AF_INET6 ? TCP_BPF_IPV6 : TCP_BPF_IPV4;
int config = psock->progs.msg_parser ? TCP_BPF_TX : TCP_BPF_BASE;
Same issue in udp_bpf_update_proto() of course.
Yep.
Either we revert the patch or we can fix it to pass the psock through.
Passing the psock works because we have a reference on it and it wont
go away. I don't have any other good ideas off-hand.
Thanks Eric! I'm a bit surprised we didn't get an RCU splat from the
tests though.
.John
From: Cong Wang <hidden> Date: 2021-04-06 18:30:42
On Mon, Apr 5, 2021 at 1:25 AM Eric Dumazet [off-list ref] wrote:
On 3/31/21 4:32 AM, Cong Wang wrote:
quoted
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.
I do not think RCU is held here ?
sk_psock() is using rcu_dereference_sk_user_data()
Right, I just saw the syzbot report. But here we already have
the writer lock of sk_callback_lock, hence RCU read lock here
makes no sense to me. Probably we just have to tell RCU we
already have sk_callback_lock.
Thanks.
From: John Fastabend <john.fastabend@gmail.com> Date: 2021-04-06 21:08:07
Cong Wang wrote:
On Mon, Apr 5, 2021 at 1:25 AM Eric Dumazet [off-list ref] wrote:
quoted
On 3/31/21 4:32 AM, Cong Wang wrote:
quoted
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.
I do not think RCU is held here ?
sk_psock() is using rcu_dereference_sk_user_data()
Right, I just saw the syzbot report. But here we already have
the writer lock of sk_callback_lock, hence RCU read lock here
makes no sense to me. Probably we just have to tell RCU we
already have sk_callback_lock.
Thanks.
I think you need to ensure its the psock we originally grabbed as
well. Otherwise how do we ensure the psock is not swapped from
another thread?