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!
