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

[Jia Jia]

>
> 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@xxxxxxxxx>
> > ---
> >  net/vmw_vsock/virtio_transport_common.c | 139 ++++++++++++++++--------
> >  1 file changed, 96 insertions(+), 43 deletions(-)
> >
>
> [...]
>
> > +/*
> > + * 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.
> >        */
> >       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);
>
> At this point the socket lookup succeeded with src/dst/net, why not just
> pass sk which already has all of them?
>

If I understand this correctly, a successful lookup is not enough to
recover all of this from sk. The lookup may fall back from the
connected table to the bound
table, so the packet source must still be validated under the socket
lock; otherwise that check would become ineffective. The packet
destination is also needed after
a transport reset, and the RX namespace used for a reset reply is not
necessarily sock_net(sk) in global mode. I’ll remove the context
struct and pass src, dst, and
net directly.

> Best,
> Bobby