Re: [Patch bpf-next v2 2/9] sock: introduce sk_prot->update_proto()

On Tue, 2 Mar 2021 at 18:23, Cong Wang [off-list ref] wrote:

quoted

if the function returned a struct proto * like it does at the moment.
That way we keep sk->sk_prot manipulation confined to the sockmap code
and don't have to copy paste it into every proto.

Well, TCP seems too special to do this, as it could call tcp_update_ulp()
to update the proto.

I had a quick look, tcp_bpf_update_proto is the only caller of tcp_update_ulp,
which in turn is the only caller of icsk_ulp_ops->update, which in turn is only
implemented as tls_update in tls_main.c. Turns out that tls_update
has another one of these calls:

} else {
    /* Pairs with lockless read in sk_clone_lock(). */
    WRITE_ONCE(sk->sk_prot, p);
    sk->sk_write_space = write_space;
}

Maybe it looks familiar? :o) I think it would be a worthwhile change.

quoted

quoted

diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 3bddd9dd2da2..13d2af5bb81c 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c

@@ -184,26 +184,10 @@ static void sock_map_unref(struct sock *sk, void *link_raw)

 static int sock_map_init_proto(struct sock *sk, struct sk_psock *psock)
 {
-       struct proto *prot;
-
-       switch (sk->sk_type) {
-       case SOCK_STREAM:
-               prot = tcp_bpf_get_proto(sk, psock);
-               break;
-
-       case SOCK_DGRAM:
-               prot = udp_bpf_get_proto(sk, psock);
-               break;
-
-       default:
+       if (!sk->sk_prot->update_proto)
                return -EINVAL;
-       }
-
-       if (IS_ERR(prot))
-               return PTR_ERR(prot);
-
-       sk_psock_update_proto(sk, psock, prot);
-       return 0;
+       psock->saved_update_proto = sk->sk_prot->update_proto;
+       return sk->sk_prot->update_proto(sk, false);

I think reads / writes from sk_prot need READ_ONCE / WRITE_ONCE. We've
not been diligent about this so far, but I think it makes sense to be
careful in new code.

Hmm, there are many places not using READ_ONCE/WRITE_ONCE,
for a quick example:

I know! I'll defer to John and Jakub.

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com