>
> 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 <[email protected]>
> >---
> > 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?
>
You're right, it doesn't have to be. I originally left the skb cleanup
to the caller so it could unwind either per-packet or batch state
while preserving
the existing error-path order (release_sock() -> drop the lookup
reference -> kfree_skb()).
However, that distinction does not require skb ownership to escape the
helper, and the resulting ownership contract is confusing since the
other paths
consume the skb internally. I'll make the helper consume it directly in v5.
> >+ */
> >+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
>
Sorry about that. You had pointed me to this guideline before, and I
missed this during the final preparation. That's entirely my
oversight, and I will fix the declaration ordering throughout the
series in v5.
> >
> >- 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.
>
Yes, I changed it because lock_sock() moved to the caller, so the old
wording no longer read clearly at that location. But I over-shortened
it and dropped the important reason for the recheck. I'll restore that
explanation:
> > 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 assumed sock_net(sk) was interchangeable with the caller's net after
a successful lookup, but I took that equivalence for granted. I'll
change this in v5.
> >+ 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
> >
>
