Jakub Sitnicki wrote:

On Sun, Jan 12, 2020 at 12:14 AM CET, John Fastabend wrote:

quoted

Jakub Sitnicki wrote:

quoted

sk_msg and ULP frameworks override protocol callbacks pointer in
sk->sk_prot, while TCP accesses it locklessly when cloning the listening
socket.

Once we enable use of listening sockets with sockmap (and hence sk_msg),
there can be shared access to sk->sk_prot if socket is getting cloned while
being inserted/deleted to/from the sockmap from another CPU. Mark the
shared access with READ_ONCE/WRITE_ONCE annotations.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>

In sockmap side I fixed this by wrapping the access in a lock_sock[0]. So
Do you think this is still needed with that in mind? The bpf_clone call
is using sk_prot_creater and also setting the newsk's proto field. Even
if the listening parent sock was being deleted in parallel would that be
a problem? We don't touch sk_prot_creator from the tear down path. I've
only scanned the 3..11 patches so maybe the answer is below. If that is
the case probably an improved commit message would be helpful.

I think it is needed. Not because of tcp_bpf_clone or that we access
listener's sk_prot_creator from there, if I'm grasping your question.

Either way I'm glad this came up. Let's go though my reasoning and
verify it. tcp stack accesses the listener sk_prot while cloning it:

tcp_v4_rcv
  sk = __inet_lookup_skb(...)
  tcp_check_req(sk)
    inet_csk(sk)->icsk_af_ops->syn_recv_sock
      tcp_v4_syn_recv_sock
        tcp_create_openreq_child
          inet_csk_clone_lock
            sk_clone_lock
              READ_ONCE(sk->sk_prot)

It grabs a reference to the listener, but doesn't grab the sk_lock.

On another CPU we can be inserting/removing the listener socket from the
sockmap and writing to its sk_prot. We have the update and the remove
path:

sock_map_ops->map_update_elem
  sock_map_update_elem
    sock_map_update_common
      sock_map_link_no_progs
        tcp_bpf_init
          tcp_bpf_update_sk_prot
            sk_psock_update_proto
              WRITE_ONCE(sk->sk_prot, ops)

sock_map_ops->map_delete_elem
  sock_map_delete_elem
    __sock_map_delete
     sock_map_unref
       sk_psock_put
         sk_psock_drop
           sk_psock_restore_proto
             tcp_update_ulp
               WRITE_ONCE(sk->sk_prot, proto)

Following the guidelines from KTSAN project [0], sk_prot looks like a
candidate for annotating it. At least on these 3 call paths.

If that sounds correct, I can add it to the patch description.

Logic looks correct to me thanks for the details, please put those in
the commit so we don't lose them. Can you also add a comment where it
makes most sense in the code? This is a bit subtle and we don't want
to miss it later. Probably in tcp_update_ulp near that WRITE_ONCE would
do. It doesn't need to be too verbose but something as simple as,

"{WRITE|READ}_ONCE wrappers needed around sk_prot to protect unlocked
 reads in sk_clone_lock"

Thanks,
-jkbs

[0] https://github.com/google/ktsan/wiki/READ_ONCE-and-WRITE_ONCE