Re: [PATCH] vsock: preserve child namespace mode on an empty write

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0

- [High] Incomplete fix: a zero-length write to
  /proc/sys/net/vsock/child_ns_mode still consumes the write-once child-…

Pre-existing issues:
- [Medium] Non-atomic publication in vsock_net_set_child_mode()
  (include/net/af_vsock.h:289-302): child_ns_mode_locked is published by…

--

vsock: preserve child namespace mode on an empty write

This initializes new_mode in vsock_net_child_mode_string() from the current
child mode, so the caller no longer reads an uninitialized enum when
__vsock_net_mode_string() returns success for a zero-length write.

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index f840498b58afb..9f89f199478b9 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -2888,6 +2888,7 @@ static int vsock_net_child_mode_string(const struct ctl_table *table, int write,
>  	int ret;
>  
>  	net = container_of(table->data, struct net, vsock.child_ns_mode);
> +	new_mode = vsock_net_child_mode(net);
>  
>  	ret = __vsock_net_mode_string(table, write, buffer, lenp, ppos,
>  				      vsock_net_child_mode(net), &new_mode);

[Severity: High]
The changelog says "Initialize new_mode from the current child mode so that
an empty write is a no-op."  Is the empty write really a no-op after this
change?

A zero-length write still reaches the handler with write == 1 and
*lenp == 0.  fs/read_write.c:vfs_write() has no count == 0 early return
(rw_verify_area() only rejects negative counts), and
fs/proc/proc_sysctl.c:proc_sys_call_handler() does not short-circuit
either:

	if (write) {
		error = -EFAULT;
		if (!copy_from_iter_full(kbuf, count, iter))
			goto out_free_buf;
		kbuf[count] = '\0';
	}
	...
	error = table->proc_handler(table, write, kbuf, &count, &iocb->ki_pos);

copy_from_iter_full(kbuf, 0, iter) returns true, so
vsock_net_child_mode_string() is entered with *lenp == 0.

__vsock_net_mode_string() then returns 0 without parsing anything:

	if (!table->data || !table->maxlen || !*lenp) {
		*lenp = 0;
		return 0;
	}

Since that return value is indistinguishable from "a mode was parsed",
vsock_net_child_mode_string() falls into its write block anyway:

	if (write) {
		...
		if (!vsock_net_set_child_mode(net, new_mode))
			return -EBUSY;
	}

so vsock_net_set_child_mode() is still called, now with the seeded current
mode rather than a stack value.

Does that still consume the write-once latch?  In
include/net/af_vsock.h:vsock_net_set_child_mode():

	int new_locked = mode + 1;
	int old_locked = 0; /* unlocked */

	if (try_cmpxchg(&net->vsock.child_ns_mode_locked,
			&old_locked, new_locked)) {
		WRITE_ONCE(net->vsock.child_ns_mode, mode);
		return true;
	}

On a fresh namespace vsock_net_init() leaves child_ns_mode_locked at 0, so
the cmpxchg succeeds and the mode is latched permanently.

Would this sequence then break a later legitimate lockdown on a global-mode
namespace?

	write(fd, "", 0);                     /* returns 0, latches GLOBAL+1 */
	echo local > .../child_ns_mode        /* cmpxchg fails, -EBUSY */

Documentation/admin-guide/sysctl/net.rst describes the behaviour as:

  The first write to ``child_ns_mode`` locks its value. Subsequent writes of
  the same value succeed, but writing a different value returns ``-EBUSY``.

which ties the lock and the -EBUSY to writing a mode value, not to a write
that supplies no value.

Would it be more in line with the changelog to skip the write handling
entirely when nothing was parsed, for example an early
"if (write && !*lenp) return 0;" in vsock_net_child_mode_string(), or having
__vsock_net_mode_string() report "no value parsed" distinctly from success,
instead of re-applying the current mode through the cmpxchg?

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but while
looking at vsock_net_set_child_mode() in include/net/af_vsock.h: is the
publication of the lock and of the mode value atomic with respect to
namespace creation?

	if (try_cmpxchg(&net->vsock.child_ns_mode_locked,
			&old_locked, new_locked)) {
		WRITE_ONCE(net->vsock.child_ns_mode, mode);
		return true;
	}

	return old_locked == new_locked;

The lock becomes observable before the child_ns_mode store, and a second
writer of the same mode returns success from the last line without waiting
for that store:

CPU0 (writes "local" on a global-mode netns)
	try_cmpxchg() succeeds, child_ns_mode_locked = LOCAL+1
	/* preempted here, before WRITE_ONCE(child_ns_mode, LOCAL) */

CPU1 (writes "local")
	try_cmpxchg() fails, old_locked == LOCAL+1 == new_locked
	return true;			/* sysctl write reports success */

CPU2 (creates a netns after CPU1's write returned)
net/vmw_vsock/af_vsock.c:vsock_net_init()
	net->vsock.mode = vsock_net_child_mode(current->nsproxy->net_ns);
	net->vsock.child_ns_mode = net->vsock.mode;

Can the new namespace read VSOCK_NET_MODE_GLOBAL here and permanently
inherit the more permissive mode even though userspace was told the "local"
lockdown was committed?

A barrier does not seem sufficient, since the reader has already observed
the lock publication that precedes the value store.  Would publishing the
mode and its locked state together help, for example deriving child_ns_mode
from child_ns_mode_locked, or adding an in-progress state that the second
writer waits on?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915111417.2726515-1-qwe.aldo%40gmail.com