Re: [bpf PATCH v2] bpf: sockmap, fix crash when ipv6 sock is added

From: Daniel Borkmann <daniel@iogearbox.net>
To: John Fastabend <john.fastabend@gmail.com>,
	Eric Dumazet <eric.dumazet@gmail.com>,
	edumazet@google.com, ast@kernel.org,
	Dave Watson <davejwatson@fb.com>
Cc: netdev@vger.kernel.org
Subject: Re: [bpf PATCH v2] bpf: sockmap, fix crash when ipv6 sock is added
Date: Mon, 4 Jun 2018 15:39:01 +0200	[thread overview]
Message-ID: <c9eab906-6793-1e98-b9d8-01d665ac1c3c@iogearbox.net> (raw)
In-Reply-To: <81abd5f7-5343-a27a-6715-8b413f6c5a27@gmail.com>

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