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

[Bobby Eshleman <bobbyeshleman@gmail.com>]

On Tue, Nov 18, 2025 at 07:10:28PM +0100, Stefano Garzarella wrote:
> On Mon, Nov 17, 2025 at 06:00:26PM -0800, Bobby Eshleman wrote:
> > From: Bobby Eshleman <bobbyeshleman@meta.com>
> > 
> > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> > index 2c937a2df83b..c8319cd1c232 100644
> > --- a/drivers/vhost/vsock.c
> > +++ b/drivers/vhost/vsock.c
> > @@ -64,6 +64,11 @@ static u32 vhost_transport_get_local_cid(void)
> > 	return VHOST_VSOCK_DEFAULT_HOST_CID;
> > }
> > 
> > +static bool vhost_transport_supports_local_mode(void)
> > +{
> > +	return true;
> 
> Should we enable this later, when we really add support, or it doesn't
> affect anything if vhost-vsock is not really supporting it in this PR
> (thinking about bisection issues).

sgtm!

> 
> > +}
> > +
> > /* Callers that dereference the return value must hold vhost_vsock_mutex or the
> >  * RCU read lock.
> >  */
> > @@ -412,6 +417,7 @@ static struct virtio_transport vhost_transport = {
> > 		.module                   = THIS_MODULE,
> > 
> > 		.get_local_cid            = vhost_transport_get_local_cid,
> > +		.supports_local_mode	  = vhost_transport_supports_local_mode,
> > 
> > 		.init                     = virtio_transport_do_socket_init,
> > 		.destruct                 = virtio_transport_destruct,
> > diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
> > index 59d97a143204..824d89657d41 100644
> > --- a/include/net/af_vsock.h
> > +++ b/include/net/af_vsock.h
> > @@ -180,6 +180,11 @@ struct vsock_transport {
> > 	/* Addressing. */
> > 	u32 (*get_local_cid)(void);
> > 
> > +	/* Return true if this transport supports VSOCK_NET_MODE_LOCAL.
> 
> nit: Here I would make it clearer that rather than supporting MODE_LOCAL,
> the transport is not compatible with it, etc.
> A summary of the excellent description we have in the commit.
> 

sounds good!

> > +	 * Otherwise, return false.
> > +	 */
> > +	bool (*supports_local_mode)(void);
> > +
> > 	/* Read a single skb */
> > 	int (*read_skb)(struct vsock_sock *, skb_read_actor_t);
> > 
> > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> > index 54373ae101c3..7a235bb94437 100644
> > --- a/net/vmw_vsock/af_vsock.c
> > +++ b/net/vmw_vsock/af_vsock.c
> > @@ -91,6 +91,12 @@
> >  *   and locked down by a namespace manager. The default is "global". The mode
> >  *   is set per-namespace.
> >  *
> > + *   Note: LOCAL mode is only supported when using namespace-aware transports
> > + *   (vhost-vsock, loopback). If a guest-to-host transport (virtio-vsock,
> > + *   hyperv-vsock, vmci-vsock) is loaded, attempts to set LOCAL mode will fail
> > + *   with EOPNOTSUPP, as these transports do not support per-namespace
> > + *   isolation.
> 
> Okay, maybe this is fine, so if you don't need to resend, feel free to
> ignore the previous comment.
> 
> > + *
> >  *   The modes affect the allocation and accessibility of CIDs as follows:
> >  *
> >  *   - global - access and allocation are all system-wide
> > @@ -2765,17 +2771,30 @@ static int vsock_net_mode_string(const struct ctl_table *table, int write,
> > 	if (*lenp >= sizeof(data))
> > 		return -EINVAL;
> > 
> > -	if (!strncmp(data, VSOCK_NET_MODE_STR_GLOBAL, sizeof(data)))
> > +	ret = 0;
> 
> IIUC `ret` should already be 0 at this point, no?
> 
> > +	mutex_lock(&vsock_register_mutex);
> 
> I honestly don't like to mix the parsing, with this new check, so what
> about leaving the parsing as before this patch (also without the mutex),
> then just (untested):
> 
> 	mutex_lock(&vsock_register_mutex);
> 	if (mode == VSOCK_NET_MODE_LOCAL && transport_g2h &&
> 	    transport_g2h->supports_local_mode &&
> 	    !transport_g2h->supports_local_mode()) {
> 		ret = -EOPNOTSUPP;
> 		goto out;
> 	}
> 
> 	if (!vsock_net_write_mode(net, mode)) {
> 		ret = -EPERM;
> 	}
> out:
> 	mutex_unlock(&vsock_register_mutex);
> 	return ret;
> }

Makes sense, I can move that around for next rev.

> 
> > +	if (!strncmp(data, VSOCK_NET_MODE_STR_GLOBAL, sizeof(data))) {
> > 		mode = VSOCK_NET_MODE_GLOBAL;
> > -	else if (!strncmp(data, VSOCK_NET_MODE_STR_LOCAL, sizeof(data)))
> > +	} else if (!strncmp(data, VSOCK_NET_MODE_STR_LOCAL, sizeof(data))) {
> > +		if (transport_g2h && transport_g2h->supports_local_mode &&
> > +		    !transport_g2h->supports_local_mode()) {
> > +			ret = -EOPNOTSUPP;
> > +			goto out;
> > +		}
> > 		mode = VSOCK_NET_MODE_LOCAL;
> > -	else
> > -		return -EINVAL;
> > +	} else {
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > 
> > -	if (!vsock_net_write_mode(net, mode))
> > -		return -EPERM;
> > +	if (!vsock_net_write_mode(net, mode)) {
> > +		ret = -EPERM;
> > +		goto out;
> > +	}
> > 
> > -	return 0;
> > +out:
> > +	mutex_unlock(&vsock_register_mutex);
> > +	return ret;
> > }
> > 
> > static struct ctl_table vsock_table[] = {
> > @@ -2916,6 +2935,7 @@ int vsock_core_register(const struct vsock_transport *t, int features)
> > {
> > 	const struct vsock_transport *t_h2g, *t_g2h, *t_dgram, *t_local;
> > 	int err = mutex_lock_interruptible(&vsock_register_mutex);
> > +	struct net *net;
> > 
> > 	if (err)
> > 		return err;
> > @@ -2938,6 +2958,22 @@ int vsock_core_register(const struct vsock_transport *t, int features)
> > 			err = -EBUSY;
> > 			goto err_busy;
> > 		}
> > +
> > +		/* G2H sockets break in LOCAL mode namespaces because G2H
> > +		 * transports don't support them yet. Block registering new G2H
> > +		 * transports if we already have local mode namespaces on the
> > +		 * system.
> > +		 */
> > +		rcu_read_lock();
> > +		for_each_net_rcu(net) {
> > +			if (vsock_net_mode(net) == VSOCK_NET_MODE_LOCAL) {
> > +				rcu_read_unlock();
> > +				err = -EOPNOTSUPP;
> > +				goto err_busy;
> > +			}
> > +		}
> > +		rcu_read_unlock();
> > +
> > 		t_g2h = t;
> > 	}
> > 
> > diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
> > index 432fcbbd14d4..279f04fcd81a 100644
> > --- a/net/vmw_vsock/hyperv_transport.c
> > +++ b/net/vmw_vsock/hyperv_transport.c
> > @@ -833,10 +833,16 @@ int hvs_notify_set_rcvlowat(struct vsock_sock *vsk, int val)
> > 	return -EOPNOTSUPP;
> > }
> > 
> > +static bool hvs_supports_local_mode(void)
> > +{
> > +	return false;
> > +}
> > +
> > static struct vsock_transport hvs_transport = {
> > 	.module                   = THIS_MODULE,
> > 
> > 	.get_local_cid            = hvs_get_local_cid,
> > +	.supports_local_mode      = hvs_supports_local_mode,
> > 
> > 	.init                     = hvs_sock_init,
> > 	.destruct                 = hvs_destruct,
> > diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> > index 5d379ccf3770..e585cb66c6f5 100644
> > --- a/net/vmw_vsock/virtio_transport.c
> > +++ b/net/vmw_vsock/virtio_transport.c
> > @@ -94,6 +94,18 @@ static u32 virtio_transport_get_local_cid(void)
> > 	return ret;
> > }
> > 
> > +static bool virtio_transport_supports_local_mode(void)
> > +{
> > +	struct virtio_vsock *vsock;
> > +
> > +	rcu_read_lock();
> > +	vsock = rcu_dereference(the_virtio_vsock);
> > +	rcu_read_unlock();
> > +
> > +	/* Local mode is supported only when no G2H device is present. */
> > +	return vsock ? false : true;
> > +}
> > +
> > /* Caller need to hold vsock->tx_lock on vq */
> > static int virtio_transport_send_skb(struct sk_buff *skb, struct virtqueue *vq,
> > 				     struct virtio_vsock *vsock, gfp_t gfp)
> > @@ -544,6 +556,7 @@ static struct virtio_transport virtio_transport = {
> > 		.module                   = THIS_MODULE,
> > 
> > 		.get_local_cid            = virtio_transport_get_local_cid,
> > +		.supports_local_mode      = virtio_transport_supports_local_mode,
> > 
> > 		.init                     = virtio_transport_do_socket_init,
> > 		.destruct                 = virtio_transport_destruct,
> > diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
> > index 7eccd6708d66..da7c52ad7b2a 100644
> > --- a/net/vmw_vsock/vmci_transport.c
> > +++ b/net/vmw_vsock/vmci_transport.c
> > @@ -2033,6 +2033,12 @@ static u32 vmci_transport_get_local_cid(void)
> > 	return vmci_get_context_id();
> > }
> > 
> > +static bool vmci_transport_supports_local_mode(void)
> > +{
> > +	/* Local mode is supported only when no device is present. */
> > +	return vmci_transport_get_local_cid() == VMCI_INVALID_ID;
> 
> IIRC vmci can be registered both as H2G and G2H, so should we filter out
> the H2G case?

In fact, I'm realizing now that this should probably just be:

static bool vmci_transport_supports_local_mode(void)
{
	return false;
}


... because even for H2G there is no mechanism for attaching a namespace
to a VM (unlike w/ vhost_vsock device open).

Does that seem right?

Best,
Bobby