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
