Re: [PATCH net-next v11 03/13] vsock: reject bad VSOCK_NET_MODE_LOCAL configuration for G2H

[Bobby Eshleman <bobbyeshleman@gmail.com>]

On Mon, Nov 24, 2025 at 06:54:45PM +0100, Stefano Garzarella wrote:
> On Mon, Nov 24, 2025 at 09:29:05AM -0800, Bobby Eshleman wrote:
> > On Mon, Nov 24, 2025 at 11:10:19AM +0100, Stefano Garzarella wrote:
> > > On Fri, Nov 21, 2025 at 11:01:53AM -0800, Bobby Eshleman wrote:
> > > > On Fri, Nov 21, 2025 at 03:24:25PM +0100, Stefano Garzarella wrote:
> > > > > On Thu, Nov 20, 2025 at 09:44:35PM -0800, Bobby Eshleman wrote:
> 
> [...]
> 
> > > >
> > > > > Since I guess we need another version of this patch, can you check the
> > > > > commit description to see if it reflects what we are doing now
> > > > > (e.g vhost is not enabled)?
> > > > >
> > > > > Also I don't understand why for vhost we will enable it later, but for
> > > > > virtio_transport and vsock_loopback we are enabling it now, also if this
> > > > > patch is before the support on that transports. I'm a bit confused.
> > > > >
> > > > > If something is unclear, let's discuss it before sending a new version.
> > > > >
> > > > >
> > > > > What I had in mind was, add this patch and explain why we need this new
> > > > > callback (like you did), but enable the support in the patches that
> > > > > really enable it for any transport. But maybe what is not clear to me is
> > > > > that we need this only for G2H. But now I'm confused about the discussion
> > > > > around vmci H2G. We decided to discard also that one, but here we are not
> > > > > checking that?
> > > > > I mean here we are calling supports_local_mode() only on G2H IIUC.
> > > >
> > > > Ah right, VMCI broke my original mental model of only needing this check
> > > > for G2H (originally I didn't realize VMCI was H2G too).
> > > >
> > > > I think now, we actually need to do this check for all of the transports
> > > > no? Including h2g, g2h, local, and dgram?
> > > >
> > > > Additionally, the commit description needs to be updated to reflect that.
> > > 
> > > Let's take a step back, though, because I tried to understand the problem
> > > better and I'm confused.
> > > 
> > > For example, in vmci (G2H side), when a packet arrives, we always use
> > > vsock_find_connected_socket(), which only searches in GLOBAL. So connections
> > > originating from the host can only reach global sockets in the guest. In
> > > this direction (host -> guest), we should be fine, right?
> > > 
> > > Now let's consider the other direction, from guest to host, so the
> > > connection should be generated via vsock_connect().
> > > Here I see that we are not doing anything with regard to the source
> > > namespace. At this point, my question is whether we should modify
> > > vsock_assign_transport() or transport->stream_allow() to do this for each
> > > stream, and not prevent loading a G2H module a priori.
> > > 
> > > For example, stream_allow() could check that the socket namespace is
> > > supported by the assigned transport. E.g., vmci can check that if the
> > > namespace mode is not GLOBAL, then it returns false. (Same thing in
> > > virtio-vsock, I mean the G2H driver).
> > > 
> > > This should solve the guest -> host direction, but at this point I wonder if
> > > I'm missing something.
> > 
> > For the G2H connect case that is true, but the situation gets a little
> > fuzzier on the G2H RX side w/ VMADDR_CID_ANY listeners.
> > 
> > Let's say we have a nested system w/ both virtio-vsock and vhost-vsock.
> > We have a listener in namespace local on VMADDR_CID_ANY. So far, no
> > transport is assigned, so we can't call t->stream_allow() yet.
> > virtio-vsock only knows of global mode, so its lookup will fail (unless
> 
> What is the problem of failing in this case?
> I mean, we are documenting that G2H will not be able to reach socket in
> namespaces with "local" mode. Old (and default) behaviour is still allowing
> them, right?
> 
> I don't think it conflicts with the definition of “local” either, because
> these connections are coming from outside, and the user doesn't expect to be
> able to receive them in a “local” namespace, unless there is a way to put
> the device in the namespace (as with net). But this method doesn't exist
> yet, and by documenting it sufficiently, we can say that it will be
> supported in the future, but not for now.
> 
> > we hack in some special case to virtio_transport_recv_pkt() to scan
> > local namespaces). vhost-vsock will work as expected. Letting local mode
> > sockets be silently unreachable by virtio-vsock seems potentially
> > confusing for users. If the system were not nested, we can pre-resolve
> > VMADDR_CID_ANY in bind() and handle things upfront as well. Rejecting
> > local mode outright is just a broad guardrail.
> 
> Okay, but in that case, we are not supporting “local” mode too, but we are
> also preventing “global” from being used on these when we are in a nested
> environment. What is the advantage of this approach?
> 
> > 
> > If we're trying to find a less heavy-handed option, we might be able to
> > do the following:
> > 
> > - change bind(cid) w/ cid != VMADDR_CID_ANY to directly assign the
> > transport
> >  for all socket types (not just SOCK_DGRAM)
> 
> That would be nice, but it wouldn't solve the problem with VMADDR_CID_ANY,
> which I guess is the use case in 99% of cases.
> 
> > 
> > - vsock_assign_transport() can outright fail if !t->supports_local_mode()
> >  and sock_net(sk) has mode local
> 
> But in this case, why not reusing stream_allow() ?
> 
> > 
> > - bind(VMADDR_CID_ANY) can maybe print (once) to dmesg a warning that
> >  only the H2G transport will land on VMADDR_CID_ANY sockets.
> 
> mmm, I'm not sure about that, we should ask net maintainer, but IMO
> documenting that in af_vsock.c and man pages should be fine, till G2H will
> support that.
> 
> > 
> > I'm certainly open to other suggestions.
> 
> IMO we should avoid the failure when loading G2H, which is more confusing
> than just discard connection from the host to a "local" namespace. We should
> try at least to support the "global" namespace.
> 
> Thanks,
> Stefano


I'm 100% fine with that approach. I just wanted to make sure we landed
in the right place for how users may encounter places that there is no
local mode support.

So for next steps, we can drop this patch and add explicit logic in
->stream_allow() to allow local mode for vhost/loopback and reject for
others? Plus, add documentation about what happens for VMADDR_CID_ANY
(will only receive vhost/loopback traffic in local mode)?

Best,
Bobby