Re: [bpf PATCH 4/6] bpf: sockmap, tcp_disconnect to listen transition

[John Fastabend <john.fastabend@gmail.com>]

On 06/13/2018 10:48 PM, John Fastabend wrote:
> On 06/13/2018 05:56 PM, Martin KaFai Lau wrote:
>> On Wed, Jun 13, 2018 at 10:50:14AM -0700, John Fastabend wrote:
>>> After adding checks to ensure TCP is in ESTABLISHED state when a
>>> sock is added we need to also ensure that user does not transition
>>> through tcp_disconnect() and back into ESTABLISHED state without
>>> sockmap removing the sock.
>>>
>>> To do this add unhash hook and remove sock from map there.
>> In bpf_tcp_init():
>>         sk->sk_prot = &tcp_bpf_proto;
>>
>> I may have missed a lock while reading sockmap.c.
>> Is it possible that tcp_disconnect() is being called while
>> the above assignment is also being done (e.g. through BPF_MAP_UPDATE_ELEM)?
>> The same situation go for the ESTABLISHED check.
>>
> 
> Right because ESTABLISHED is checked without any locking its
> possible that the state changes during the update (from userspce
> BPF_MAP_UPDATE, from sock_ops program it is locked). I have
> the below patch on my tree now, I was thinking to send it as
> a follow on but on second thought it likely makes more sense
> as part of the patch that adds the ESTABLISHED check.
> 
> Also after the below the sk_callback lock used to protect
> psock->maps is becoming increasingly pointless it allows the
> delete and map free ops to be called without taking the full
> sock lock. It might be time to just drop it in bpf-next and
> use the sock lock in the delete cases. The more annoying part
> will be the delete will have to have different userspace and
> bpf program helpers so we know when we need the lock.
> 
> --- a/kernel/bpf/sockmap.c

Hi Martin,

I went ahead and sent a v2 with the sock lock addition included.

Thanks,
John