Re: [PATCH net 2/2] vsock/test: add MSG_PEEK after partial recv test

[Luigi Leonardi <leonardi@redhat.com>]

On Thu, Apr 02, 2026 at 03:28:25PM +0200, Stefano Garzarella wrote:
>On Thu, Apr 02, 2026 at 10:18:02AM +0200, Luigi Leonardi wrote:
>>Add a test that verifies MSG_PEEK works correctly after a partial
>>recv().
>>
>>This is to test a bug that was present in the `virtio_transport_stream_do_peek()`
>
>WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
>#11: This is to test a bug that was present in the 
>`virtio_transport_stream_do_peek()`
>

oops, thanks :)

>>when computing the number of bytes to copy: After a partial read, the
>>peek function didn't take into consideration the number of bytes that
>>were already read. So peeking the whole buffer would cause a out-of-bounds read,
>>that resulted in a -EFAULT.
>>
>>This test does exactly this: do a partial recv on a buffer, then try to
>>peek the whole buffer content.
>>
>>Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
>>---
>>tools/testing/vsock/vsock_test.c | 64 ++++++++++++++++++++++++++++++++++++++++
>>1 file changed, 64 insertions(+)
>>
>>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>>index 5bd20ccd9335caafe68e8b7a5d02a4deb3d2deec..308f9f8f30d22bec5aaa282356e400d8438fe321 100644
>>--- a/tools/testing/vsock/vsock_test.c
>>+++ b/tools/testing/vsock/vsock_test.c
>>@@ -346,6 +346,65 @@ static void test_stream_msg_peek_server(const struct test_opts *opts)
>>	return test_msg_peek_server(opts, false);
>>}
>>
>>+#define PEEK_AFTER_RECV_LEN 100
>
>Why 100 ?
>Better to use a power of 2 IMO like we do in all other cases IIRC.
>

Right, I'll reuse `MSG_PEEK_BUF_LEN`.

>>+
>>+static void test_stream_peek_after_recv_client(const struct test_opts *opts)
>>+{
>>+	unsigned char buf[PEEK_AFTER_RECV_LEN];
>>+	int fd;
>>+	int i;
>
>nit: int fd, i;
>
>>+
>>+	fd = vsock_stream_connect(opts->peer_cid, opts->peer_port);
>>+	if (fd < 0) {
>>+		perror("connect");
>>+		exit(EXIT_FAILURE);
>>+	}
>>+
>>+	for (i = 0; i < sizeof(buf); i++)
>>+		buf[i] = (unsigned char)i;
>
>Why setting the payload in this way ? Can we just do a memset() ?

Good point.

>
>>+
>>+	control_expectln("SRVREADY");
>
>Why we need this barrier ?

leftover from development, will remove.

>
>>+
>>+	send_buf(fd, buf, sizeof(buf), 0, sizeof(buf));
>>+
>>+	close(fd);
>>+}
>>+
>>+static void test_stream_peek_after_recv_server(const struct test_opts *opts)
>>+{
>>+	unsigned char buf[PEEK_AFTER_RECV_LEN];
>>+	int half = PEEK_AFTER_RECV_LEN / 2;
>>+	ssize_t ret;
>>+	int fd;
>>+
>>+	fd = vsock_stream_accept(VMADDR_CID_ANY, opts->peer_port, NULL);
>>+	if (fd < 0) {
>>+		perror("accept");
>>+		exit(EXIT_FAILURE);
>>+	}
>>+
>>+	control_writeln("SRVREADY");
>>+
>>+	/* Partial recv to advance offset within the skb */
>>+	recv_buf(fd, buf, half, 0, half);
>
>Why reading half of the size ?
>
>IMO is better to read just 1 byte, since it is almost certain that an 
>skb does not have a 1-byte payload.
>

will do

>>+
>>+	/* Try to peek more than what remains: should return only 'half'
>
>How we are sure that the sender sent all the bytes ?
>
>>+	 * bytes. Note: we can't use recv_buf() because it loops until
>>+	 * all requested bytes are returned.
>
>Why this is a problem ? (an useful comment should explain the reason)
>

Some changes are required to `recv_buf`, I have a working v2 version
that uses that. Thanks for the hint.

>>+	 */
>>+	ret = recv(fd, buf, sizeof(buf), MSG_PEEK);
>>+	if (ret < 0) {
>
>Should we handle EINTR like we do in recv_buf() ?
>But I still don't understand why we can't use it directly.
>
>Thanks,
>Stefano
>
>>+		perror("recv");
>>+		exit(EXIT_FAILURE);
>>+	} else if (ret != half) {
>>+		fprintf(stderr, "MSG_PEEK after partial recv returned %d (expected %d)\n",
>>+			ret, half);
>>+		exit(EXIT_FAILURE);
>>+	}
>>+
>>+	close(fd);
>>+}
>>+
>>#define SOCK_BUF_SIZE (2 * 1024 * 1024)
>>#define SOCK_BUF_SIZE_SMALL (64 * 1024)
>>#define MAX_MSG_PAGES 4
>>@@ -2520,6 +2579,11 @@ static struct test_case test_cases[] = {
>>		.run_client = test_stream_tx_credit_bounds_client,
>>		.run_server = test_stream_tx_credit_bounds_server,
>>	},
>>+	{
>>+		.name = "SOCK_STREAM MSG_PEEK after partial recv",
>>+		.run_client = test_stream_peek_after_recv_client,
>>+		.run_server = test_stream_peek_after_recv_server,
>>+	},
>>	{},
>>};
>>
>>
>>-- 
>>2.53.0
>>
>