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

[Bobby Eshleman <bobbyeshleman@gmail.com>]

On Fri, Oct 16, 2026 at 03:47:33PM +0200, Stefano Garzarella wrote:
> 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?

I think you may be remembering this issue:

a6ae12a599e0 ("virtio: set skb owner of virtio_transport_reset_no_sock() reply")

?

If so, sock_net(sk) might still be okay, but I 100% agree we shouldn't
change things haphazardly if avoidable.

> 
> > +		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
> > 
>