From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Fastabend Subject: Re: [bpf PATCH v2] bpf: sockmap, fix crash when ipv6 sock is added Date: Mon, 4 Jun 2018 06:57:28 -0700 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: Daniel Borkmann , Eric Dumazet , edumazet@google.com, ast@kernel.org, Dave Watson Return-path: Received: from mail-it0-f67.google.com ([209.85.214.67]:35252 "EHLO mail-it0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753210AbeFDN5p (ORCPT ); Mon, 4 Jun 2018 09:57:45 -0400 Received: by mail-it0-f67.google.com with SMTP id a3-v6so10325166itd.0 for ; Mon, 04 Jun 2018 06:57:44 -0700 (PDT) In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 06/04/2018 06:39 AM, Daniel Borkmann wrote: > 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. > 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. > 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. > Thanks, > Daniel >