From mboxrd@z Thu Jan  1 00:00:00 1970
From: John Fastabend <john.fastabend@gmail.com>
Subject: Re: [bpf PATCH v2] bpf: sockmap, fix crash when ipv6 sock is added
Date: Sat, 2 Jun 2018 14:39:20 -0700
Message-ID: <81abd5f7-5343-a27a-6715-8b413f6c5a27@gmail.com>
References: <20180601194641.5717.11725.stgit@john-Precision-Tower-5810>
 <13d3be75-4ed2-aeca-caba-797766e9b676@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Cc: netdev@vger.kernel.org
To: Eric Dumazet <eric.dumazet@gmail.com>, edumazet@google.com,
        ast@kernel.org, daniel@iogearbox.net,
        Dave Watson <davejwatson@fb.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-io0-f195.google.com ([209.85.223.195]:40702 "EHLO
        mail-io0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751753AbeFBVjd (ORCPT
        <rfc822;netdev@vger.kernel.org>); Sat, 2 Jun 2018 17:39:33 -0400
Received: by mail-io0-f195.google.com with SMTP id g22-v6so5576516iob.7
        for <netdev@vger.kernel.org>; Sat, 02 Jun 2018 14:39:33 -0700 (PDT)
In-Reply-To: <13d3be75-4ed2-aeca-caba-797766e9b676@gmail.com>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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!

> 
> Thanks !
>