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

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