From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Fastabend Subject: Re: [bpf PATCH v2 1/6] bpf: sockmap, fix crash when ipv6 sock is added Date: Thu, 14 Jun 2018 21:46:36 -0700 Message-ID: <95c027ce-e1d5-22ba-09c5-6b81504318b8@gmail.com> References: <20180614164148.24994.65250.stgit@john-Precision-Tower-5810> <20180614164446.24994.22118.stgit@john-Precision-Tower-5810> <20180614235321.hi3qcno7cee4cgc4@kafai-mbp.dhcp.thefacebook.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: ast@kernel.org, daniel@iogearbox.net, netdev@vger.kernel.org To: Martin KaFai Lau Return-path: Received: from mail-io0-f194.google.com ([209.85.223.194]:45067 "EHLO mail-io0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753370AbeFOErZ (ORCPT ); Fri, 15 Jun 2018 00:47:25 -0400 Received: by mail-io0-f194.google.com with SMTP id l25-v6so9516815ioh.12 for ; Thu, 14 Jun 2018 21:47:25 -0700 (PDT) In-Reply-To: <20180614235321.hi3qcno7cee4cgc4@kafai-mbp.dhcp.thefacebook.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 06/14/2018 04:53 PM, Martin KaFai Lau wrote: > On Thu, Jun 14, 2018 at 09:44:46AM -0700, John Fastabend wrote: >> This fixes a crash where we assign tcp_prot to IPv6 sockets instead >> of tcpv6_prot. >> >> Previously we overwrote the sk->prot field with tcp_prot even in the >> AF_INET6 case. This patch ensures the correct tcp_prot and tcpv6_prot >> are used. > >> Further, only allow ESTABLISHED connections to join the >> map per note in TLS ULP, >> >> /* The TLS ulp is currently supported only for TCP sockets >> * in ESTABLISHED state. >> * Supporting sockets in LISTEN state will require us >> * to modify the accept implementation to clone rather then >> * share the ulp context. >> */ > This bit has been moved to patch 2. Yep better cut the comment as well. > >> >> Also tested with 'netserver -6' and 'netperf -H [IPv6]' as well as >> 'netperf -H [IPv4]'. The ESTABLISHED check resolves the previously >> crashing case here. >> >> Fixes: 174a79ff9515 ("bpf: sockmap with sk redirect support") >> Reported-by: syzbot+5c063698bdbfac19f363@syzkaller.appspotmail.com >> Signed-off-by: John Fastabend >> Signed-off-by: Wei Wang >> --- >> 0 files changed >> 0 files changed will fix that as well. >> diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c >> index 52a91d8..f6dd4cd 100644 >> --- a/kernel/bpf/sockmap.c >> +++ b/kernel/bpf/sockmap.c >> @@ -140,6 +140,7 @@ static int bpf_tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, >> static int bpf_tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size); >> static int bpf_tcp_sendpage(struct sock *sk, struct page *page, >> int offset, size_t size, int flags); >> +static void bpf_tcp_close(struct sock *sk, long timeout); >> >> static inline struct smap_psock *smap_psock_sk(const struct sock *sk) >> { >> @@ -161,7 +162,42 @@ static bool bpf_tcp_stream_read(const struct sock *sk) >> return !empty; >> } >> >> -static struct proto tcp_bpf_proto; >> +enum { >> + SOCKMAP_IPV4, >> + SOCKMAP_IPV6, >> + SOCKMAP_NUM_PROTS, >> +}; >> + >> +enum { >> + SOCKMAP_BASE, >> + SOCKMAP_TX, >> + SOCKMAP_NUM_CONFIGS, >> +}; >> + >> +static struct proto *saved_tcpv6_prot; > __read_mostly > Sure makes sense. >> +static DEFINE_MUTEX(tcpv6_prot_mutex); >> +static struct proto bpf_tcp_prots[SOCKMAP_NUM_PROTS][SOCKMAP_NUM_CONFIGS]; >> +static void build_protos(struct proto prot[SOCKMAP_NUM_CONFIGS], >> + struct proto *base) >> +{ >> + prot[SOCKMAP_BASE] = *base; >> + prot[SOCKMAP_BASE].close = bpf_tcp_close; >> + prot[SOCKMAP_BASE].recvmsg = bpf_tcp_recvmsg; >> + prot[SOCKMAP_BASE].stream_memory_read = bpf_tcp_stream_read; >> + >> + prot[SOCKMAP_TX] = prot[SOCKMAP_BASE]; >> + prot[SOCKMAP_TX].sendmsg = bpf_tcp_sendmsg; >> + prot[SOCKMAP_TX].sendpage = bpf_tcp_sendpage; >> +} >> + >> +static void update_sk_prot(struct sock *sk, struct smap_psock *psock) >> +{ >> + int family = sk->sk_family == AF_INET6 ? SOCKMAP_IPV6 : SOCKMAP_IPV4; >> + int conf = psock->bpf_tx_msg ? SOCKMAP_TX : SOCKMAP_BASE; >> + >> + sk->sk_prot = &bpf_tcp_prots[family][conf]; >> +} >> + >> static int bpf_tcp_init(struct sock *sk) >> { >> struct smap_psock *psock; >> @@ -181,14 +217,17 @@ static int bpf_tcp_init(struct sock *sk) >> psock->save_close = sk->sk_prot->close; >> psock->sk_proto = sk->sk_prot; >> >> - if (psock->bpf_tx_msg) { >> - tcp_bpf_proto.sendmsg = bpf_tcp_sendmsg; >> - tcp_bpf_proto.sendpage = bpf_tcp_sendpage; >> - tcp_bpf_proto.recvmsg = bpf_tcp_recvmsg; >> - tcp_bpf_proto.stream_memory_read = bpf_tcp_stream_read; >> + /* Build IPv6 sockmap whenever the address of tcpv6_prot changes */ >> + if (sk->sk_family == AF_INET6 && >> + unlikely(sk->sk_prot != smp_load_acquire(&saved_tcpv6_prot))) { >> + mutex_lock(&tcpv6_prot_mutex); > bpf_tcp_init() can be called by skops? > Can mutex_lock() be used here? > No mutex lock can not be used here. Both are called with rcu_read_lock() and we can not sleep. Thanks for catching. Also this will give a kernel splat now that I have the right config options. Guess we need a v3 :/ Thanks, John