Re: [PATCH net-next v6 5/8] vsock: add a netlink command to assign the g2h device to a netns

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new `vsock` generic netlink family defines `dev-netns-set`
  (VSOCK_CMD_DEV_NETNS_SET) as a do-request with no reply.

--

vsock: add a netlink command to assign the g2h device to a netns

This patch adds a vsock generic netlink family. VSOCK_CMD_DEV_NETNS_SET
moves the guest's G2H vsock device into a given network namespace, and
VSOCK_CMD_DEV_NETNS_GET reports the assignment as an nsid relative to
the caller. Connected sockets that can no longer reach the device are
closed with ECONNRESET, and an assignment that would cause a CID
collision is refused.

Link: https://lore.kernel.org/all/20200427142518.uwssa6dtasrp3bfc@steredhat/

> diff --git a/Documentation/netlink/specs/vsock.yaml b/Documentation/netlink/specs/vsock.yaml
> new file mode 100644
> index 0000000000000..f260ee88b1402
> --- /dev/null
> +++ b/Documentation/netlink/specs/vsock.yaml

[ ... ]

> +operations:
> +  list:
> +    -
> +      name: dev-netns-set
> +      doc: |
> +        Set the network namespace of the guest's vsock device.

[ ... ]

> +        The assignment holds a reference on the namespace.
> +      attribute-set: vsock
> +      flags: [admin-perm]
> +      do:
> +        request:
> +          attributes:
> +            - netns-fd
> +            - netns-pid

[Severity: Low]
This isn't a bug, but should dev-netns-set have a reply block here?

The spec gives dev-netns-set a request and no reply. As a result,
vsock_nl_dev_netns_set_doit() only returns an errno and the caller gets a
bare ACK.

The netlink guidance for new families says every operation should reply
with a full message that identifies the resulting object. It also says
that once a command only ACKs, the ACK-only behaviour becomes uAPI and
cannot be changed later.

The v3 revision replied with netns-id. The v5 changelog says the reply
was dropped at Stefano's request, so this looks deliberate. Since this
family has not been released yet, could that decision be checked once
more?

A netns-id reply would tell the caller where the device ended up,
relative to its own namespace, without a second dev-netns-get request.

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 37abde1ca26cd..d4d88c8cf142f 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c

[ ... ]

> +int vsock_nl_dev_netns_set_doit(struct sk_buff *skb, struct genl_info *info)
> +{

[ ... ]

> +	err = vsock_g2h_net_assign(net, info->extack);
> +	put_net(net);
> +
> +	return err;
> +}

This is where a reply would be built, for example by reusing
vsock_nl_dev_netns_fill() as vsock_nl_dev_netns_get_doit() does.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113-vsock-guest-ns-v6-0-3737ce4673c0%40meta.com