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 <[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?
>
> > + */
> > +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
> >
>
