Re: [PATCH bpf 3/4] bpf, vsock: Invoke proto::close on close()

On Mon, Nov 18, 2024 at 10:03:43PM +0100, Michal Luczaj wrote:

vsock defines a BPF callback to be invoked when close() is called. However, this callback is never actually executed. As a result, a closed vsock socket is not automatically removed from the sockmap/sockhash.

Introduce a dummy vsock_close() and make vsock_release() call proto::close.

Note: changes in __vsock_release() look messy, but it's only due to indent level reduction and variables xmas tree reorder.

Fixes: 634f1a7110b4 ("vsock: support sockmap") Signed-off-by: Michal Luczaj mhal@rbox.co

---

net/vmw_vsock/af_vsock.c | 67 +++++++++++++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 27 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index 919da8edd03c838cbcdbf1618425da6c5ec2df1a..b52b798aa4c2926c3f233aad6cd31b4056f6fee2 100644 --- a/net/vmw_vsock/af_vsock.c +++ b/net/vmw_vsock/af_vsock.c @@ -117,12 +117,14 @@ static int __vsock_bind(struct sock *sk, struct sockaddr_vm *addr); static void vsock_sk_destruct(struct sock *sk); static int vsock_queue_rcv_skb(struct sock *sk, struct sk_buff *skb); +static void vsock_close(struct sock *sk, long timeout);

/* Protocol family. */ struct proto vsock_proto = { .name = "AF_VSOCK", .owner = THIS_MODULE, .obj_size = sizeof(struct vsock_sock),

• .close = vsock_close,

#ifdef CONFIG_BPF_SYSCALL .psock_update_sk_prot = vsock_bpf_update_proto, #endif @@ -797,39 +799,37 @@ static bool sock_type_connectible(u16 type)

static void __vsock_release(struct sock *sk, int level) {

• if (sk) {

• struct sock *pending;
  

• struct vsock_sock *vsk;
  

• vsk = vsock_sk(sk);
  

• pending = NULL;	/* Compiler warning. */
  

• struct vsock_sock *vsk;
• struct sock *pending;

• /* When "level" is SINGLE_DEPTH_NESTING, use the nested
  

•  * version to avoid the warning "possible recursive locking
  

•  * detected". When "level" is 0, lock_sock_nested(sk, level)
  

•  * is the same as lock_sock(sk).
  

•  */
  

• lock_sock_nested(sk, level);
  

• vsk = vsock_sk(sk);
• pending = NULL; /* Compiler warning. */

• if (vsk->transport)
  

• 	vsk->transport->release(vsk);
  

• else if (sock_type_connectible(sk->sk_type))
  

• 	vsock_remove_sock(vsk);
  

• /* When "level" is SINGLE_DEPTH_NESTING, use the nested

• * version to avoid the warning "possible recursive locking
  

• * detected". When "level" is 0, lock_sock_nested(sk, level)
  

• * is the same as lock_sock(sk).
  

• */
  

• lock_sock_nested(sk, level);

• sock_orphan(sk);
  

• sk->sk_shutdown = SHUTDOWN_MASK;
  

• if (vsk->transport)

• vsk->transport->release(vsk);
  

• else if (sock_type_connectible(sk->sk_type))

• vsock_remove_sock(vsk);
  
  

• skb_queue_purge(&sk->sk_receive_queue);
  

• sock_orphan(sk);
• sk->sk_shutdown = SHUTDOWN_MASK;

• /* Clean up any sockets that never were accepted. */
  

• while ((pending = vsock_dequeue_accept(sk)) != NULL) {
  

• 	__vsock_release(pending, SINGLE_DEPTH_NESTING);
  

• 	sock_put(pending);
  

• }
  

• skb_queue_purge(&sk->sk_receive_queue);

• release_sock(sk);
  

• sock_put(sk);
  

• /* Clean up any sockets that never were accepted. */
• while ((pending = vsock_dequeue_accept(sk)) != NULL) {

• __vsock_release(pending, SINGLE_DEPTH_NESTING);
  

• sock_put(pending);
  

  }
• release_sock(sk);
• sock_put(sk);

}

static void vsock_sk_destruct(struct sock *sk) @@ -901,9 +901,22 @@ void vsock_data_ready(struct sock *sk) } EXPORT_SYMBOL_GPL(vsock_data_ready);

+/* Dummy callback required by sockmap.

• • See unconditional call of saved_close() in sock_map_close().
• */

+static void vsock_close(struct sock *sk, long timeout) +{ +}

static int vsock_release(struct socket *sock) {

• __vsock_release(sock->sk, 0);

• struct sock *sk = sock->sk;
• if (!sk)

• return 0;
  

Compared with before, now we return earlier and so we don't set SS_FREE, could it be risky?

I think no, because in theory we have already set it in a previous call, right?

Reviewed-by: Stefano Garzarella sgarzare@redhat.com

• sk->sk_prot->close(sk, 0);
• __vsock_release(sk, 0); sock->sk = NULL; sock->state = SS_FREE;

-- 2.46.2