[PATCH net 0/2] vsock: Fix SO_ZEROCOPY on accept()ed vsocks

[PATCH net 0/2] vsock: Fix SO_ZEROCOPY on accept()ed vsocks

[Michal Luczaj]

vsock has its own handling of setsockopt(SO_ZEROCOPY). Which works just
fine unless socket comes from a call to accept(). Because
SOCK_CUSTOM_SOCKOPT flag is missing, attempting to set the option always
results in errno EOPNOTSUPP.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
Michal Luczaj (2):
      vsock: Make accept()ed sockets use custom setsockopt()
      vsock/test: Test setting SO_ZEROCOPY on accept()ed socket

 net/vmw_vsock/af_vsock.c         |  1 +
 tools/testing/vsock/vsock_test.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)
---
base-commit: 5e7365b5a1ac8f517a7a84442289d7de242deb76
change-id: 20251222-vsock-child-sock-custom-sockopt-23b779c30c8f

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

[PATCH net 1/2] vsock: Make accept()ed sockets use custom setsockopt()

[Michal Luczaj]

SO_ZEROCOPY handling in vsock_connectible_setsockopt() does not get called
on accept()ed sockets due to a missing flag. Flip it.

Fixes: e0718bd82e27 ("vsock: enable setting SO_ZEROCOPY")
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/af_vsock.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index adcba1b7bf74..c093db8fec2d 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1787,6 +1787,7 @@ static int vsock_accept(struct socket *sock, struct socket *newsock,
 		} else {
 			newsock->state = SS_CONNECTED;
 			sock_graft(connected, newsock);
+			set_bit(SOCK_CUSTOM_SOCKOPT, &newsock->flags);
 			if (vsock_msgzerocopy_allow(vconnected->transport))
 				set_bit(SOCK_SUPPORT_ZC,
 					&connected->sk_socket->flags);

-- 
2.52.0

Re: [PATCH net 1/2] vsock: Make accept()ed sockets use custom setsockopt()

[Stefano Garzarella]

On Tue, Dec 23, 2025 at 10:15:28AM +0100, Michal Luczaj wrote:
>SO_ZEROCOPY handling in vsock_connectible_setsockopt() does not get called
>on accept()ed sockets due to a missing flag. Flip it.
>
>Fixes: e0718bd82e27 ("vsock: enable setting SO_ZEROCOPY")
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> net/vmw_vsock/af_vsock.c | 1 +
> 1 file changed, 1 insertion(+)

Thanks for the fix!

>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index adcba1b7bf74..c093db8fec2d 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -1787,6 +1787,7 @@ static int vsock_accept(struct socket *sock, struct socket *newsock,
> 		} else {
> 			newsock->state = SS_CONNECTED;
> 			sock_graft(connected, newsock);
>+			set_bit(SOCK_CUSTOM_SOCKOPT, &newsock->flags);

I was a bit confused about next lines calling set_bit on 
`connected->sk_socket->flags`, but after `sock_graft(connected, 
newsock)` they are equivalent.

So, maybe I would move the new line before the sock_graft() call or use 
`connected->sk_socket->flags` if you want to keep it after it.

WDYT?

BTW the fix LGTM.
Thanks,
Stefano

> 			if (vsock_msgzerocopy_allow(vconnected->transport))
> 				set_bit(SOCK_SUPPORT_ZC,
> 					&connected->sk_socket->flags);
>
>-- 
>2.52.0
>

Re: [PATCH net 1/2] vsock: Make accept()ed sockets use custom setsockopt()

[Michal Luczaj]

On 12/23/25 11:26, Stefano Garzarella wrote:
> On Tue, Dec 23, 2025 at 10:15:28AM +0100, Michal Luczaj wrote:
...
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index adcba1b7bf74..c093db8fec2d 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -1787,6 +1787,7 @@ static int vsock_accept(struct socket *sock, struct socket *newsock,
>> 		} else {
>> 			newsock->state = SS_CONNECTED;
>> 			sock_graft(connected, newsock);
>> +			set_bit(SOCK_CUSTOM_SOCKOPT, &newsock->flags);
> 
> I was a bit confused about next lines calling set_bit on 
> `connected->sk_socket->flags`, but after `sock_graft(connected, 
> newsock)` they are equivalent.
> 
> So, maybe I would move the new line before the sock_graft() call or use 
> `connected->sk_socket->flags` if you want to keep it after it.
...
>> 			if (vsock_msgzerocopy_allow(vconnected->transport))
>> 				set_bit(SOCK_SUPPORT_ZC,
>> 					&connected->sk_socket->flags);

Hmm, isn't using both `connected->sk_socket->flags` and `newsock->flags` a
bit confusing? `connected->sk_socket->flags` feels unnecessary long to me.
So how about a not-so-minimal-patch to have

	newsock->state = SS_CONNECTED;
	set_bit(SOCK_CUSTOM_SOCKOPT, &newsock->flags);
	if (vsock_msgzerocopy_allow(vconnected->transport))
		set_bit(SOCK_SUPPORT_ZC, &newsock->flags);
	sock_graft(connected, newsock);

?

Re: [PATCH net 1/2] vsock: Make accept()ed sockets use custom setsockopt()

[Stefano Garzarella]

On Tue, Dec 23, 2025 at 12:09:51PM +0100, Michal Luczaj wrote:
>On 12/23/25 11:26, Stefano Garzarella wrote:
>> On Tue, Dec 23, 2025 at 10:15:28AM +0100, Michal Luczaj wrote:
>...
>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>> index adcba1b7bf74..c093db8fec2d 100644
>>> --- a/net/vmw_vsock/af_vsock.c
>>> +++ b/net/vmw_vsock/af_vsock.c
>>> @@ -1787,6 +1787,7 @@ static int vsock_accept(struct socket *sock, struct socket *newsock,
>>> 		} else {
>>> 			newsock->state = SS_CONNECTED;
>>> 			sock_graft(connected, newsock);
>>> +			set_bit(SOCK_CUSTOM_SOCKOPT, &newsock->flags);
>>
>> I was a bit confused about next lines calling set_bit on
>> `connected->sk_socket->flags`, but after `sock_graft(connected,
>> newsock)` they are equivalent.
>>
>> So, maybe I would move the new line before the sock_graft() call or use
>> `connected->sk_socket->flags` if you want to keep it after it.
>...
>>> 			if (vsock_msgzerocopy_allow(vconnected->transport))
>>> 				set_bit(SOCK_SUPPORT_ZC,
>>> 					&connected->sk_socket->flags);
>
>Hmm, isn't using both `connected->sk_socket->flags` and `newsock->flags` a
>bit confusing?

Yep, for that reason I suggested to use `connected->sk_socket->flags`.

>`connected->sk_socket->flags` feels unnecessary long to me.
>So how about a not-so-minimal-patch to have
>
>	newsock->state = SS_CONNECTED;
>	set_bit(SOCK_CUSTOM_SOCKOPT, &newsock->flags);
>	if (vsock_msgzerocopy_allow(vconnected->transport))
>		set_bit(SOCK_SUPPORT_ZC, &newsock->flags);
>	sock_graft(connected, newsock);

No, please, this is a fix, so let's touch less as possible.

As I mentioned before, we have 2 options IMO:
1. use `set_bit(SOCK_CUSTOM_SOCKOPT, &newsock->flags);` but move it
    before `sock_graft()`
2. use `connected->sk_socket->flags` and set it after `sock_graft()` if
    we want to be a bit more consistent

I'd go with option 2, because I like to be consistent and it's less
confusing IMHO, but I'm fine also with option 1.

Thanks,
Stefano

Re: [PATCH net 1/2] vsock: Make accept()ed sockets use custom setsockopt()

[Michal Luczaj]

On 12/23/25 14:15, Stefano Garzarella wrote:
> On Tue, Dec 23, 2025 at 12:09:51PM +0100, Michal Luczaj wrote:
>> On 12/23/25 11:26, Stefano Garzarella wrote:
>>> On Tue, Dec 23, 2025 at 10:15:28AM +0100, Michal Luczaj wrote:
>> ...
>>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>>> index adcba1b7bf74..c093db8fec2d 100644
>>>> --- a/net/vmw_vsock/af_vsock.c
>>>> +++ b/net/vmw_vsock/af_vsock.c
>>>> @@ -1787,6 +1787,7 @@ static int vsock_accept(struct socket *sock, struct socket *newsock,
>>>> 		} else {
>>>> 			newsock->state = SS_CONNECTED;
>>>> 			sock_graft(connected, newsock);
>>>> +			set_bit(SOCK_CUSTOM_SOCKOPT, &newsock->flags);
>>>
>>> I was a bit confused about next lines calling set_bit on
>>> `connected->sk_socket->flags`, but after `sock_graft(connected,
>>> newsock)` they are equivalent.
>>>
>>> So, maybe I would move the new line before the sock_graft() call or use
>>> `connected->sk_socket->flags` if you want to keep it after it.
>> ...
>>>> 			if (vsock_msgzerocopy_allow(vconnected->transport))
>>>> 				set_bit(SOCK_SUPPORT_ZC,
>>>> 					&connected->sk_socket->flags);
>>
>> Hmm, isn't using both `connected->sk_socket->flags` and `newsock->flags` a
>> bit confusing?
> 
> Yep, for that reason I suggested to use `connected->sk_socket->flags`.
> 
>> `connected->sk_socket->flags` feels unnecessary long to me.
>> So how about a not-so-minimal-patch to have
>>
>> 	newsock->state = SS_CONNECTED;
>> 	set_bit(SOCK_CUSTOM_SOCKOPT, &newsock->flags);
>> 	if (vsock_msgzerocopy_allow(vconnected->transport))
>> 		set_bit(SOCK_SUPPORT_ZC, &newsock->flags);
>> 	sock_graft(connected, newsock);
> 
> No, please, this is a fix, so let's touch less as possible.
> 
> As I mentioned before, we have 2 options IMO:
> 1. use `set_bit(SOCK_CUSTOM_SOCKOPT, &newsock->flags);` but move it
>     before `sock_graft()`
> 2. use `connected->sk_socket->flags` and set it after `sock_graft()` if
>     we want to be a bit more consistent
> 
> I'd go with option 2, because I like to be consistent and it's less
> confusing IMHO, but I'm fine also with option 1.

Yeah, all right, here it is:
https://lore.kernel.org/netdev/20251229-vsock-child-sock-custom-sockopt-v2-0-64778d6c4f88@rbox.co/

Thanks,
Michal

[PATCH net 2/2] vsock/test: Test setting SO_ZEROCOPY on accept()ed socket

[Michal Luczaj]

Make sure setsockopt(SOL_SOCKET, SO_ZEROCOPY) on an accept()ed socket is
handled by vsock's implementation.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 tools/testing/vsock/vsock_test.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 9e1250790f33..8ec8f0844e22 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -2192,6 +2192,34 @@ static void test_stream_nolinger_server(const struct test_opts *opts)
 	close(fd);
 }
 
+static void test_stream_accepted_setsockopt_client(const struct test_opts *opts)
+{
+	int fd;
+
+	fd = vsock_stream_connect(opts->peer_cid, opts->peer_port);
+	if (fd < 0) {
+		perror("connect");
+		exit(EXIT_FAILURE);
+	}
+
+	vsock_wait_remote_close(fd);
+	close(fd);
+}
+
+static void test_stream_accepted_setsockopt_server(const struct test_opts *opts)
+{
+	int fd;
+
+	fd = vsock_stream_accept(VMADDR_CID_ANY, opts->peer_port, NULL);
+	if (fd < 0) {
+		perror("accept");
+		exit(EXIT_FAILURE);
+	}
+
+	enable_so_zerocopy_check(fd);
+	close(fd);
+}
+
 static struct test_case test_cases[] = {
 	{
 		.name = "SOCK_STREAM connection reset",
@@ -2371,6 +2399,11 @@ static struct test_case test_cases[] = {
 		.run_client = test_seqpacket_unread_bytes_client,
 		.run_server = test_seqpacket_unread_bytes_server,
 	},
+	{
+		.name = "SOCK_STREAM accept()ed socket custom setsockopt()",
+		.run_client = test_stream_accepted_setsockopt_client,
+		.run_server = test_stream_accepted_setsockopt_server,
+	},
 	{},
 };
 

-- 
2.52.0

Re: [PATCH net 2/2] vsock/test: Test setting SO_ZEROCOPY on accept()ed socket

[Stefano Garzarella]

On Tue, Dec 23, 2025 at 10:15:29AM +0100, Michal Luczaj wrote:
>Make sure setsockopt(SOL_SOCKET, SO_ZEROCOPY) on an accept()ed socket is
>handled by vsock's implementation.
>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> tools/testing/vsock/vsock_test.c | 33 +++++++++++++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
>
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index 9e1250790f33..8ec8f0844e22 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -2192,6 +2192,34 @@ static void test_stream_nolinger_server(const struct test_opts *opts)
> 	close(fd);
> }
>
>+static void test_stream_accepted_setsockopt_client(const struct test_opts *opts)
>+{
>+	int fd;
>+
>+	fd = vsock_stream_connect(opts->peer_cid, opts->peer_port);
>+	if (fd < 0) {
>+		perror("connect");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	vsock_wait_remote_close(fd);
>+	close(fd);
>+}
>+
>+static void test_stream_accepted_setsockopt_server(const struct test_opts *opts)
>+{
>+	int fd;
>+
>+	fd = vsock_stream_accept(VMADDR_CID_ANY, opts->peer_port, NULL);
>+	if (fd < 0) {
>+		perror("accept");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	enable_so_zerocopy_check(fd);

This test is passing on my env also without the patch applied.

Is that expected?

Thanks,
Stefano

>+	close(fd);
>+}
>+
> static struct test_case test_cases[] = {
> 	{
> 		.name = "SOCK_STREAM connection reset",
>@@ -2371,6 +2399,11 @@ static struct test_case test_cases[] = {
> 		.run_client = test_seqpacket_unread_bytes_client,
> 		.run_server = test_seqpacket_unread_bytes_server,
> 	},
>+	{
>+		.name = "SOCK_STREAM accept()ed socket custom setsockopt()",
>+		.run_client = test_stream_accepted_setsockopt_client,
>+		.run_server = test_stream_accepted_setsockopt_server,
>+	},
> 	{},
> };
>
>
>-- 
>2.52.0
>

Re: [PATCH net 2/2] vsock/test: Test setting SO_ZEROCOPY on accept()ed socket

[Michal Luczaj]

On 12/23/25 11:27, Stefano Garzarella wrote:
> On Tue, Dec 23, 2025 at 10:15:29AM +0100, Michal Luczaj wrote:
>> Make sure setsockopt(SOL_SOCKET, SO_ZEROCOPY) on an accept()ed socket is
>> handled by vsock's implementation.
>>
>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> ---
>> tools/testing/vsock/vsock_test.c | 33 +++++++++++++++++++++++++++++++++
>> 1 file changed, 33 insertions(+)
>>
>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>> index 9e1250790f33..8ec8f0844e22 100644
>> --- a/tools/testing/vsock/vsock_test.c
>> +++ b/tools/testing/vsock/vsock_test.c
>> @@ -2192,6 +2192,34 @@ static void test_stream_nolinger_server(const struct test_opts *opts)
>> 	close(fd);
>> }
>>
>> +static void test_stream_accepted_setsockopt_client(const struct test_opts *opts)
>> +{
>> +	int fd;
>> +
>> +	fd = vsock_stream_connect(opts->peer_cid, opts->peer_port);
>> +	if (fd < 0) {
>> +		perror("connect");
>> +		exit(EXIT_FAILURE);
>> +	}
>> +
>> +	vsock_wait_remote_close(fd);
>> +	close(fd);
>> +}
>> +
>> +static void test_stream_accepted_setsockopt_server(const struct test_opts *opts)
>> +{
>> +	int fd;
>> +
>> +	fd = vsock_stream_accept(VMADDR_CID_ANY, opts->peer_port, NULL);
>> +	if (fd < 0) {
>> +		perror("accept");
>> +		exit(EXIT_FAILURE);
>> +	}
>> +
>> +	enable_so_zerocopy_check(fd);
> 
> This test is passing on my env also without the patch applied.
> 
> Is that expected?

Oh, no, definitely not. It fails for me:
36 - SOCK_STREAM accept()ed socket custom setsockopt()...36 - SOCK_STREAM
accept()ed socket custom setsockopt()...setsockopt err: Operation not
supported (95)
setsockopt SO_ZEROCOPY val 1

I have no idea what's going on :)

Re: [PATCH net 2/2] vsock/test: Test setting SO_ZEROCOPY on accept()ed socket

[Stefano Garzarella]

On Tue, Dec 23, 2025 at 12:10:25PM +0100, Michal Luczaj wrote:
>On 12/23/25 11:27, Stefano Garzarella wrote:
>> On Tue, Dec 23, 2025 at 10:15:29AM +0100, Michal Luczaj wrote:
>>> Make sure setsockopt(SOL_SOCKET, SO_ZEROCOPY) on an accept()ed socket is
>>> handled by vsock's implementation.
>>>
>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>> ---
>>> tools/testing/vsock/vsock_test.c | 33 +++++++++++++++++++++++++++++++++
>>> 1 file changed, 33 insertions(+)
>>>
>>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>>> index 9e1250790f33..8ec8f0844e22 100644
>>> --- a/tools/testing/vsock/vsock_test.c
>>> +++ b/tools/testing/vsock/vsock_test.c
>>> @@ -2192,6 +2192,34 @@ static void test_stream_nolinger_server(const struct test_opts *opts)
>>> 	close(fd);
>>> }
>>>
>>> +static void test_stream_accepted_setsockopt_client(const struct test_opts *opts)
>>> +{
>>> +	int fd;
>>> +
>>> +	fd = vsock_stream_connect(opts->peer_cid, opts->peer_port);
>>> +	if (fd < 0) {
>>> +		perror("connect");
>>> +		exit(EXIT_FAILURE);
>>> +	}
>>> +
>>> +	vsock_wait_remote_close(fd);
>>> +	close(fd);
>>> +}
>>> +
>>> +static void test_stream_accepted_setsockopt_server(const struct test_opts *opts)
>>> +{
>>> +	int fd;
>>> +
>>> +	fd = vsock_stream_accept(VMADDR_CID_ANY, opts->peer_port, NULL);
>>> +	if (fd < 0) {
>>> +		perror("accept");
>>> +		exit(EXIT_FAILURE);
>>> +	}
>>> +
>>> +	enable_so_zerocopy_check(fd);
>>
>> This test is passing on my env also without the patch applied.
>>
>> Is that expected?
>
>Oh, no, definitely not. It fails for me:
>36 - SOCK_STREAM accept()ed socket custom setsockopt()...36 - SOCK_STREAM
>accept()ed socket custom setsockopt()...setsockopt err: Operation not
>supported (95)
>setsockopt SO_ZEROCOPY val 1

aaa, right, the server is failing, sorry ;-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>
>I have no idea what's going on :)
>

In my suite, I'm checking the client, and if the last test fails only on 
the server, I'm missing it. I'd fix my suite, and maybe also vsock_test 
adding another sync point.

Thanks,
Stefano

Re: [PATCH net 2/2] vsock/test: Test setting SO_ZEROCOPY on accept()ed socket

[Stefano Garzarella]

On Tue, Dec 23, 2025 at 02:20:33PM +0100, Stefano Garzarella wrote:
>On Tue, Dec 23, 2025 at 12:10:25PM +0100, Michal Luczaj wrote:
>>On 12/23/25 11:27, Stefano Garzarella wrote:
>>>On Tue, Dec 23, 2025 at 10:15:29AM +0100, Michal Luczaj wrote:
>>>>Make sure setsockopt(SOL_SOCKET, SO_ZEROCOPY) on an accept()ed socket is
>>>>handled by vsock's implementation.
>>>>
>>>>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>>>---
>>>>tools/testing/vsock/vsock_test.c | 33 +++++++++++++++++++++++++++++++++
>>>>1 file changed, 33 insertions(+)
>>>>
>>>>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>>>>index 9e1250790f33..8ec8f0844e22 100644
>>>>--- a/tools/testing/vsock/vsock_test.c
>>>>+++ b/tools/testing/vsock/vsock_test.c
>>>>@@ -2192,6 +2192,34 @@ static void test_stream_nolinger_server(const struct test_opts *opts)
>>>>	close(fd);
>>>>}
>>>>
>>>>+static void test_stream_accepted_setsockopt_client(const struct test_opts *opts)
>>>>+{
>>>>+	int fd;
>>>>+
>>>>+	fd = vsock_stream_connect(opts->peer_cid, opts->peer_port);
>>>>+	if (fd < 0) {
>>>>+		perror("connect");
>>>>+		exit(EXIT_FAILURE);
>>>>+	}
>>>>+
>>>>+	vsock_wait_remote_close(fd);

On a second look, why we need to wait the remote close?
can we just have a control message?

I'm not sure even on that, I mean why this peer can't close the
connection while the other is checking if it's able to set zerocopy?


>>>>+	close(fd);
>>>>+}
>>>>+
>>>>+static void test_stream_accepted_setsockopt_server(const struct test_opts *opts)
>>>>+{
>>>>+	int fd;
>>>>+
>>>>+	fd = vsock_stream_accept(VMADDR_CID_ANY, opts->peer_port, NULL);
>>>>+	if (fd < 0) {
>>>>+		perror("accept");
>>>>+		exit(EXIT_FAILURE);
>>>>+	}
>>>>+
>>>>+	enable_so_zerocopy_check(fd);
>>>
>>>This test is passing on my env also without the patch applied.
>>>
>>>Is that expected?
>>
>>Oh, no, definitely not. It fails for me:
>>36 - SOCK_STREAM accept()ed socket custom setsockopt()...36 - SOCK_STREAM
>>accept()ed socket custom setsockopt()...setsockopt err: Operation not
>>supported (95)
>>setsockopt SO_ZEROCOPY val 1
>
>aaa, right, the server is failing, sorry ;-)
>
>Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>>
>>I have no idea what's going on :)
>>
>
>In my suite, I'm checking the client, and if the last test fails only 
>on the server, I'm missing it. I'd fix my suite, and maybe also 
>vsock_test adding another sync point.

Added a full barrier here: 
https://lore.kernel.org/netdev/20251223162210.43976-1-sgarzare@redhat.com

Thanks,
Stefano

Re: [PATCH net 2/2] vsock/test: Test setting SO_ZEROCOPY on accept()ed socket

[Michal Luczaj]

On 12/23/25 17:50, Stefano Garzarella wrote:
> On Tue, Dec 23, 2025 at 02:20:33PM +0100, Stefano Garzarella wrote:
>> On Tue, Dec 23, 2025 at 12:10:25PM +0100, Michal Luczaj wrote:
>>> On 12/23/25 11:27, Stefano Garzarella wrote:
>>>> On Tue, Dec 23, 2025 at 10:15:29AM +0100, Michal Luczaj wrote:
>>>>> Make sure setsockopt(SOL_SOCKET, SO_ZEROCOPY) on an accept()ed socket is
>>>>> handled by vsock's implementation.
>>>>>
>>>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>>>> ---
>>>>> tools/testing/vsock/vsock_test.c | 33 +++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 33 insertions(+)
>>>>>
>>>>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>>>>> index 9e1250790f33..8ec8f0844e22 100644
>>>>> --- a/tools/testing/vsock/vsock_test.c
>>>>> +++ b/tools/testing/vsock/vsock_test.c
>>>>> @@ -2192,6 +2192,34 @@ static void test_stream_nolinger_server(const struct test_opts *opts)
>>>>> 	close(fd);
>>>>> }
>>>>>
>>>>> +static void test_stream_accepted_setsockopt_client(const struct test_opts *opts)
>>>>> +{
>>>>> +	int fd;
>>>>> +
>>>>> +	fd = vsock_stream_connect(opts->peer_cid, opts->peer_port);
>>>>> +	if (fd < 0) {
>>>>> +		perror("connect");
>>>>> +		exit(EXIT_FAILURE);
>>>>> +	}
>>>>> +
>>>>> +	vsock_wait_remote_close(fd);
> 
> On a second look, why we need to wait the remote close?
> can we just have a control message?

I think we can. I've used vsock_wait_remote_close() simply as a sync
primitive. It's one line of code less.

> I'm not sure even on that, I mean why this peer can't close the
> connection while the other is checking if it's able to set zerocopy?

I was worried that without any sync, client-side close() may race
server-side accept(), but I've just checked and it doesn't seem to cause
any issues, at least for the virtio transports.

>>>>> +	close(fd);
>>>>> +}
>>>>> +
>>>>> +static void test_stream_accepted_setsockopt_server(const struct test_opts *opts)
>>>>> +{
>>>>> +	int fd;
>>>>> +
>>>>> +	fd = vsock_stream_accept(VMADDR_CID_ANY, opts->peer_port, NULL);
>>>>> +	if (fd < 0) {
>>>>> +		perror("accept");
>>>>> +		exit(EXIT_FAILURE);
>>>>> +	}
>>>>> +
>>>>> +	enable_so_zerocopy_check(fd);
>>>>
>>>> This test is passing on my env also without the patch applied.
>>>>
>>>> Is that expected?
>>>
>>> Oh, no, definitely not. It fails for me:
>>> 36 - SOCK_STREAM accept()ed socket custom setsockopt()...36 - SOCK_STREAM
>>> accept()ed socket custom setsockopt()...setsockopt err: Operation not
>>> supported (95)
>>> setsockopt SO_ZEROCOPY val 1
>>
>> aaa, right, the server is failing, sorry ;-)
>>
>> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>>>
>>> I have no idea what's going on :)
>>>
>>
>> In my suite, I'm checking the client, and if the last test fails only 
>> on the server, I'm missing it. I'd fix my suite, and maybe also 
>> vsock_test adding another sync point.
> 
> Added a full barrier here: 
> https://lore.kernel.org/netdev/20251223162210.43976-1-sgarzare@redhat.com

Which reminds me of discussion in
https://lore.kernel.org/netdev/151bf5fe-c9ca-4244-aa21-8d7b8ff2470f@rbox.co/
. Sorry for postponing, I've put it on my vsock-cleanups branch and kept
adding more little fixes, and it was never the right time to post the series.

Re: [PATCH net 2/2] vsock/test: Test setting SO_ZEROCOPY on accept()ed socket

[Stefano Garzarella]

On Tue, Dec 23, 2025 at 09:38:07PM +0100, Michal Luczaj wrote:
>On 12/23/25 17:50, Stefano Garzarella wrote:
>> On Tue, Dec 23, 2025 at 02:20:33PM +0100, Stefano Garzarella wrote:
>>> On Tue, Dec 23, 2025 at 12:10:25PM +0100, Michal Luczaj wrote:
>>>> On 12/23/25 11:27, Stefano Garzarella wrote:
>>>>> On Tue, Dec 23, 2025 at 10:15:29AM +0100, Michal Luczaj wrote:
>>>>>> Make sure setsockopt(SOL_SOCKET, SO_ZEROCOPY) on an accept()ed socket is
>>>>>> handled by vsock's implementation.
>>>>>>
>>>>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>>>>> ---
>>>>>> tools/testing/vsock/vsock_test.c | 33 +++++++++++++++++++++++++++++++++
>>>>>> 1 file changed, 33 insertions(+)
>>>>>>
>>>>>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>>>>>> index 9e1250790f33..8ec8f0844e22 100644
>>>>>> --- a/tools/testing/vsock/vsock_test.c
>>>>>> +++ b/tools/testing/vsock/vsock_test.c
>>>>>> @@ -2192,6 +2192,34 @@ static void test_stream_nolinger_server(const struct test_opts *opts)
>>>>>> 	close(fd);
>>>>>> }
>>>>>>
>>>>>> +static void test_stream_accepted_setsockopt_client(const struct test_opts *opts)
>>>>>> +{
>>>>>> +	int fd;
>>>>>> +
>>>>>> +	fd = vsock_stream_connect(opts->peer_cid, opts->peer_port);
>>>>>> +	if (fd < 0) {
>>>>>> +		perror("connect");
>>>>>> +		exit(EXIT_FAILURE);
>>>>>> +	}
>>>>>> +
>>>>>> +	vsock_wait_remote_close(fd);
>>
>> On a second look, why we need to wait the remote close?
>> can we just have a control message?
>
>I think we can. I've used vsock_wait_remote_close() simply as a sync
>primitive. It's one line of code less.
>
>> I'm not sure even on that, I mean why this peer can't close the
>> connection while the other is checking if it's able to set zerocopy?
>
>I was worried that without any sync, client-side close() may race
>server-side accept(), but I've just checked and it doesn't seem to cause
>any issues, at least for the virtio transports.

Okay, I see. Feel free to leave it, but if it's not really needed, I'd 
prefer to keep the tests as simple as possible.

>
>>>>>> +	close(fd);
>>>>>> +}
>>>>>> +
>>>>>> +static void test_stream_accepted_setsockopt_server(const struct test_opts *opts)
>>>>>> +{
>>>>>> +	int fd;
>>>>>> +
>>>>>> +	fd = vsock_stream_accept(VMADDR_CID_ANY, opts->peer_port, NULL);
>>>>>> +	if (fd < 0) {
>>>>>> +		perror("accept");
>>>>>> +		exit(EXIT_FAILURE);
>>>>>> +	}
>>>>>> +
>>>>>> +	enable_so_zerocopy_check(fd);
>>>>>
>>>>> This test is passing on my env also without the patch applied.
>>>>>
>>>>> Is that expected?
>>>>
>>>> Oh, no, definitely not. It fails for me:
>>>> 36 - SOCK_STREAM accept()ed socket custom setsockopt()...36 - SOCK_STREAM
>>>> accept()ed socket custom setsockopt()...setsockopt err: Operation not
>>>> supported (95)
>>>> setsockopt SO_ZEROCOPY val 1
>>>
>>> aaa, right, the server is failing, sorry ;-)
>>>
>>> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>>>>
>>>> I have no idea what's going on :)
>>>>
>>>
>>> In my suite, I'm checking the client, and if the last test fails only
>>> on the server, I'm missing it. I'd fix my suite, and maybe also
>>> vsock_test adding another sync point.
>>
>> Added a full barrier here:
>> https://lore.kernel.org/netdev/20251223162210.43976-1-sgarzare@redhat.com
>
>Which reminds me of discussion in
>https://lore.kernel.org/netdev/151bf5fe-c9ca-4244-aa21-8d7b8ff2470f@rbox.co/

Oh, I forgot that we already discussed that.

My first attempt was exactly that, but then discovered that it didn't 
add too much except for the last one since for the others we have 2 full 
barriers back to back, so I preferred to move outside the loop. In that 
way we can also be sure the 2 `vsock_tests` are in sync with the amount 
of tests to run.

But yeah, also that one fix the issue.

>. Sorry for postponing, I've put it on my vsock-cleanups branch and kept
>adding more little fixes, and it was never the right time to post the series.
>

Nah, don't apologize, you're doing an amazing job!

Thanks,
Stefano

Re: [PATCH net 2/2] vsock/test: Test setting SO_ZEROCOPY on accept()ed socket

[Michal Luczaj]

On 12/24/25 10:15, Stefano Garzarella wrote:
> On Tue, Dec 23, 2025 at 09:38:07PM +0100, Michal Luczaj wrote:
>> On 12/23/25 17:50, Stefano Garzarella wrote:
>>> On Tue, Dec 23, 2025 at 02:20:33PM +0100, Stefano Garzarella wrote:
>>>> On Tue, Dec 23, 2025 at 12:10:25PM +0100, Michal Luczaj wrote:
>>>>> On 12/23/25 11:27, Stefano Garzarella wrote:
>>>>>> On Tue, Dec 23, 2025 at 10:15:29AM +0100, Michal Luczaj wrote:
>>>>>>> Make sure setsockopt(SOL_SOCKET, SO_ZEROCOPY) on an accept()ed socket is
>>>>>>> handled by vsock's implementation.
>>>>>>>
>>>>>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>>>>>> ---
>>>>>>> tools/testing/vsock/vsock_test.c | 33 +++++++++++++++++++++++++++++++++
>>>>>>> 1 file changed, 33 insertions(+)
>>>>>>>
>>>>>>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>>>>>>> index 9e1250790f33..8ec8f0844e22 100644
>>>>>>> --- a/tools/testing/vsock/vsock_test.c
>>>>>>> +++ b/tools/testing/vsock/vsock_test.c
>>>>>>> @@ -2192,6 +2192,34 @@ static void test_stream_nolinger_server(const struct test_opts *opts)
>>>>>>> 	close(fd);
>>>>>>> }
>>>>>>>
>>>>>>> +static void test_stream_accepted_setsockopt_client(const struct test_opts *opts)
>>>>>>> +{
>>>>>>> +	int fd;
>>>>>>> +
>>>>>>> +	fd = vsock_stream_connect(opts->peer_cid, opts->peer_port);
>>>>>>> +	if (fd < 0) {
>>>>>>> +		perror("connect");
>>>>>>> +		exit(EXIT_FAILURE);
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	vsock_wait_remote_close(fd);
>>>
>>> On a second look, why we need to wait the remote close?
>>> can we just have a control message?
>>
>> I think we can. I've used vsock_wait_remote_close() simply as a sync
>> primitive. It's one line of code less.
>>
>>> I'm not sure even on that, I mean why this peer can't close the
>>> connection while the other is checking if it's able to set zerocopy?
>>
>> I was worried that without any sync, client-side close() may race
>> server-side accept(), but I've just checked and it doesn't seem to cause
>> any issues, at least for the virtio transports.
> 
> Okay, I see. Feel free to leave it, but if it's not really needed, I'd 
> prefer to keep the tests as simple as possible.

OK, dropping the sync here. It will be interesting to see if it ever blows up.

...
>>>> In my suite, I'm checking the client, and if the last test fails only
>>>> on the server, I'm missing it. I'd fix my suite, and maybe also
>>>> vsock_test adding another sync point.
>>>
>>> Added a full barrier here:
>>> https://lore.kernel.org/netdev/20251223162210.43976-1-sgarzare@redhat.com
>>
>> Which reminds me of discussion in
>> https://lore.kernel.org/netdev/151bf5fe-c9ca-4244-aa21-8d7b8ff2470f@rbox.co/
> 
> Oh, I forgot that we already discussed that.
> 
> My first attempt was exactly that, but then discovered that it didn't 
> add too much except for the last one since for the others we have 2 full 
> barriers back to back, so I preferred to move outside the loop. In that 
> way we can also be sure the 2 `vsock_tests` are in sync with the amount 
> of tests to run.

Might it be that we're solving different issues?

I was annoyed by the next test's name/prompt being printed when the
previous test is still running on the other side. Which happens e.g. when
one side takes longer than the other. Or when one of the sides is
unimplemented.

How about something like below; would that cover your case as well?

diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c
index d843643ced6b..5d94ffd2fa82 100644
--- a/tools/testing/vsock/util.c
+++ b/tools/testing/vsock/util.c
@@ -495,7 +495,7 @@ void run_tests(const struct test_case *test_cases,
 			printf("skipped\n");

 			free(line);
-			continue;
+			goto sync;
 		}

 		control_cmpln(line, "NEXT", true);
@@ -510,6 +510,9 @@ void run_tests(const struct test_case *test_cases,
 			run(opts);

 		printf("ok\n");
+sync:
+		control_writeln("RUN_TESTS_SYNC");
+		control_expectln("RUN_TESTS_SYNC");
 	}
 }