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

[Bobby Eshleman <bobbyeshleman@gmail.com>]

On Sat, Oct 10, 2026 at 10:22:43PM +0800, Jia Jia wrote:
> Split virtio_transport_recv_pkt() so socket lookup is separate from the
> receive state machine that runs under the socket lock.
> 
> Keep virtio_transport_recv_pkt() on its existing per-packet locking path.
> Pass the source and destination addresses used for lookup to the locked
> receive path so they are decoded only once.
> 
> Pass the network namespace and decoded address tuple through a packet
> context to the locked receive path.
> 
> Preserve source validation and failure handling. Keep the lookup reference
> separate from the skb owner reference so the locked receive path can manage
> the two lifetimes independently.
> 
> Signed-off-by: Jia Jia <physicalmtea@gmail.com>
> ---
>  net/vmw_vsock/virtio_transport_common.c | 139 ++++++++++++++++--------
>  1 file changed, 96 insertions(+), 43 deletions(-)
> 
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index f225f53..e3e75e5 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -1774,81 +1774,97 @@ 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 void
> +virtio_transport_recv_pkt_init_addrs(struct sk_buff *skb,
> +				     struct sockaddr_vm *src,
> +				     struct sockaddr_vm *dst)

I'd prefer these calls stay inline, there is nothing complex enough here
to justify abstracting IMHO.

Could we move things around so that we don't have the arbitrary move of
virtio_transport_recv_pkt()? It tends to create noise and makes it
harder to focus on the more important parts of the patch.

>  {
>  	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),
> +	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),
> +	vsock_addr_init(dst, le64_to_cpu(hdr->dst_cid),
>  			le32_to_cpu(hdr->dst_port));
> +}
> +
> +static void
> +virtio_transport_trace_recv_pkt(struct sk_buff *skb,
> +				const struct sockaddr_vm *src,
> +				const struct sockaddr_vm *dst)
> +{
> +	struct virtio_vsock_hdr *hdr = virtio_vsock_hdr(skb);
>  
> -	trace_virtio_transport_recv_pkt(src.svm_cid, src.svm_port,
> -					dst.svm_cid, dst.svm_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));
> +}

Any chance we can get rid of this wrapper and just call
trace_virtio_transport_recv_pkt() directly? Even from multiple call
sites I don't think this is a big simplification win.

>  
> -	if (!virtio_transport_valid_type(le16_to_cpu(hdr->type))) {
> -		(void)virtio_transport_reset_no_sock(t, skb, net);
> -		goto free_pkt;
> -	}
> +static struct sock *
> +virtio_transport_recv_pkt_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 sock *sk;
>  
> -	/* 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;
> -		}
> -	}
> +	if (!virtio_transport_valid_type(le16_to_cpu(hdr->type)))
> +		return NULL;
> +
> +	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;
> +}

Similarly here, the unrelated functions blending makes for a harder
read. Maybe worth moving things around or trying a different diff
algorithm?

nit: maybe we could just call this virtio_transport_find_socket()? AFAIK
the tx side doesn't need a socket lookup since we already have the
socket there, so we can omit the disambiguation if it lends a nicer
name.

>  
> -	vsk = vsock_sk(sk);
> +struct virtio_transport_rx_pkt_ctx {
> +	struct net *net;
> +	const struct sockaddr_vm *src;
> +	const struct sockaddr_vm *dst;
> +};

Let's just pass these directly too...

>  
> -	lock_sock(sk);
> +/*
> + * The caller holds sk's socket lock and must free skb if this returns true.
> + */
> +static bool
> +virtio_transport_recv_pkt_locked(struct virtio_transport *t,
> +				 struct sk_buff *skb, struct sock *sk,
> +				 const struct virtio_transport_rx_pkt_ctx *ctx)
> +{
> +	const struct sockaddr_vm *src = ctx->src;
> +	const struct sockaddr_vm *dst = ctx->dst;
> +	struct vsock_sock *vsk = vsock_sk(sk);
> +	struct net *net = ctx->net;
> +	bool space_available;
>  
> -	/* Check if sk has been closed or assigned to another transport before
> -	 * lock_sock (note: listener sockets are not assigned to any transport)
> +	/* Check after acquiring the socket lock. Listener sockets accept packets
> +	 * from any source and are not assigned to a transport.

We can probably remove the first sentence since the function-level
comment already declares we are holding the socket lock.

Best,
Bobby

>  	 */
>  	if (sock_flag(sk, SOCK_DONE) ||
>  	    (sk->sk_state != TCP_LISTEN &&
> -	     !vsock_check_source(vsk, &t->transport, &src))) {
> +	     !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;
> +		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 = dst->svm_cid;
>  
>  	if (space_available)
>  		sk->sk_write_space(sk);
> @@ -1875,12 +1891,49 @@ void virtio_transport_recv_pkt(struct virtio_transport *t,
>  		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_transport_rx_pkt_ctx ctx;
> +	struct sockaddr_vm src, dst;
> +	struct sock *sk;
> +	bool free_pkt;
> +
> +	virtio_transport_recv_pkt_init_addrs(skb, &src, &dst);
> +	virtio_transport_trace_recv_pkt(skb, &src, &dst);
> +
> +	sk = virtio_transport_recv_pkt_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);
> +	ctx = (struct virtio_transport_rx_pkt_ctx) {
> +		.net = net,
> +		.src = &src,
> +		.dst = &dst,
> +	}
> +	free_pkt = virtio_transport_recv_pkt_locked(t, skb, sk, &ctx);
>  	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.34.1
>