>
> 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 <[email protected]>
> > ---
> > 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.
>
Sure, I'll update this in the next version.
> > {
> > 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.
>
Sure, I'll update this in the next version.
> >
> > - 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?
>
Sure, I'll update this in the next version
> 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.
>
Sure, I'll update this in the next version
> >
> > - 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...
>
Sure, I'll update this in the next version.
> >
> > - 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.
>
Sure, I'll update this in the next version.
> 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
> >
