[PATCH 0/2] vsock/virtio: Fix data loss/disclosure due to joining of non-linear skb in RX queue

[PATCH 0/2] vsock/virtio: Fix data loss/disclosure due to joining of non-linear skb in RX queue

[Michal Luczaj]

Loopback transport coalesces some skbs too eagerly. Handling a zerocopy
(non-linear) skb as a linear one leads to skb data loss and kernel memory
disclosure.

Plug the loss/leak by allowing only linear skb join. Provide a test.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
Michal Luczaj (2):
      vsock/virtio: Coalesce only linear skb
      vsock/test: Add test for a linear and non-linear skb getting coalesced

 net/vmw_vsock/virtio_transport_common.c   |  3 +-
 tools/testing/vsock/vsock_test.c          |  5 +++
 tools/testing/vsock/vsock_test_zerocopy.c | 67 +++++++++++++++++++++++++++++++
 tools/testing/vsock/vsock_test_zerocopy.h |  3 ++
 4 files changed, 77 insertions(+), 1 deletion(-)
---
base-commit: 653267321f05316f159e05b3ef562aa700632db6
change-id: 20260103-vsock-recv-coalescence-38178fafd10c

Best regards,
-- 
Michal Luczaj <mhal@rbox.co>

[PATCH 1/2] vsock/virtio: Coalesce only linear skb

[Michal Luczaj]

Vsock/virtio common tries to coalesce buffers in rx queue: if a linear skb
(with a spare tail room) is followed by a small skb (length limited by
GOOD_COPY_LEN = 128), an attempt is made to join them.

Since the introduction of MSG_ZEROCOPY support, assumption that a small skb
will always be linear is incorrect (see loopback transport). In the
zerocopy case, data is lost and the linear skb is appended with
uninitialized kernel memory.

Ensure only linear skbs are coalesced. Note that skb_tailroom(last_skb) > 0
guarantees last_skb is linear.

Fixes: 581512a6dc93 ("vsock/virtio: MSG_ZEROCOPY flag support")
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/virtio_transport_common.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index dcc8a1d5851e..cf35eb7190cc 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -1375,7 +1375,8 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk,
 		 * of a new message.
 		 */
 		if (skb->len < skb_tailroom(last_skb) &&
-		    !(le32_to_cpu(last_hdr->flags) & VIRTIO_VSOCK_SEQ_EOM)) {
+		    !(le32_to_cpu(last_hdr->flags) & VIRTIO_VSOCK_SEQ_EOM) &&
+		    !skb_is_nonlinear(skb)) {
 			memcpy(skb_put(last_skb, skb->len), skb->data, skb->len);
 			free_pkt = true;
 			last_hdr->flags |= hdr->flags;

-- 
2.52.0

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

[Michal Luczaj]

Loopback transport can mangle data in rx queue when a linear skb is
followed by a small MSG_ZEROCOPY packet.

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",
+		.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 */
+
+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);
+	}
+
+	close(fd);
+}
+
+static void recv_verify(int fd, char *buf, unsigned int len, char pattern)
+{
+	recv_buf(fd, buf, len, 0, len);
+
+	while (len--) {
+		if (*buf++ != pattern) {
+			fprintf(stderr, "Incorrect data received\n");
+			exit(EXIT_FAILURE);
+		}
+	}
+}
+
+void test_stream_msgzcopy_mangle_server(const struct test_opts *opts)
+{
+	char rbuf[PAGE_SIZE + 1];
+	int fd;
+
+	fd = vsock_stream_accept(VMADDR_CID_ANY, opts->peer_port, NULL);
+	if (fd < 0) {
+		perror("accept");
+		exit(EXIT_FAILURE);
+	}
+
+	/* Wait, don't race the (buggy) skbs coalescence. */
+	vsock_ioctl_int(fd, SIOCINQ, PAGE_SIZE + 1 + GOOD_COPY_LEN);
+
+	recv_verify(fd, rbuf, PAGE_SIZE + 1, '1');
+	recv_verify(fd, rbuf, GOOD_COPY_LEN, '2');
+
+	close(fd);
+}
diff --git a/tools/testing/vsock/vsock_test_zerocopy.h b/tools/testing/vsock/vsock_test_zerocopy.h
index 3ef2579e024d..d46c91a69f16 100644
--- a/tools/testing/vsock/vsock_test_zerocopy.h
+++ b/tools/testing/vsock/vsock_test_zerocopy.h
@@ -12,4 +12,7 @@ void test_seqpacket_msgzcopy_server(const struct test_opts *opts);
 void test_stream_msgzcopy_empty_errq_client(const struct test_opts *opts);
 void test_stream_msgzcopy_empty_errq_server(const struct test_opts *opts);
 
+void test_stream_msgzcopy_mangle_client(const struct test_opts *opts);
+void test_stream_msgzcopy_mangle_server(const struct test_opts *opts);
+
 #endif /* VSOCK_TEST_ZEROCOPY_H */

-- 
2.52.0

Re: [PATCH 0/2] vsock/virtio: Fix data loss/disclosure due to joining of non-linear skb in RX queue

[Michal Luczaj]

On 1/8/26 10:54, Michal Luczaj wrote:
> Loopback transport coalesces some skbs too eagerly. Handling a zerocopy
> (non-linear) skb as a linear one leads to skb data loss and kernel memory
> disclosure.
> 
> Plug the loss/leak by allowing only linear skb join. Provide a test.

Aaand I forgot to set the "net" prefix. Please let me know if I should
resend just for that.

Sorry,
Michal

Re: [PATCH 1/2] vsock/virtio: Coalesce only linear skb

[Stefano Garzarella]

On Thu, Jan 08, 2026 at 10:54:54AM +0100, Michal Luczaj wrote:
>Vsock/virtio common tries to coalesce buffers in rx queue: if a linear skb
>(with a spare tail room) is followed by a small skb (length limited by
>GOOD_COPY_LEN = 128), an attempt is made to join them.
>
>Since the introduction of MSG_ZEROCOPY support, assumption that a small skb
>will always be linear is incorrect (see loopback transport). In the
>zerocopy case, data is lost and the linear skb is appended with
>uninitialized kernel memory.
>
>Ensure only linear skbs are coalesced. Note that skb_tailroom(last_skb) > 0
>guarantees last_skb is linear.
>
>Fixes: 581512a6dc93 ("vsock/virtio: MSG_ZEROCOPY flag support")
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> net/vmw_vsock/virtio_transport_common.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index dcc8a1d5851e..cf35eb7190cc 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -1375,7 +1375,8 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk,
> 		 * of a new message.
> 		 */
> 		if (skb->len < skb_tailroom(last_skb) &&
>-		    !(le32_to_cpu(last_hdr->flags) & VIRTIO_VSOCK_SEQ_EOM)) {
>+		    !(le32_to_cpu(last_hdr->flags) & VIRTIO_VSOCK_SEQ_EOM) &&
>+		    !skb_is_nonlinear(skb)) {

Why here? I mean we can do the check even early, something like this:

--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -1361,7 +1361,8 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk,
          * to avoid wasting memory queueing the entire buffer with a small
          * payload.
          */
-       if (len <= GOOD_COPY_LEN && !skb_queue_empty(&vvs->rx_queue)) {
+       if (len <= GOOD_COPY_LEN && !skb_queue_empty(&vvs->rx_queue) &&
+           !skb_is_nonlinear(skb)) {
                 struct virtio_vsock_hdr *last_hdr;
                 struct sk_buff *last_skb;


I would also add the reason in the comment before that to make it clear.

Thanks for the fix!
Stefano

> 			memcpy(skb_put(last_skb, skb->len), skb->data, skb->len);
> 			free_pkt = true;
> 			last_hdr->flags |= hdr->flags;
>
>-- 
>2.52.0
>

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

[Stefano Garzarella]

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?

>
>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.

>+		.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?

>+
>+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().

>+
>+	close(fd);
>+}
>+
>+static void recv_verify(int fd, char *buf, unsigned int len, char pattern)
>+{
>+	recv_buf(fd, buf, len, 0, len);
>+
>+	while (len--) {
>+		if (*buf++ != pattern) {
>+			fprintf(stderr, "Incorrect data received\n");
>+			exit(EXIT_FAILURE);
>+		}
>+	}
>+}
>+
>+void test_stream_msgzcopy_mangle_server(const struct test_opts *opts)
>+{
>+	char rbuf[PAGE_SIZE + 1];
>+	int fd;
>+
>+	fd = vsock_stream_accept(VMADDR_CID_ANY, opts->peer_port, NULL);
>+	if (fd < 0) {
>+		perror("accept");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	/* Wait, don't race the (buggy) skbs coalescence. */
>+	vsock_ioctl_int(fd, SIOCINQ, PAGE_SIZE + 1 + GOOD_COPY_LEN);

This is cool, another option is to add a barrier here (and after the 
poll), but yeah, this should be even better.

Thanks,
Stefano

>+
>+	recv_verify(fd, rbuf, PAGE_SIZE + 1, '1');
>+	recv_verify(fd, rbuf, GOOD_COPY_LEN, '2');
>+
>+	close(fd);
>+}
>diff --git a/tools/testing/vsock/vsock_test_zerocopy.h b/tools/testing/vsock/vsock_test_zerocopy.h
>index 3ef2579e024d..d46c91a69f16 100644
>--- a/tools/testing/vsock/vsock_test_zerocopy.h
>+++ b/tools/testing/vsock/vsock_test_zerocopy.h
>@@ -12,4 +12,7 @@ void test_seqpacket_msgzcopy_server(const struct test_opts *opts);
> void test_stream_msgzcopy_empty_errq_client(const struct test_opts *opts);
> void test_stream_msgzcopy_empty_errq_server(const struct test_opts 
> *opts);
>
>+void test_stream_msgzcopy_mangle_client(const struct test_opts *opts);
>+void test_stream_msgzcopy_mangle_server(const struct test_opts *opts);
>+
> #endif /* VSOCK_TEST_ZEROCOPY_H */
>
>-- 
>2.52.0
>

Re: [PATCH 1/2] vsock/virtio: Coalesce only linear skb

[Michal Luczaj]

On 1/9/26 17:18, Stefano Garzarella wrote:
> On Thu, Jan 08, 2026 at 10:54:54AM +0100, Michal Luczaj wrote:
...
>> @@ -1375,7 +1375,8 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk,
>> 		 * of a new message.
>> 		 */
>> 		if (skb->len < skb_tailroom(last_skb) &&
>> -		    !(le32_to_cpu(last_hdr->flags) & VIRTIO_VSOCK_SEQ_EOM)) {
>> +		    !(le32_to_cpu(last_hdr->flags) & VIRTIO_VSOCK_SEQ_EOM) &&
>> +		    !skb_is_nonlinear(skb)) {
> 
> Why here? I mean we can do the check even early, something like this:
> 
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -1361,7 +1361,8 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk,
>           * to avoid wasting memory queueing the entire buffer with a small
>           * payload.
>           */
> -       if (len <= GOOD_COPY_LEN && !skb_queue_empty(&vvs->rx_queue)) {
> +       if (len <= GOOD_COPY_LEN && !skb_queue_empty(&vvs->rx_queue) &&
> +           !skb_is_nonlinear(skb)) {
>                  struct virtio_vsock_hdr *last_hdr;
>                  struct sk_buff *last_skb;

Right, can do. I've assumed skb being non-linear is the least likely in
this context.

> I would also add the reason in the comment before that to make it clear.

OK, sure.

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

[Michal Luczaj]

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.

>> 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).

>> +		.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.

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?

>> +
>> +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.

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

[Stefano Garzarella]

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

Re: [PATCH 1/2] vsock/virtio: Coalesce only linear skb

[Stefano Garzarella]

On Sun, Jan 11, 2026 at 11:59:44AM +0100, Michal Luczaj wrote:
>On 1/9/26 17:18, Stefano Garzarella wrote:
>> On Thu, Jan 08, 2026 at 10:54:54AM +0100, Michal Luczaj wrote:
>...
>>> @@ -1375,7 +1375,8 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk,
>>> 		 * of a new message.
>>> 		 */
>>> 		if (skb->len < skb_tailroom(last_skb) &&
>>> -		    !(le32_to_cpu(last_hdr->flags) & VIRTIO_VSOCK_SEQ_EOM)) {
>>> +		    !(le32_to_cpu(last_hdr->flags) & VIRTIO_VSOCK_SEQ_EOM) &&
>>> +		    !skb_is_nonlinear(skb)) {
>>
>> Why here? I mean we can do the check even early, something like this:
>>
>> --- a/net/vmw_vsock/virtio_transport_common.c
>> +++ b/net/vmw_vsock/virtio_transport_common.c
>> @@ -1361,7 +1361,8 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk,
>>           * to avoid wasting memory queueing the entire buffer with a small
>>           * payload.
>>           */
>> -       if (len <= GOOD_COPY_LEN && !skb_queue_empty(&vvs->rx_queue)) {
>> +       if (len <= GOOD_COPY_LEN && !skb_queue_empty(&vvs->rx_queue) &&
>> +           !skb_is_nonlinear(skb)) {
>>                  struct virtio_vsock_hdr *last_hdr;
>>                  struct sk_buff *last_skb;
>
>Right, can do. I've assumed skb being non-linear is the least likely in
>this context.

Yeah, but it's a very simple check, so IMHO the code is more readable if 
we put it in the first conditions, where we check if the current packet 
has the requisites, rather than in the nested conditions, where we check 
that the packet already queued can receive the new payload.

>
>> I would also add the reason in the comment before that to make it clear.
>
>OK, sure.
>

Thanks,
Stefano

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

[Michal Luczaj]

On 1/12/26 14:44, Stefano Garzarella wrote:
> On Sun, Jan 11, 2026 at 11:59:54AM +0100, Michal Luczaj wrote:
>>>> 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?

With the usual caveat that I may be completely missing something, aren't
all virtio-transport's rx skbs linear? See virtio_vsock_alloc_linear_skb()
in virtio_vsock_rx_fill().

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

[Stefano Garzarella]

On Mon, Jan 12, 2026 at 04:52:02PM +0100, Michal Luczaj wrote:
>On 1/12/26 14:44, Stefano Garzarella wrote:
>> On Sun, Jan 11, 2026 at 11:59:54AM +0100, Michal Luczaj wrote:
>>>>> 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?
>
>With the usual caveat that I may be completely missing something, aren't
>all virtio-transport's rx skbs linear? See virtio_vsock_alloc_linear_skb()
>in virtio_vsock_rx_fill().
>

True, but what about drivers/vhost/vsock.c ?

IIUC in vhost_vsock_handle_tx_kick() we call vhost_vsock_alloc_skb(), 
that calls virtio_vsock_alloc_skb() and pass that skb to 
virtio_transport_recv_pkt(). So, it's also affected right?

BTW in general we consider loopback as one of virtio devices since it 
really shares with them most of the code.

That said, now I'm thinking more about Fixes tag.
Before commit 6693731487a8 ("vsock/virtio: Allocate nonlinear SKBs for 
handling large transmit buffers") was that a real issue?

Thanks,
Stefano

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

[Michal Luczaj]

On 1/12/26 17:48, Stefano Garzarella wrote:
>>>>>> 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?
>>
>> With the usual caveat that I may be completely missing something, aren't
>> all virtio-transport's rx skbs linear? See virtio_vsock_alloc_linear_skb()
>> in virtio_vsock_rx_fill().
>>
> 
> True, but what about drivers/vhost/vsock.c ?
> 
> IIUC in vhost_vsock_handle_tx_kick() we call vhost_vsock_alloc_skb(), 
> that calls virtio_vsock_alloc_skb() and pass that skb to 
> virtio_transport_recv_pkt(). So, it's also affected right?

virtio_vsock_alloc_skb() returns a non-linear skb only if size >
SKB_WITH_OVERHEAD(PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER)). And that is way
more than GOOD_COPY_LEN, so we're good.

At least until someone increases GOOD_COPY_LEN and/or reduces the size
condition for non-linear allocation. So, yeah, a bit brittle.

> BTW in general we consider loopback as one of virtio devices since it 
> really shares with them most of the code.

Fair enough, I'll add "virtio" to the test name.

> That said, now I'm thinking more about Fixes tag.
> Before commit 6693731487a8 ("vsock/virtio: Allocate nonlinear SKBs for 
> handling large transmit buffers") was that a real issue?

I don't really think that commit changes anything for the zerocopy case. It
only makes some big (>GOOD_COPY_LEN) non-ZC skbs turn non-linear.

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

[Stefano Garzarella]

On Mon, Jan 12, 2026 at 10:20:50PM +0100, Michal Luczaj wrote:
>On 1/12/26 17:48, Stefano Garzarella wrote:
>>>>>>> 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?
>>>
>>> With the usual caveat that I may be completely missing something, aren't
>>> all virtio-transport's rx skbs linear? See virtio_vsock_alloc_linear_skb()
>>> in virtio_vsock_rx_fill().
>>>
>>
>> True, but what about drivers/vhost/vsock.c ?
>>
>> IIUC in vhost_vsock_handle_tx_kick() we call vhost_vsock_alloc_skb(),
>> that calls virtio_vsock_alloc_skb() and pass that skb to
>> virtio_transport_recv_pkt(). So, it's also affected right?
>
>virtio_vsock_alloc_skb() returns a non-linear skb only if size >
>SKB_WITH_OVERHEAD(PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER)). And that is way
>more than GOOD_COPY_LEN, so we're good.
>
>At least until someone increases GOOD_COPY_LEN and/or reduces the size
>condition for non-linear allocation. So, yeah, a bit brittle.

I see, thanks for clarify. So please add all of this conclusions in the 
patch 1 description to make it clear that only loopback is affected, so 
no guest/host attack is possible. (not really severe CVE)

>
>> BTW in general we consider loopback as one of virtio devices since it
>> really shares with them most of the code.
>
>Fair enough, I'll add "virtio" to the test name.

Thanks.

>
>> That said, now I'm thinking more about Fixes tag.
>> Before commit 6693731487a8 ("vsock/virtio: Allocate nonlinear SKBs for
>> handling large transmit buffers") was that a real issue?
>
>I don't really think that commit changes anything for the zerocopy case. It
>only makes some big (>GOOD_COPY_LEN) non-ZC skbs turn non-linear.
>

I see.

Stefano

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

[Michal Luczaj]

On 1/13/26 10:36, Stefano Garzarella wrote:
> On Mon, Jan 12, 2026 at 10:20:50PM +0100, Michal Luczaj wrote:
>> On 1/12/26 17:48, Stefano Garzarella wrote:
>>>>>>>> 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?
>>>>
>>>> With the usual caveat that I may be completely missing something, aren't
>>>> all virtio-transport's rx skbs linear? See virtio_vsock_alloc_linear_skb()
>>>> in virtio_vsock_rx_fill().
>>>>
>>>
>>> True, but what about drivers/vhost/vsock.c ?
>>>
>>> IIUC in vhost_vsock_handle_tx_kick() we call vhost_vsock_alloc_skb(),
>>> that calls virtio_vsock_alloc_skb() and pass that skb to
>>> virtio_transport_recv_pkt(). So, it's also affected right?
>>
>> virtio_vsock_alloc_skb() returns a non-linear skb only if size >
>> SKB_WITH_OVERHEAD(PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER)). And that is way
>> more than GOOD_COPY_LEN, so we're good.
>>
>> At least until someone increases GOOD_COPY_LEN and/or reduces the size
>> condition for non-linear allocation. So, yeah, a bit brittle.
> 
> I see, thanks for clarify. So please add all of this conclusions in the 
> patch 1 description to make it clear that only loopback is affected, so 
> no guest/host attack is possible. (not really severe CVE)

OK, here's v2:
https://lore.kernel.org/netdev/20260113-vsock-recv-coalescence-v2-0-552b17837cf4@rbox.co/