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 16:55:04 +0200 Message-ID: <1e5d6144-6e8d-3d01-8112-a7908a3a9997@iogearbox.net> 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]:37622 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751782AbeFDOzJ (ORCPT ); Mon, 4 Jun 2018 10:55:09 -0400 In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 06/04/2018 03:57 PM, John Fastabend wrote: > On 06/04/2018 06:39 AM, Daniel Borkmann wrote: >> 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. > > OK. > >> 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. >> > > This fix works for both sockmap and sockhash because they use the same > ulp register and init paths. But, will rebase for net-next and send out > this morning. Ok, right, because in bpf-next this eventually goes into __sock_map_ctx_update_elem() instead of sock_map_ctx_update_elem() call site. >> After that, likely also fixes for the disconnect + listen case are needed. > > Yep will have a fix today for this. > >> (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.) > > hash map is also resolved with the same patch but please do queue this > up for -stable. Will do, thanks!