Re: [PATCH net-next v5 3/9] vsock: add netns to vsock core

[Bobby Eshleman <bobbyeshleman@gmail.com>]

On Tue, Sep 02, 2025 at 05:39:10PM +0200, Stefano Garzarella wrote:
> On Wed, Aug 27, 2025 at 05:31:31PM -0700, Bobby Eshleman wrote:
> > From: Bobby Eshleman <bobbyeshleman@meta.com>

...

> > {
> > 	enum vsock_net_mode ret;
> > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> > index 0538948d5fd9..68a8875c8106 100644
> > --- a/net/vmw_vsock/af_vsock.c
> > +++ b/net/vmw_vsock/af_vsock.c
> > @@ -83,6 +83,24 @@
> >  *   TCP_ESTABLISHED - connected
> >  *   TCP_CLOSING - disconnecting
> >  *   TCP_LISTEN - listening
> > + *
> > + * - Namespaces in vsock support two different modes configured
> > + *   through /proc/sys/net/vsock/ns_mode. The modes are "local" and "global".
> > + *   Each mode defines how the namespace interacts with CIDs.
> > + *   /proc/sys/net/vsock/ns_mode is write-once, so that it may be configured
> > + *   and locked down by a namespace manager. The default is "global". The mode
> > + *   is set per-namespace.
> > + *
> > + *   The modes affect the allocation and accessibility of CIDs as follows:
> > + *   - global - aka fully public
> > + *      - CID allocation draws from the public pool
> > + *      - AF_VSOCK sockets may reach any CID allocated from the public pool
> > + *      - AF_VSOCK sockets may not reach CIDs allocated from private
> > pools
> 
> Should we define what public and private pools are?
> 
> What I found difficult to understand was the allocation of CIDs, meaning I
> had to reread it two or three times to perhaps understand it.
> 
> IIUC, netns with mode=global can only allocate public CIDs, while netns with
> mode=local can only allocate private CIDs, right?
> 

Correct.

> Perhaps we should first better define how CIDs are allocated and then
> explain the interaction between them.
> 

Makes sense, I'll clarify that.

> > + *
> > + *   - local - aka fully private
> > + *     - CID allocation draws only from the private pool, does not affect public pool
> > + *     - AF_VSOCK sockets may only reach CIDs from the private pool
> > + *     - AF_VSOCK sockets may not reach CIDs allocated from outside the pool
> 
> Why using "may" ? I mean, can be cases when this is not true?
> 


Good point, will change to stronger language since it is always true.

[...]

> > 
> > @@ -2636,6 +2670,137 @@ static struct miscdevice vsock_device = {
> > 	.fops		= &vsock_device_ops,
> > };
> > 
> > +#define VSOCK_NET_MODE_STRING_MAX 7
> > +
> > +static int vsock_net_mode_string(const struct ctl_table *table, int write,
> > +				 void *buffer, size_t *lenp, loff_t *ppos)
> > +{
> > +	char buf[VSOCK_NET_MODE_STRING_MAX] = {0};
> 
> Can we change `buf` name?
> 
> I find it confusing to have both a `buffer` variable and a `buf` variable in
> the same function.
> 

Makes sense, will do.

> > +	enum vsock_net_mode mode;
> > +	struct ctl_table tmp;
> > +	struct net *net;
> > +	const char *p;
> 
> Can we move `p` declaration in the `if (!write) {` block?
> 

yes.

> > +	int ret;
> > +
> > +	if (!table->data || !table->maxlen || !*lenp) {
> > +		*lenp = 0;
> > +		return 0;
> > +	}
> > +
> > +	net = current->nsproxy->net_ns;
> > +	tmp = *table;
> > +	tmp.data = buf;
> > +
> > +	if (!write) {
> > +		mode = vsock_net_mode(net);
> > +
> > +		if (mode == VSOCK_NET_MODE_GLOBAL) {
> > +			p = "global";
> > +		} else if (mode == VSOCK_NET_MODE_LOCAL) {
> > +			p = "local";
> > +		} else {
> > +			WARN_ONCE(true, "netns has invalid vsock mode");
> > +			*lenp = 0;
> > +			return 0;
> > +		}
> > +
> > +		strscpy(buf, p, sizeof(buf));
> > +		tmp.maxlen = strlen(p);
> > +	}
> > +
> > +	ret = proc_dostring(&tmp, write, buffer, lenp, ppos);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (write) {
> > +		if (!strncmp(buffer, "global", 6))
> 
> Are we sure that the `buffer` is at least 6 bytes long and NULL-terminated?
> 
> Maybe we can just check that `lenp <= sizeof(buf)`...
> 
> Should we add macros for "global" and "local" ?
> 

That all sounds reasonable. IIRC I tested with some garbage writes, but might
as well err on the side of caution.

> 
> > +			mode = VSOCK_NET_MODE_GLOBAL;
> > +		else if (!strncmp(buffer, "local", 5))
> > +			mode = VSOCK_NET_MODE_LOCAL;
> > +		else
> > +			return -EINVAL;
> > +
> > +		if (!vsock_net_write_mode(net, mode))
> > +			return -EPERM;
> > +	}
> > +
> > +	return 0;
> > +}
> > +

...


Thanks for the review!

Best,
Bobby