This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible.
For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html --- vsock: add G2H fallback for CIDs not owned by H2G transport This commit enables G2H transport fallback for CIDs not claimed by the H2G transport in environments like Nitro Enclaves. It adds a per-netns sysctl net.vsock.g2h_fallback (default 1) and a has_remote_cid callback for transports to declare CID ownership. > The flag was introduced 6 years ago and I am still not aware of any tool This isn't a bug, but the timing is slightly inaccurate. VMADDR_FLAG_TO_HOST was introduced in commit caaf95e0f23f on 2020-12-14. As of March 2026, this is approximately 5 years and 3 months, not 6 years. > diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h > index 533d8e75f7bb..4e40063adab4 100644 > --- a/include/net/af_vsock.h > +++ b/include/net/af_vsock.h > @@ -179,6 +179,15 @@ struct vsock_transport { > /* Addressing. */ > u32 (*get_local_cid)(void); > > + /* Check if this transport serves a specific remote CID. > + * For H2G transports: return true if the CID belongs to a registered > + * guest. If not implemented, all CIDs > VMADDR_CID_HOST go to H2G. > + * For G2H transports: return true if the transport can reach arbitrary > + * CIDs via the hypervisor (i.e. supports the fallback overlay). VMCI > + * does not implement this as it only serves CIDs 0 and 2. > + */ > + bool (*has_remote_cid)(struct vsock_sock *vsk, u32 remote_cid); > + The G2H transport description doesn't mention a critical consequence: G2H transports that do not implement has_remote_cid (like hyperv_transport) will never receive fallback traffic even with g2h_fallback=1, because the fallback branch in vsock_assign_transport() requires transport_g2h->has_remote_cid to be non-NULL. Should the comment clarify this requirement for G2H transports to participate in fallback? [ ... ] > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c > index f0ab2f13e9db..cc4b225250b9 100644 > --- a/net/vmw_vsock/af_vsock.c > +++ b/net/vmw_vsock/af_vsock.c > @@ -545,9 +545,13 @@ static void vsock_deassign_transport(struct vsock_sock > *vsk) > * The vsk->remote_addr is used to decide which transport to use: > * - remote CID == VMADDR_CID_LOCAL or g2h->local_cid or VMADDR_CID_HOST if > * g2h is not loaded, will use local transport; > - * - remote CID <= VMADDR_CID_HOST or h2g is not loaded or remote flags > field > - * includes VMADDR_FLAG_TO_HOST flag value, will use guest->host > transport; > - * - remote CID > VMADDR_CID_HOST will use host->guest transport; > + * - remote CID <= VMADDR_CID_HOST or remote flags field includes > + * VMADDR_FLAG_TO_HOST, will use guest->host transport; > + * - remote CID > VMADDR_CID_HOST and h2g is loaded and h2g claims that CID, > + * will use host->guest transport; > + * - h2g not loaded or h2g does not claim that CID and g2h claims the CID > via > + * has_remote_cid, will use guest->host transport (when g2h_fallback=1) > + * - anything else goes to h2g or returns -ENODEV if no h2g is available > */ > int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk) > { The commit message states this patch extends the existing behavior: "When no H2G transport is loaded, vsock currently routes all CIDs to the G2H transport...Extend that existing behavior: when an H2G transport is loaded but does not claim a given CID, the connection falls back to G2H in the same way." Does this accurately describe what the patch does? > @@ -581,11 +585,21 @@ int vsock_assign_transport(struct vsock_sock *vsk, > struct vsock_sock *psk) > case SOCK_SEQPACKET: > if (vsock_use_local_transport(remote_cid)) > new_transport = transport_local; > - else if (remote_cid <= VMADDR_CID_HOST || !transport_h2g || > + else if (remote_cid <= VMADDR_CID_HOST || > (remote_flags & VMADDR_FLAG_TO_HOST)) > new_transport = transport_g2h; > - else > + else if (transport_h2g && > + (!transport_h2g->has_remote_cid || > + transport_h2g->has_remote_cid(vsk, remote_cid))) > + new_transport = transport_h2g; > + else if (sock_net(sk)->vsock.g2h_fallback && > + transport_g2h && transport_g2h->has_remote_cid && > + transport_g2h->has_remote_cid(vsk, remote_cid)) { > + vsk->remote_addr.svm_flags |= VMADDR_FLAG_TO_HOST; > + new_transport = transport_g2h; > + } else { > new_transport = transport_h2g; > + } > break; The old code had `!transport_h2g` in the second else-if condition at line -583, which routed all CID > VMADDR_CID_HOST traffic to transport_g2h when no H2G transport was loaded. The new code removes this check. When transport_h2g is NULL (no H2G transport loaded) and the G2H transport does not implement has_remote_cid (as is the case for VMCI and Hyper-V transports), what happens? The G2H fallback branch at line +596 requires `transport_g2h->has_remote_cid` to be non-NULL. If it's NULL, the short-circuit evaluation skips this branch and executes the final else at line +601, setting `new_transport = transport_h2g` (which is NULL), ultimately returning -ENODEV. The commit message explicitly states the intent to preserve the existing no-H2G fallback behavior. Does this implementation break it for G2H transports that do not implement the new optional has_remote_cid callback? This affects VMCI and Hyper-V environments: when no H2G transport is loaded, CID > 2 connections that previously fell back to G2H now get ENODEV. [ ... ] > @@ -2928,6 +2952,7 @@ static void vsock_net_init(struct net *net) > net->vsock.mode = > vsock_net_child_mode(current->nsproxy->net_ns); > > net->vsock.child_ns_mode = net->vsock.mode; > + net->vsock.g2h_fallback = 1; > } Stefano Garzarella raised a question in v3 about the sysctl inheritance policy for g2h_fallback: should it inherit from the parent namespace or always default to 1 in all namespaces? He cc'd net maintainers (Michael, Paolo, Jakub) for guidance: https://lore.kernel.org/all/aahRzq5vS76rPI28@sgarzare-redhat/ The current code always defaults to 1 for all namespaces (including child namespaces), which differs from how ns_mode is handled (which inherits from parent). Was there a resolution to this question? [ ... ] > diff --git a/net/vmw_vsock/virtio_transport.c > b/net/vmw_vsock/virtio_transport.c > index 77fe5b7b066c..57f2d6ec3ffc 100644 > --- a/net/vmw_vsock/virtio_transport.c > +++ b/net/vmw_vsock/virtio_transport.c > @@ -547,11 +547,18 @@ bool virtio_transport_stream_allow(struct vsock_sock > *vsk, u32 cid, u32 port) > static bool virtio_transport_seqpacket_allow(struct vsock_sock *vsk, > u32 remote_cid); > > +static bool virtio_transport_has_remote_cid(struct vsock_sock *vsk, u32 cid) > +{ > + /* The CID could be implemented by the host. Always assume it is. */ > + return true; > +} > + The comment says "always assume it is" implemented by the host. Is this a verifiable fact or a design choice? The guest cannot know what CIDs the host or hypervisor can reach, so this is a reasonable design choice. However, it also means all unclaimed CIDs fall back to G2H when virtio is the G2H transport, which could silently create host vsock traffic for typos or invalid CIDs.
