From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Borkmann Subject: Re: [bpf PATCH v2] bpf: sockmap, fix crash when ipv6 sock is added Date: Mon, 4 Jun 2018 15:39:01 +0200 Message-ID: References: <20180601194641.5717.11725.stgit@john-Precision-Tower-5810> <13d3be75-4ed2-aeca-caba-797766e9b676@gmail.com> <81abd5f7-5343-a27a-6715-8b413f6c5a27@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org To: John Fastabend , Eric Dumazet , edumazet@google.com, ast@kernel.org, Dave Watson Return-path: Received: from www62.your-server.de ([213.133.104.62]:58965 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753063AbeFDNjH (ORCPT ); Mon, 4 Jun 2018 09:39:07 -0400 In-Reply-To: <81abd5f7-5343-a27a-6715-8b413f6c5a27@gmail.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: Hey guys, On 06/02/2018 11:39 PM, John Fastabend wrote: > On 06/01/2018 12:58 PM, Eric Dumazet wrote: >> On 06/01/2018 03:46 PM, John Fastabend wrote: >>> This fixes a crash where we assign tcp_prot to IPv6 sockets instead >>> of tcpv6_prot. >> >> ... >> >>> + /* ULPs are 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. >>> + */ >>> + if (sock->sk_state != TCP_ESTABLISHED) >>> + return -ENOTSUPP; >>> + >>> /* 1. If sock map has BPF programs those will be inherited by the >>> * sock being added. If the sock is already attached to BPF programs >>> * this results in an error. >> >> Next question will be then : What happens if syzbot uses tcp_disconnect() and then listen() ? > > Yep we need to fix that as well :( Looks like we can plumb the > unhash callback and remove it from the sockmap when the socket > goes through tcp_disconnect(). > > This patch should go in as-is though and we can fix the disconnect > issue with a new patch. > > Adding Dave Watson to the thread as well because I'm guessing > the disconnect() case is also applicable to TLS. At least I see > a hw handler for unhash but there does not appear to be a handler > in the SW case, at least from a quick glance. > > Thanks again! Given the discussion and fixes weren't resolved resp. ready in time for 4.17, and last bpf pr for it went out last week, we need to route this via -stable once all is hashed out. This fix here therefore needs to be rebased against bpf-next tree, and as far as I can see another fix for hash map is also needed to address the same issue. After that, likely also fixes for the disconnect + listen case are needed. (I can use the one here later on for -stable backport, but given merge window is open this needs a rebase and a resolution for hash map.) Thanks, Daniel