Re: [PATCH RFC iproute2-next 2/2] ip: add "ip vsock" to move the guest vsock device to a netns

[Bobby Eshleman <bobbyeshleman@meta.com>]

On Fri, Nov 06, 2026 at 10:36:46AM +0100, Stephen Hemminger wrote:
> On Thu, 5 Nov 2026 11:30:36 -0800
> Bobby Eshleman <bobbyeshleman@meta.com> wrote:
> 
> > The guest's vsock device is not a netdev, so rtnetlink cannot move it
> > between network namespaces. Add an "ip vsock" object that goes through
> > the "vsock" generic netlink family instead:
> > 
> >   ip vsock set netns { PID | NETNSNAME | NETNSFILE }
> >   ip vsock show
> > 
> > "set" issues VSOCK_CMD_DEV_NETNS_SET from inside the target namespace,
> > since the kernel moves the device to the namespace the request was sent
> > from. "show" issues VSOCK_CMD_DEV_NETNS_GET and prints where the device
> > is, relative to the current namespace.
> > 
> > Link: https://lore.kernel.org/all/20261103-vsock-guest-ns-v5-0-48179b9b66b8@meta.com/
> > Signed-off-by: Bobby Eshleman <bobbyeshleman@meta.com>
> > ---
> 
> Semi-automated review using review-prompts for iproute2
> 
> 
> On Thu, 5 Nov 2026 11:30:36 -0800, Bobby Eshleman wrote:
> 
> Thanks, this is mostly in good shape. A few things to fix before it
> goes beyond RFC.
> 
> > +#include <linux/vsock_netlink.h>
> 
> This depends on a uapi header (and kernel series) that is not yet
> merged. iproute2-next can only take this once the kernel side is in
> net-next; the header update should then come from the normal uapi
> sync rather than a hand-added copy in patch 1/2.
> 
> > +static int vsock_talk(__u8 cmd, struct nlmsghdr **answer)
> > +{
> > +	GENL_REQUEST(req, 128, 0, 0, VSOCK_FAMILY_VERSION, cmd, NLM_F_REQUEST);
> > +	struct rtnl_handle grth = { .fd = -1 };
> > +	int family = -1;
> > +	int ret;
> > +
> > +	if (genl_init_handle(&grth, VSOCK_FAMILY_NAME, &family)) {
> > +		fprintf(stderr,
> > +			"Cannot resolve the \"%s\" netlink family, the kernel may not support moving the vsock device.\n",
> > +			VSOCK_FAMILY_NAME);
> > +		return -1;
> > +	}
> 
> genl_init_handle() leaves the socket open when genl_resolve_family()
> fails, so this leaks grth.fd. Harmless today since ip exits, but not
> under "ip -batch". Add rtnl_close(&grth) on this path.
> 
> Also genl_resolve_family() already prints "Error talking to the kernel"
> on failure, so the user gets two messages. Not a big deal, just be aware.
> 
> > +static void vsock_print_netnsid(int id)
> > +{
> > +	char *name;
> > +
> > +	if (is_json_context()) {
> > +		print_int(PRINT_JSON, "netnsid", NULL, id);
> > +		return;
> > +	}
> 
> This copies the IFLA_LINK_NETNSID pattern from ipaddress.c, which is
> fine for consistency, but note that JSON users never get the netns
> name and get -1 for "unknown". Consider also emitting
> print_string(PRINT_JSON, "netns", NULL, name) when a name exists.
> 
> > +int do_ipvsock(int argc, char **argv)
> > +{
> > +	if (argc < 1)
> > +		return vsock_dev_netns_show();
> > +
> > +	if (matches(*argv, "show") == 0 ||
> > +	    matches(*argv, "list") == 0) {
> ...
> > +	if (matches(*argv, "set") == 0)
> ...
> > +	if (matches(*argv, "help") == 0)
> > +		usage();
> 
> and in vsock_set():
> 
> > +		} else if (matches(*argv, "help") == 0) {
> 
> New code must use strcmp(), not matches(). Abbreviation matching has
> caused repeated breakage when keywords are added later (e.g. "s" here
> already matches both "show" and "set"; order decides which wins).
> Please convert all of these to strcmp().
> 
> > +	ret = rtnl_talk(&grth, &req.n, answer);
> > +	rtnl_close(&grth);
> > +
> > +	return ret < 0 ? -2 : 0;
> 
> The -2 propagates out as the exit status of ip (254). Other ip
> subcommands return -1/1 on failure; either is fine but please be
> consistent with vsock_dev_netns_set() which returns -1 for its own
> errors and -2 for a kernel error.
> 
> > +		"Usage:	ip vsock set netns { PID | NETNSNAME | NETNSFILE }\n"
> > +		"	ip vsock show\n");
> 
> Literal tabs in the usage string; the rest of ip uses spaces for
> alignment here ("Usage: ip vsock ...\n       ip vsock show\n").
> Also "ip vsock help" should be listed.
> 
> > +static int vsock_dev_netns_set(const char *netns)
> > +{
> ...
> > +	if (setns(netns_fd, CLONE_NEWNET) < 0) {
> 
> Design note, more for the kernel side than this patch: because
> VSOCK_CMD_DEV_NETNS_SET has no target attribute, userspace has to do a
> setns() dance, and "ip -n foo vsock set netns bar" ends up depending on
> which namespace the genl socket was opened in. rtnetlink and devlink
> both take an explicit NETNS_FD / NETNS_PID / NETNS_ID attribute for
> this; doing the same for vsock would make the tool trivial and avoid
> needing setns() (and the privileges to enter the target ns) at all.
> Since this is RFC on both sides it seems worth reconsidering now.

This does make sense, we'll use this approach in the kernel.

> 
> If the kernel API stays as is, the setns/restore logic looks correct.
> 
> > +ip vsock set netns 1
> > +.RS 4
> > +Move the device back to the initial network namespace, undoing the assignment.
> 
> That only holds if ip is run in the initial pid namespace; "1" is a
> PID, not "the initial netns". Maybe say so, or use /proc/1/ns/net.
> 
> Minor: the man page says "Nothing follows vsock when the device is in
> the current namespace" -- worth also showing an example of the
> "netnsid unknown" output so users know what it means.

Will address other feedback too.

Best,
Bobby