On Thu, Nov 21, 2024 at 8:54 PM Michal Luczaj <mhal@xxxxxxx> wrote:
>
> On 11/21/24 10:22, Stefano Garzarella wrote:
> > 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@xxxxxxx>
> >> ---
> >> 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?
>
> Yeah, and is there actually a way to call vsock_release() for a second
> time? The only caller I see is __sock_release(), which won't allow that.
Maybe no, but the `sock->sk` check made me think so.
>
> As for the sockets that never had ->sk assigned, I assume it doesn't matter.
Yep, so my R-b stays here ;-)
Thanks for these great fixes,
Stefano
>
> > Reviewed-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
> >
> >> +
> >> + sk->sk_prot->close(sk, 0);
> >> + __vsock_release(sk, 0);
> >> sock->sk = NULL;
> >> sock->state = SS_FREE;
>
