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