From mboxrd@z Thu Jan  1 00:00:00 1970
From: John Fastabend <john.fastabend@gmail.com>
Subject: Re: [bpf PATCH 2/3] bpf: sockmap, fix transition through disconnect
 without close
Date: Mon, 17 Sep 2018 21:40:21 -0700
Message-ID: <0b55da5e-893b-8cfe-87df-b536a1e3565b@gmail.com>
References: <20180917172946.21218.66049.stgit@john-Precision-Tower-5810>
 <20180917173155.21218.11026.stgit@john-Precision-Tower-5810>
 <CAH3MdRU-0p9wno4Vmo6xCaEF9r83+WP_UBLccqA0tmcP3yOCnA@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Cc: edumazet@google.com, Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        netdev <netdev@vger.kernel.org>
To: Y Song <ys114321@gmail.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-it0-f68.google.com ([209.85.214.68]:55375 "EHLO
        mail-it0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1726399AbeIRKLX (ORCPT
        <rfc822;netdev@vger.kernel.org>); Tue, 18 Sep 2018 06:11:23 -0400
Received: by mail-it0-f68.google.com with SMTP id d10-v6so1382586itj.5
        for <netdev@vger.kernel.org>; Mon, 17 Sep 2018 21:40:37 -0700 (PDT)
In-Reply-To: <CAH3MdRU-0p9wno4Vmo6xCaEF9r83+WP_UBLccqA0tmcP3yOCnA@mail.gmail.com>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On 09/17/2018 02:09 PM, Y Song wrote:
> On Mon, Sep 17, 2018 at 10:32 AM John Fastabend
> <john.fastabend@gmail.com> wrote:
>>
>> It is possible (via shutdown()) for TCP socks to go trough TCP_CLOSE
>> state via tcp_disconnect() without actually calling tcp_close which
>> would then call our bpf_tcp_close() callback. Because of this a user
>> could disconnect a socket then put it in a LISTEN state which would
>> break our assumptions about sockets always being ESTABLISHED state.
>>
>> To resolve this rely on the unhash hook, which is called in the
>> disconnect case, to remove the sock from the sockmap.
>>
>> Reported-by: Eric Dumazet <edumazet@google.com>
>> Fixes: 1aa12bdf1bfb ("bpf: sockmap, add sock close() hook to remove socks")
>> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
>> ---

[...]

>> +{
>> +       void (*unhash_fun)(struct sock *sk);
>> +       struct smap_psock *psock;
>> +
>> +       rcu_read_lock();
>> +       psock = smap_psock_sk(sk);
>> +       if (unlikely(!psock)) {
>> +               rcu_read_unlock();
>> +               release_sock(sk);
> 
> Can socket be released here?
>

Right, it was an error (it can not be released) fixed in v3.


>> +               return sk->sk_prot->unhash(sk);
>> +       }
>> +
>> +       /* The psock may be destroyed anytime after exiting the RCU critial
>> +        * section so by the time we use close_fun the psock may no longer
>> +        * be valid. However, bpf_tcp_close is called with the sock lock
>> +        * held so the close hook and sk are still valid.
>> +        */
> 
> the comments above are not correct. A copy-paste mistake?

I just removed the comments they are not overly helpful at this point
and the commit msg is more useful anyways.

Thanks,
John