Re: [PATCH RFC v2 8/8] vsock: add datagram tests to vsock_test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] vsock_test: local tests calling local_cid() fail without
  virtio or vhost
- [Medium] vsock_test: false-positive DGRAM_VIRTIO and DGRAM_PEER in
  dgram_capabilities()
- [Medium] vsock_test: VMCI coexistence fails in test_dgram_bind() and
  local CID bind
--

commit 004d4f33594e6af5b36eeee37f6e14f8e43323f5
Author: Sergio Lopez <slp@redhat.com>

vsock: add datagram tests to vsock_test

This commit adds AF_VSOCK datagram tests to vsock_test covering local
loopback behavior, host/guest transfers, routing, and queue accounting,
along with a --local execution mode and capability-based test skipping.

> diff --git a/tools/testing/vsock/vsock_test_dgram.c b/tools/testing/vsock/vsock_test_dgram.c
> new file mode 100644
> index 0000000000000..78bdf015a352f
> --- /dev/null
> +++ b/tools/testing/vsock/vsock_test_dgram.c
[ ... ]
> +static unsigned int local_cid(void)
> +{
> +	unsigned int cid;
> +	int fd;
> +
> +	fd = open("/dev/vsock", O_RDONLY | O_CLOEXEC);
> +	check(fd, "open /dev/vsock");
> +	check(ioctl(fd, IOCTL_VM_SOCKETS_GET_LOCAL_CID, &cid), "get local CID");
> +	close(fd);
> +	if (cid == VMADDR_CID_ANY || cid <= VMADDR_CID_LOCAL)
> +		fail("a virtio or vhost transport is required (local CID %u)", cid);
> +	return cid;
> +}

[Severity: Medium]
Will test_dgram_source_cid(), test_dgram_bound_lookup(), and
test_dgram_connected_cid() abort the test run instead of skipping when
vsock_loopback is loaded without a virtio or vhost transport?

Those three tests are guarded only by vsock_dgram_check_local(), which
succeeds whenever vsock_loopback is registered, but they unconditionally
call local_cid().  Without a guest-to-host or host-to-guest transport,
vsock_dev_do_ioctl() falls back to transport_local and returns a local cid
of 1, causing local_cid() to call fail().

[ ... ]

> +static unsigned int dgram_capabilities(const struct test_opts *opts,
> +				       unsigned int needs)
> +{
> +	unsigned int caps = DGRAM_SOCKET;
> +	struct sockaddr_vm peer;
> +	bool vmci;
> +	int fd, local_fd;
> +
> +	fd = socket(AF_VSOCK, SOCK_DGRAM | SOCK_NONBLOCK | SOCK_CLOEXEC, 0);
> +	if (fd < 0) {
> +		if (dgram_unavailable(errno))
> +			return 0;
> +		check(fd, "probe datagram socket");
> +	}
> +	/* VMCI may own the socket while a directional transport serves peers.
> +	 * Check the drivers independently; connect() below probes the peer route.
> +	 */
> +	vmci = !access("/sys/module/vmw_vsock_vmci_transport", F_OK);
> +	if (vmci)
> +		caps |= DGRAM_VMCI;
> +	if (!vmci ||
> +	    !access("/sys/bus/virtio/drivers/vmw_vsock_virtio_transport", F_OK) ||
> +	    !access("/sys/class/misc/vhost-vsock", F_OK))
> +		caps |= DGRAM_VIRTIO;
> +
> +	if ((needs & DGRAM_PEER) && (caps & DGRAM_VIRTIO)) {
> +		bind_fd(fd, opts->bind_cid, VMADDR_PORT_ANY);
> +		peer = address(opts->peer_cid, opts->peer_port);
> +		if (opts->to_host)
> +			peer.svm_flags = VMADDR_FLAG_TO_HOST;
> +		if (!connect(fd, (struct sockaddr *)&peer, sizeof(peer)))
> +			caps |= DGRAM_PEER;
> +		else if (!dgram_unavailable(errno) && errno != EINVAL)
> +			check(-1, "probe peer datagram route");
> +	}
> +	close(fd);

[Severity: Medium]
Can dgram_capabilities() report false-positive virtio and peer datagram
support when either vmw_vsock_vmci_transport is absent or the directional
transport did not negotiate datagrams?

When vmw_vsock_vmci_transport is not loaded, !vmci short-circuits the sysfs
checks and sets the virtio capability even if only vsock_loopback is
loaded; if opts->peer_cid is 2 without a guest-to-host transport,
vsock_use_local_transport() routes connect() to loopback and also sets the
peer capability.  Conversely, when vmw_vsock_vmci_transport is loaded
alongside virtio or vhost without datagram support negotiated,
vsock_dgram_get_transport() falls back to vmci_transport during connect(),
so connect() succeeds and peer tests run and fail instead of skipping.

[ ... ]

> +void test_dgram_bind(const struct test_opts *opts)
> +{
> +	int rx = bound_socket(VMADDR_CID_ANY);
> +	struct sockaddr_vm dest = local_address(rx);
> +	int tx = dgram_socket();
> +	int other = dgram_socket();
> +	struct sockaddr_vm src;
> +	char byte;
> +
> +	dest.svm_cid = VMADDR_CID_ANY;
> +	expect_error(bind(other, (struct sockaddr *)&dest, sizeof(dest)),
> +		     EADDRINUSE, "duplicate bind");
> +	send_packet(tx, &dest, "A", 1);
> +	src = recv_packet(rx, &byte, 1, 0, 1, false);
> +	if (byte != 'A' || src.svm_port != sock_name(tx, false).svm_port ||
> +	    src.svm_cid != VMADDR_CID_LOCAL || src.svm_port < 1024)
> +		fail("autobind or wildcard destination returned an invalid source");
[ ... ]
> +static void check_connected_local_cid(unsigned int peer_cid,
> +				      unsigned int stray_cid, bool shutdown_write)
> +{
> +	int rx = bound_socket(VMADDR_CID_ANY);
> +	int allowed = bound_socket(peer_cid);
> +	int stray = dgram_socket();
> +	struct sockaddr_vm peer = sock_name(allowed, false);
> +	struct sockaddr_vm dest = local_address(rx);
> +	struct sockaddr_vm src;
> +	char byte;
> +
> +	bind_fd(stray, stray_cid, peer.svm_port);

[Severity: Medium]
Do test_dgram_bind() and check_connected_local_cid() fail when
vmw_vsock_vmci_transport is loaded alongside vsock_loopback?

When vmci_transport owns the datagram socket, vsock_dgram_sendmsg()
resolves a wildcard destination cid via transport->get_local_cid()
(vmci_get_context_id()) instead of 1, so test_dgram_bind() failseither in
send_packet() or on the src.svm_cid check.  In check_connected_local_cid(),
vmci_transport_dgram_bind() passes flags = 0 to dg_create_handle() for any
non-wildcard cid, ignoring addr->svm_cid and registering both allowed and
stray under (vmci_get_context_id(), peer.svm_port), so binding stray fails
with -EADDRINUSE.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-vsock-dgram-support-v2-0-bacfc85b71ce@redhat.com?part=8