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 <[email protected]>
...
> > {
> > 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
