Re: [PATCH 2/2] vsock/test: Add test for a linear and non-linear skb getting coalesced

[Stefano Garzarella <sgarzare@redhat.com>]

On Sun, Jan 11, 2026 at 11:59:54AM +0100, Michal Luczaj wrote:
>On 1/9/26 17:32, Stefano Garzarella wrote:
>> On Thu, Jan 08, 2026 at 10:54:55AM +0100, Michal Luczaj wrote:
>>> Loopback transport can mangle data in rx queue when a linear skb is
>>> followed by a small MSG_ZEROCOPY packet.
>>
>> Can we describe a bit more what the test is doing?
>
>I've expanded the commit message:
>
>To exercise the logic, send out two packets: a weirdly sized one (to ensure
>some spare tail room in the skb) and a zerocopy one that's small enough to
>fit in the spare room of its predecessor. Then, wait for both to land in
>the rx queue, and check the data received. Faulty packets merger manifests
>itself by corrupting payload of the later packet.

Thanks! LGTM!

>
>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>> ---
>>> tools/testing/vsock/vsock_test.c          |  5 +++
>>> tools/testing/vsock/vsock_test_zerocopy.c | 67 +++++++++++++++++++++++++++++++
>>> tools/testing/vsock/vsock_test_zerocopy.h |  3 ++
>>> 3 files changed, 75 insertions(+)
>>>
>>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>>> index bbe3723babdc..21c8616100f1 100644
>>> --- a/tools/testing/vsock/vsock_test.c
>>> +++ b/tools/testing/vsock/vsock_test.c
>>> @@ -2403,6 +2403,11 @@ static struct test_case test_cases[] = {
>>> 		.run_client = test_stream_accepted_setsockopt_client,
>>> 		.run_server = test_stream_accepted_setsockopt_server,
>>> 	},
>>> +	{
>>> +		.name = "SOCK_STREAM MSG_ZEROCOPY coalescence corruption",
>>
>> This is essentially a regression test for virtio transport, so I'd add
>> virtio in the test name.
>
>Isn't virtio transport unaffected? It's about loopback transport (that
>shares common code with virtio transport).

Why virtio transport is not affected?

>
>>> +		.run_client = test_stream_msgzcopy_mangle_client,
>>> +		.run_server = test_stream_msgzcopy_mangle_server,
>>> +	},
>>> 	{},
>>> };
>>>
>>> diff --git a/tools/testing/vsock/vsock_test_zerocopy.c b/tools/testing/vsock/vsock_test_zerocopy.c
>>> index 9d9a6cb9614a..6735a9d7525d 100644
>>> --- a/tools/testing/vsock/vsock_test_zerocopy.c
>>> +++ b/tools/testing/vsock/vsock_test_zerocopy.c
>>> @@ -9,11 +9,13 @@
>>> #include <stdio.h>
>>> #include <stdlib.h>
>>> #include <string.h>
>>> +#include <sys/ioctl.h>
>>> #include <sys/mman.h>
>>> #include <unistd.h>
>>> #include <poll.h>
>>> #include <linux/errqueue.h>
>>> #include <linux/kernel.h>
>>> +#include <linux/sockios.h>
>>> #include <errno.h>
>>>
>>> #include "control.h"
>>> @@ -356,3 +358,68 @@ void test_stream_msgzcopy_empty_errq_server(const struct test_opts *opts)
>>> 	control_expectln("DONE");
>>> 	close(fd);
>>> }
>>> +
>>> +#define GOOD_COPY_LEN	128	/* net/vmw_vsock/virtio_transport_common.c */
>>
>> I think we don't need this, I mean we can eventually just send a single
>> byte, no?
>
>For a single byte sent, you get a single byte of uninitialized kernel
>memory. Uninitialized memory can by anything, in particular it can be
>(coincidentally) what you happen to expect. Which would result in a false
>positive. So instead of estimating what length sufficiently reduces
>probability of such false positive, I just took the upper bound.

I see, makes sense to me.

>
>BTW, I've realized recv_verify() is reinventing the wheel. How about
>dropping it in favour of what test_seqpacket_msg_bounds_client() does, i.e.
>calc the hash of payload and send it over the control channel for verification?

Yeah, strongly agree on that.

>
>>> +
>>> +void test_stream_msgzcopy_mangle_client(const struct test_opts *opts)
>>> +{
>>> +	char sbuf1[PAGE_SIZE + 1], sbuf2[GOOD_COPY_LEN];
>>> +	struct pollfd fds;
>>> +	int fd;
>>> +
>>> +	fd = vsock_stream_connect(opts->peer_cid, opts->peer_port);
>>> +	if (fd < 0) {
>>> +		perror("connect");
>>> +		exit(EXIT_FAILURE);
>>> +	}
>>> +
>>> +	enable_so_zerocopy_check(fd);
>>> +
>>> +	memset(sbuf1, '1', sizeof(sbuf1));
>>> +	memset(sbuf2, '2', sizeof(sbuf2));
>>> +
>>> +	send_buf(fd, sbuf1, sizeof(sbuf1), 0, sizeof(sbuf1));
>>> +	send_buf(fd, sbuf2, sizeof(sbuf2), MSG_ZEROCOPY, sizeof(sbuf2));
>>> +
>>> +	fds.fd = fd;
>>> +	fds.events = 0;
>>> +
>>> +	if (poll(&fds, 1, -1) != 1 || !(fds.revents & POLLERR)) {
>>> +		perror("poll");
>>> +		exit(EXIT_FAILURE);
>>> +	}
>>
>> Should we also call vsock_recv_completion() or we don't care about the
>> result?
>>
>> If we need it, maybe we can factor our the poll +
>> vsock_recv_completion().
>
>Nope, we don't care about the result.
>

Okay, I see.

Thanks,
Stefano