Re: [PATCH net-next v4 1/4] vsock/virtio: split socket lookup from locked RX processing

[Stefano Garzarella <sgarzare@redhat.com>]

On Wed, Oct 14, 2026 at 07:56:37PM +0800, Jia Jia wrote:
>Split virtio_transport_recv_pkt() so socket lookup is separate from
>receive state machine processing under the socket lock. This prepares the
>receive path to keep a looked-up socket across multiple packets in a later
>change.
>
>Keep virtio_transport_recv_pkt() on its existing per-packet locking path.
>The locked helper obtains packet-specific addresses from the skb and uses
>the matched socket namespace for reset replies. Preserve source validation
>and failure handling.
>
>No functional change intended.
>
>Signed-off-by: Jia Jia <physicalmtea@gmail.com>
>---
> net/vmw_vsock/virtio_transport_common.c | 128 ++++++++++++++----------
> 1 file changed, 75 insertions(+), 53 deletions(-)
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index f225f53ed4bab..61ee3bc205d5d 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -1774,81 +1774,60 @@ static bool virtio_transport_valid_type(u16 type)
> 	       (type == VIRTIO_VSOCK_TYPE_SEQPACKET);
> }
>
>-/* We are under the virtio-vsock's vsock->rx_lock or vhost-vsock's vq->mutex
>- * lock.
>- */
>-void virtio_transport_recv_pkt(struct virtio_transport *t,
>-			       struct sk_buff *skb, struct net *net)
>+static struct sock *
>+virtio_transport_find_socket(struct sk_buff *skb, struct sockaddr_vm *src,
>+			     struct sockaddr_vm *dst, struct net *net)
> {
> 	struct virtio_vsock_hdr *hdr = virtio_vsock_hdr(skb);
>-	struct sockaddr_vm src, dst;
>-	struct vsock_sock *vsk;
> 	struct sock *sk;
>-	bool space_available;
>-
>-	vsock_addr_init(&src, le64_to_cpu(hdr->src_cid),
>-			le32_to_cpu(hdr->src_port));
>-	vsock_addr_init(&dst, le64_to_cpu(hdr->dst_cid),
>-			le32_to_cpu(hdr->dst_port));
>-
>-	trace_virtio_transport_recv_pkt(src.svm_cid, src.svm_port,
>-					dst.svm_cid, dst.svm_port,
>-					le32_to_cpu(hdr->len),
>-					le16_to_cpu(hdr->type),
>-					le16_to_cpu(hdr->op),
>-					le32_to_cpu(hdr->flags),
>-					le32_to_cpu(hdr->buf_alloc),
>-					le32_to_cpu(hdr->fwd_cnt));
>
>-	if (!virtio_transport_valid_type(le16_to_cpu(hdr->type))) {
>-		(void)virtio_transport_reset_no_sock(t, skb, net);
>-		goto free_pkt;
>-	}
>+	if (!virtio_transport_valid_type(le16_to_cpu(hdr->type)))
>+		return NULL;
>
>-	/* The socket must be in connected or bound table
>-	 * otherwise send reset back
>-	 */
>-	sk = vsock_find_connected_socket_net(&src, &dst, net);
>-	if (!sk) {
>-		sk = vsock_find_bound_socket_net(&dst, net);
>-		if (!sk) {
>-			(void)virtio_transport_reset_no_sock(t, skb, net);
>-			goto free_pkt;
>-		}
>-	}
>+	sk = vsock_find_connected_socket_net(src, dst, net);
>+	if (!sk)
>+		sk = vsock_find_bound_socket_net(dst, net);
>+	if (!sk)
>+		return NULL;
>
> 	if (virtio_transport_get_type(sk) != le16_to_cpu(hdr->type)) {
>-		(void)virtio_transport_reset_no_sock(t, skb, net);
> 		sock_put(sk);
>-		goto free_pkt;
>+		return NULL;
> 	}
>
>-	if (!skb_set_owner_sk_safe(skb, sk)) {
>-		WARN_ONCE(1, "receiving vsock socket has sk_refcnt == 0\n");
>-		goto free_pkt;
>-	}
>+	return sk;
>+}
>
>-	vsk = vsock_sk(sk);
>+/*
>+ * The caller holds sk's socket lock and must free skb if this returns true.

Why the skb must be freed by the caller and not by this function itself?

>+ */
>+static bool
>+virtio_transport_recv_pkt_locked(struct virtio_transport *t,
>+				 struct sk_buff *skb, struct sock *sk)
>+{
>+	struct virtio_vsock_hdr *hdr = virtio_vsock_hdr(skb);
>+	struct sockaddr_vm src;
>+	struct vsock_sock *vsk = vsock_sk(sk);
>+	bool space_available;

Please, follow the netdev convection for ordering local variables: 
https://www.kernel.org/doc/html/v7.2/process/maintainer-netdev.html#local-variable-ordering-reverse-xmas-tree-rcs

>
>-	lock_sock(sk);
>+	vsock_addr_init(&src, le64_to_cpu(hdr->src_cid),
>+			le32_to_cpu(hdr->src_port));
>
>-	/* Check if sk has been closed or assigned to another transport before
>-	 * lock_sock (note: listener sockets are not assigned to any transport)
>+	/* Listener sockets accept packets from any source and are not assigned to
>+	 * a transport.
> 	 */

Why changing this comment in this commit?

Oh because we moved out lock_sock, right?

I think we should keep in some way the old info that was much more 
informative that the current comment that just say what the check is 
doing and not why.

> 	if (sock_flag(sk, SOCK_DONE) ||
> 	    (sk->sk_state != TCP_LISTEN &&
> 	     !vsock_check_source(vsk, &t->transport, &src))) {
>-		(void)virtio_transport_reset_no_sock(t, skb, net);
>-		release_sock(sk);
>-		sock_put(sk);
>-		goto free_pkt;
>+		(void)virtio_transport_reset_no_sock(t, skb, sock_net(sk));

We need to be careful here, IIRC Bobby used the net from the caller for 
a reason.

Are you sure it's fine this change? Why not passing net as parameter and 
minimize changes?

>+		return true;
> 	}
>
> 	space_available = virtio_transport_space_update(sk, skb);
>
> 	/* Update CID in case it has changed after a transport reset event */
> 	if (vsk->local_addr.svm_cid != VMADDR_CID_ANY)
>-		vsk->local_addr.svm_cid = dst.svm_cid;
>+		vsk->local_addr.svm_cid = le64_to_cpu(hdr->dst_cid);
>
> 	if (space_available)
> 		sk->sk_write_space(sk);
>@@ -1870,17 +1849,60 @@ void virtio_transport_recv_pkt(struct virtio_transport *t,
> 		kfree_skb(skb);
> 		break;
> 	default:
>-		(void)virtio_transport_reset_no_sock(t, skb, net);
>+		(void)virtio_transport_reset_no_sock(t, skb, sock_net(sk));
> 		kfree_skb(skb);
> 		break;
> 	}
>
>+	return false;
>+}
>+
>+/* We are under the virtio-vsock's vsock->rx_lock or vhost-vsock's vq->mutex
>+ * lock.
>+ */
>+void virtio_transport_recv_pkt(struct virtio_transport *t,
>+			       struct sk_buff *skb, struct net *net)
>+{
>+	struct virtio_vsock_hdr *hdr = virtio_vsock_hdr(skb);
>+	struct sockaddr_vm src, dst;
>+	struct sock *sk;
>+	bool free_pkt;
>+
>+	vsock_addr_init(&src, le64_to_cpu(hdr->src_cid),
>+			le32_to_cpu(hdr->src_port));
>+	vsock_addr_init(&dst, le64_to_cpu(hdr->dst_cid),
>+			le32_to_cpu(hdr->dst_port));
>+
>+	trace_virtio_transport_recv_pkt(src.svm_cid, src.svm_port,
>+					dst.svm_cid, dst.svm_port,
>+					le32_to_cpu(hdr->len),
>+					le16_to_cpu(hdr->type),
>+					le16_to_cpu(hdr->op),
>+					le32_to_cpu(hdr->flags),
>+					le32_to_cpu(hdr->buf_alloc),
>+					le32_to_cpu(hdr->fwd_cnt));
>+
>+	sk = virtio_transport_find_socket(skb, &src, &dst, net);
>+	if (!sk) {
>+		(void)virtio_transport_reset_no_sock(t, skb, net);
>+		goto free_pkt;
>+	}
>+
>+	if (!skb_set_owner_sk_safe(skb, sk)) {
>+		WARN_ONCE(1, "receiving vsock socket has sk_refcnt == 0\n");
>+		goto free_pkt;
>+	}
>+
>+	lock_sock(sk);
>+	free_pkt = virtio_transport_recv_pkt_locked(t, skb, sk);
> 	release_sock(sk);
>
> 	/* Release refcnt obtained when we fetched this socket out of the
> 	 * bound or connected list.
> 	 */
> 	sock_put(sk);
>+	if (free_pkt)
>+		kfree_skb(skb);
> 	return;
>
> free_pkt:
>-- 
>2.53.0
>