On 09/17/2018 02:09 PM, Y Song wrote:
> On Mon, Sep 17, 2018 at 10:32 AM John Fastabend
> <john.fastab...@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 <eduma...@google.com>
>> Fixes: 1aa12bdf1bfb ("bpf: sockmap, add sock close() hook to remove socks")
>> Signed-off-by: John Fastabend <john.fastab...@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
