Re: [net-next, v4] vsock: add G2H fallback for CIDs not owned by H2G transport

[Alexander Graf]

On 10.03.26 10:26, Paolo Abeni wrote:
> Hi,
>
> On 3/10/26 10:18 AM, Paolo Abeni wrote:
> > 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
> > ---
> > > @@ -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.
> Other comments from AI looked like nit picking to me, but the above one
> looks relevant. I forwarded verbatim all the feedback for completeness.


Yes, this is on purpose. Instead of having bi-modality based on whether 
the vhost_vsock kernel module is loaded, we now clearly define that with 
the sysctl off, you get the same mode that "vhost_vsock is loaded" gave 
you before. The oddball case of "vhost_vsock is not loaded" that meant 
everything goes to the G2H transport and confused users is really not 
worth keeping IMHO. It's replaced by the auto-fallback logic.

Broadcom also mentioned that G2H for CID > 2 does not exist. IIUC 
Hyper-V also does not support CID > 2.


Alex




Amazon Web Services Development Center Germany GmbH
Tamara-Danz-Str. 13
10243 Berlin
Geschaeftsfuehrung: Christof Hellmis, Andreas Stieger
Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B
Sitz: Berlin
Ust-ID: DE 365 538 597