Commit 665bcfc9 authored by Paolo Abeni's avatar Paolo Abeni
Browse files

Merge branch 'vsock-some-fixes-due-to-transport-de-assignment' 

Stefano Garzarella says:

====================
vsock: some fixes due to transport de-assignment

v1: https://lore.kernel.org/netdev/20250108180617.154053-1-sgarzare@redhat.com/
v2:
- Added patch 3 to cancel the virtio close delayed work when de-assigning
  the transport
- Added patch 4 to clean the socket state after de-assigning the transport
- Added patch 5 as suggested by Michael and Hyunwoo Kim. It's based on
  Hyunwoo Kim and Wongi Lee patch [1] but using WARN_ON and covering more
  functions
- Added R-b/T-b tags

This series includes two patches discussed in the thread started by
Hyunwoo Kim a few weeks ago [1], plus 3 more patches added after some
discussions on v1 (see changelog). All related to the case where a vsock
socket is de-assigned from a transport (e.g., because the connect fails
or is interrupted by a signal) and then assigned to another transport
or to no-one (NULL).

I tested with usual vsock test suite, plus Michal repro [2]. (Note: the repo
works only if a G2H transport is not loaded, e.g. virtio-vsock driver).

The first patch is a fix more appropriate to the problem reported in
that thread, the second patch on the other hand is a related fix but
of a different problem highlighted by Michal Luczaj. It's present only
in vsock_bpf and already handled in af_vsock.c
The third patch is to cancel the virtio close delayed work when de-assigning
the transport, the fourth patch is to clean the socket state after de-assigning
the transport, the last patch adds warnings and prevents null-ptr-deref in
vsock_*[has_data|has_space].

Hyunwoo Kim, Michal, if you can test and report your Tested-by that
would be great!

[1] https://lore.kernel.org/netdev/Z2K%2FI4nlHdfMRTZC@v4bel-B760M-AORUS-ELITE-AX/
[2] https://lore.kernel.org/netdev/2b3062e3-bdaa-4c94-a3c0-2930595b9670@rbox.co/
====================

Link: https://patch.msgid.link/20250110083511.30419-1-sgarzare@redhat.com


Signed-off-by: default avatarPaolo Abeni <pabeni@redhat.com>
parents 0865b9fd 91751e24

net/vmw_vsock/af_vsock.c

+18 −0
Original line number Diff line number Diff line
@@ -491,6 +491,15 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk) 
		 */

		vsk->transport->release(vsk);

		vsock_deassign_transport(vsk);



		/* transport's release() and destruct() can touch some socket

		 * state, since we are reassigning the socket to a new transport

		 * during vsock_connect(), let's reset these fields to have a

		 * clean state.

		 */

		sock_reset_flag(sk, SOCK_DONE);

		sk->sk_state = TCP_CLOSE;

		vsk->peer_shutdown = 0;

	}



	/* We increase the module refcnt to prevent the transport unloading
@@ -870,6 +879,9 @@ EXPORT_SYMBOL_GPL(vsock_create_connected); 


s64 vsock_stream_has_data(struct vsock_sock *vsk)

{

	if (WARN_ON(!vsk->transport))

		return 0;



	return vsk->transport->stream_has_data(vsk);

}

EXPORT_SYMBOL_GPL(vsock_stream_has_data);
@@ -878,6 +890,9 @@ s64 vsock_connectible_has_data(struct vsock_sock *vsk) 
{

	struct sock *sk = sk_vsock(vsk);



	if (WARN_ON(!vsk->transport))

		return 0;



	if (sk->sk_type == SOCK_SEQPACKET)

		return vsk->transport->seqpacket_has_data(vsk);

	else
@@ -887,6 +902,9 @@ EXPORT_SYMBOL_GPL(vsock_connectible_has_data); 


s64 vsock_stream_has_space(struct vsock_sock *vsk)

{

	if (WARN_ON(!vsk->transport))

		return 0;



	return vsk->transport->stream_has_space(vsk);

}

EXPORT_SYMBOL_GPL(vsock_stream_has_space);

net/vmw_vsock/virtio_transport_common.c

+26 −10
Original line number Diff line number Diff line
@@ -26,6 +26,9 @@ 
/* Threshold for detecting small packets to copy */

#define GOOD_COPY_LEN  128



static void virtio_transport_cancel_close_work(struct vsock_sock *vsk,

					       bool cancel_timeout);



static const struct virtio_transport *

virtio_transport_get_ops(struct vsock_sock *vsk)

{
@@ -1109,6 +1112,8 @@ void virtio_transport_destruct(struct vsock_sock *vsk) 
{

	struct virtio_vsock_sock *vvs = vsk->trans;



	virtio_transport_cancel_close_work(vsk, true);



	kfree(vvs);

	vsk->trans = NULL;

}
@@ -1204,17 +1209,11 @@ static void virtio_transport_wait_close(struct sock *sk, long timeout) 
	}

}



static void virtio_transport_do_close(struct vsock_sock *vsk,

static void virtio_transport_cancel_close_work(struct vsock_sock *vsk,

					       bool cancel_timeout)

{

	struct sock *sk = sk_vsock(vsk);



	sock_set_flag(sk, SOCK_DONE);

	vsk->peer_shutdown = SHUTDOWN_MASK;

	if (vsock_stream_has_data(vsk) <= 0)

		sk->sk_state = TCP_CLOSING;

	sk->sk_state_change(sk);



	if (vsk->close_work_scheduled &&

	    (!cancel_timeout || cancel_delayed_work(&vsk->close_work))) {

		vsk->close_work_scheduled = false;
@@ -1226,6 +1225,20 @@ static void virtio_transport_do_close(struct vsock_sock *vsk, 
	}

}



static void virtio_transport_do_close(struct vsock_sock *vsk,

				      bool cancel_timeout)

{

	struct sock *sk = sk_vsock(vsk);



	sock_set_flag(sk, SOCK_DONE);

	vsk->peer_shutdown = SHUTDOWN_MASK;

	if (vsock_stream_has_data(vsk) <= 0)

		sk->sk_state = TCP_CLOSING;

	sk->sk_state_change(sk);



	virtio_transport_cancel_close_work(vsk, cancel_timeout);

}



static void virtio_transport_close_timeout(struct work_struct *work)

{

	struct vsock_sock *vsk =
@@ -1628,8 +1641,11 @@ void virtio_transport_recv_pkt(struct virtio_transport *t, 


	lock_sock(sk);



	/* Check if sk has been closed before lock_sock */

	if (sock_flag(sk, SOCK_DONE)) {

	/* Check if sk has been closed or assigned to another transport before

	 * lock_sock (note: listener sockets are not assigned to any transport)

	 */

	if (sock_flag(sk, SOCK_DONE) ||

	    (sk->sk_state != TCP_LISTEN && vsk->transport != &t->transport)) {

		(void)virtio_transport_reset_no_sock(t, skb);

		release_sock(sk);

		sock_put(sk);

net/vmw_vsock/vsock_bpf.c

+9 −0
Original line number Diff line number Diff line
@@ -77,6 +77,7 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg, 
			     size_t len, int flags, int *addr_len)

{

	struct sk_psock *psock;

	struct vsock_sock *vsk;

	int copied;



	psock = sk_psock_get(sk);
@@ -84,6 +85,13 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg, 
		return __vsock_recvmsg(sk, msg, len, flags);



	lock_sock(sk);

	vsk = vsock_sk(sk);



	if (!vsk->transport) {

		copied = -ENODEV;

		goto out;

	}



	if (vsock_has_data(sk, psock) && sk_psock_queue_empty(psock)) {

		release_sock(sk);

		sk_psock_put(sk, psock);
@@ -108,6 +116,7 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg, 
		copied = sk_msg_recvmsg(sk, psock, msg, len, flags);

	}



out:

	release_sock(sk);

	sk_psock_put(sk, psock);