On Thu, Jan 23, 2020 at 08:15 PM CET, John Fastabend wrote:

Jakub Sitnicki wrote:

quoted

On Thu, Jan 23, 2020 at 06:18 PM CET, Eric Dumazet wrote:

quoted

On 1/23/20 7:55 AM, 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, that is with neither sk_lock nor sk_callback_lock held.

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

[...]

quoted

quoted

quoted

 include/linux/skmsg.h | 3 ++-
 net/core/sock.c       | 5 +++--
 net/ipv4/tcp_bpf.c    | 4 +++-
 net/ipv4/tcp_ulp.c    | 3 ++-
 net/tls/tls_main.c    | 3 ++-
 5 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index 41ea1258d15e..55c834a5c25e 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h

@@ -352,7 +352,8 @@ static inline void sk_psock_update_proto(struct sock *sk,
 	psock->saved_write_space = sk->sk_write_space;

 	psock->sk_proto = sk->sk_prot;
-	sk->sk_prot = ops;
+	/* Pairs with lockless read in sk_clone_lock() */
+	WRITE_ONCE(sk->sk_prot, ops);

Note there are dozens of calls like

if (sk->sk_prot->handler)
    sk->sk_prot->handler(...);

Some of them being done lockless.

I know it is painful, but presumably we need

Correct.

quoted

quoted

const struct proto *ops = READ_ONCE(sk->sk_prot);

if (ops->handler)
    ops->handler(....);

[...]

quoted

Considering these are lacking today, can I do it as a follow up?

In my opinion yes. I think pulling your patches in is OK and I started
doing this conversion already so we can have a fix shortly. I didn't
want to push it into rc7 though so I'll push it next week or into
net-next tree.

Thanks, John. I'll take a look if there is any access to sk_prot, apart
from the clone path, applicable only to TCP listening sockets that needs
annotating.