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

[Daniel Borkmann <daniel@iogearbox.net>]

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!