[PATCH net-next v2 0/3] vsock: SOCK_LINGER rework

[PATCH net-next v2 0/3] vsock: SOCK_LINGER rework

[Michal Luczaj]

Change vsock's lingerning to wait on close() until all data is sent, i.e.
until workers picked all the packets for processing.

Changes in v2:
- Comment that some transports do not implement unsent_bytes [Stefano]
- Reduce the indentation of virtio_transport_wait_close() [Stefano] 
- Do not linger on shutdown(), expand the commit messages [Paolo]
- Link to v1: https://lore.kernel.org/r/20250407-vsock-linger-v1-0-1458038e3492@rbox.co

Changes in v1:
- Do not assume `unsent_bytes()` is implemented by all transports [Stefano]
- Link to v0: https://lore.kernel.org/netdev/df2d51fd-03e7-477f-8aea-938446f47864@rbox.co/

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
Michal Luczaj (3):
      vsock: Linger on unsent data
      vsock: Reduce indentation in virtio_transport_wait_close()
      vsock/test: Expand linger test to ensure close() does not misbehave

 net/vmw_vsock/virtio_transport_common.c | 29 +++++++++++++++++++----------
 tools/testing/vsock/vsock_test.c        | 30 +++++++++++++++++++++++++++---
 2 files changed, 46 insertions(+), 13 deletions(-)
---
base-commit: 8066e388be48f1ad62b0449dc1d31a25489fa12a
change-id: 20250304-vsock-linger-9026e5f9986c

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

[PATCH net-next v2 1/3] vsock: Linger on unsent data

[Michal Luczaj]

Currently vsock's lingering effectively boils down to waiting (or timing
out) until packets are consumed or dropped by the peer; be it by receiving
the data, closing or shutting down the connection.

To align with the semantics described in the SO_LINGER section of man
socket(7) and to mimic AF_INET's behaviour more closely, change the logic
of a lingering close(): instead of waiting for all data to be handled,
block until data is considered sent from the vsock's transport point of
view. That is until worker picks the packets for processing and decrements
virtio_vsock_sock::bytes_unsent down to 0.

Note that such lingering is limited to transports that actually implement
vsock_transport::unsent_bytes() callback. This excludes Hyper-V and VMCI,
under which no lingering would be observed.

The implementation does not adhere strictly to man page's interpretation of
SO_LINGER: shutdown() will not trigger the lingering. This follows AF_INET.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/virtio_transport_common.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 7f7de6d8809655fe522749fbbc9025df71f071bd..aeb7f3794f7cfc251dde878cb44fdcc54814c89c 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -1196,12 +1196,21 @@ static void virtio_transport_wait_close(struct sock *sk, long timeout)
 {
 	if (timeout) {
 		DEFINE_WAIT_FUNC(wait, woken_wake_function);
+		ssize_t (*unsent)(struct vsock_sock *vsk);
+		struct vsock_sock *vsk = vsock_sk(sk);
+
+		/* Some transports (Hyper-V, VMCI) do not implement
+		 * unsent_bytes. For those, no lingering on close().
+		 */
+		unsent = vsk->transport->unsent_bytes;
+		if (!unsent)
+			return;
 
 		add_wait_queue(sk_sleep(sk), &wait);
 
 		do {
-			if (sk_wait_event(sk, &timeout,
-					  sock_flag(sk, SOCK_DONE), &wait))
+			if (sk_wait_event(sk, &timeout, unsent(vsk) == 0,
+					  &wait))
 				break;
 		} while (!signal_pending(current) && timeout);
 

-- 
2.49.0

Re: [PATCH net-next v2 1/3] vsock: Linger on unsent data

[Luigi Leonardi]

Hi Michal,

On Mon, Apr 21, 2025 at 11:50:41PM +0200, Michal Luczaj wrote:
>Currently vsock's lingering effectively boils down to waiting (or timing
>out) until packets are consumed or dropped by the peer; be it by receiving
>the data, closing or shutting down the connection.
>
>To align with the semantics described in the SO_LINGER section of man
>socket(7) and to mimic AF_INET's behaviour more closely, change the logic
>of a lingering close(): instead of waiting for all data to be handled,
>block until data is considered sent from the vsock's transport point of
>view. That is until worker picks the packets for processing and decrements
>virtio_vsock_sock::bytes_unsent down to 0.
>
>Note that such lingering is limited to transports that actually implement
>vsock_transport::unsent_bytes() callback. This excludes Hyper-V and VMCI,
>under which no lingering would be observed.
>
>The implementation does not adhere strictly to man page's interpretation of
>SO_LINGER: shutdown() will not trigger the lingering. This follows AF_INET.
>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> net/vmw_vsock/virtio_transport_common.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index 7f7de6d8809655fe522749fbbc9025df71f071bd..aeb7f3794f7cfc251dde878cb44fdcc54814c89c 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -1196,12 +1196,21 @@ static void virtio_transport_wait_close(struct sock *sk, long timeout)
> {
> 	if (timeout) {
> 		DEFINE_WAIT_FUNC(wait, woken_wake_function);
>+		ssize_t (*unsent)(struct vsock_sock *vsk);
>+		struct vsock_sock *vsk = vsock_sk(sk);
>+
>+		/* Some transports (Hyper-V, VMCI) do not implement
>+		 * unsent_bytes. For those, no lingering on close().
>+		 */
>+		unsent = vsk->transport->unsent_bytes;
>+		if (!unsent)
>+			return;

IIUC if `unsent_bytes` is not implemented, virtio_transport_wait_close 
basically does nothing. My concern is that we are breaking the userspace 
due to a change in the behavior: Before this patch, with a vmci/hyper-v 
transport, this function would wait for SOCK_DONE to be set, but not 
anymore.

>
> 		add_wait_queue(sk_sleep(sk), &wait);
>
> 		do {
>-			if (sk_wait_event(sk, &timeout,
>-					  sock_flag(sk, SOCK_DONE), 
>&wait))
>+			if (sk_wait_event(sk, &timeout, unsent(vsk) == 
>0,
>+					  &wait))
> 				break;
> 		} while (!signal_pending(current) && timeout);
>
>
>-- 2.49.0
>

Thanks,
Luigi

Re: [PATCH net-next v2 1/3] vsock: Linger on unsent data

[Stefano Garzarella]

On Wed, Apr 23, 2025 at 05:53:12PM +0200, Luigi Leonardi wrote:
>Hi Michal,
>
>On Mon, Apr 21, 2025 at 11:50:41PM +0200, Michal Luczaj wrote:
>>Currently vsock's lingering effectively boils down to waiting (or timing
>>out) until packets are consumed or dropped by the peer; be it by receiving
>>the data, closing or shutting down the connection.
>>
>>To align with the semantics described in the SO_LINGER section of man
>>socket(7) and to mimic AF_INET's behaviour more closely, change the logic
>>of a lingering close(): instead of waiting for all data to be handled,
>>block until data is considered sent from the vsock's transport point of
>>view. That is until worker picks the packets for processing and decrements
>>virtio_vsock_sock::bytes_unsent down to 0.
>>
>>Note that such lingering is limited to transports that actually implement
>>vsock_transport::unsent_bytes() callback. This excludes Hyper-V and VMCI,
>>under which no lingering would be observed.
>>
>>The implementation does not adhere strictly to man page's interpretation of
>>SO_LINGER: shutdown() will not trigger the lingering. This follows AF_INET.
>>
>>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>---
>>net/vmw_vsock/virtio_transport_common.c | 13 +++++++++++--
>>1 file changed, 11 insertions(+), 2 deletions(-)
>>
>>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>index 7f7de6d8809655fe522749fbbc9025df71f071bd..aeb7f3794f7cfc251dde878cb44fdcc54814c89c 100644
>>--- a/net/vmw_vsock/virtio_transport_common.c
>>+++ b/net/vmw_vsock/virtio_transport_common.c
>>@@ -1196,12 +1196,21 @@ static void virtio_transport_wait_close(struct sock *sk, long timeout)
>>{
>>	if (timeout) {
>>		DEFINE_WAIT_FUNC(wait, woken_wake_function);
>>+		ssize_t (*unsent)(struct vsock_sock *vsk);
>>+		struct vsock_sock *vsk = vsock_sk(sk);
>>+
>>+		/* Some transports (Hyper-V, VMCI) do not implement
>>+		 * unsent_bytes. For those, no lingering on close().
>>+		 */
>>+		unsent = vsk->transport->unsent_bytes;
>>+		if (!unsent)
>>+			return;
>
>IIUC if `unsent_bytes` is not implemented, virtio_transport_wait_close 
>basically does nothing. My concern is that we are breaking the 
>userspace due to a change in the behavior: Before this patch, with a 
>vmci/hyper-v transport, this function would wait for SOCK_DONE to be 
>set, but not anymore.

Wait, we are in virtio_transport_common.c, why we are talking about 
Hyper-V and VMCI?

I asked to check `vsk->transport->unsent_bytes` in the v1, because this 
code was part of af_vsock.c, but now we are back to virtio code, so I'm 
confused...

Stefano

>
>>
>>		add_wait_queue(sk_sleep(sk), &wait);
>>
>>		do {
>>-			if (sk_wait_event(sk, &timeout,
>>-					  sock_flag(sk, SOCK_DONE), &wait))
>>+			if (sk_wait_event(sk, &timeout, unsent(vsk) == 0,
>>+					  &wait))
>>				break;
>>		} while (!signal_pending(current) && timeout);
>>
>>
>>-- 2.49.0
>>
>
>Thanks,
>Luigi
>

Re: [PATCH net-next v2 1/3] vsock: Linger on unsent data

[Michal Luczaj]

On 4/23/25 18:34, Stefano Garzarella wrote:
> On Wed, Apr 23, 2025 at 05:53:12PM +0200, Luigi Leonardi wrote:
>> Hi Michal,
>>
>> On Mon, Apr 21, 2025 at 11:50:41PM +0200, Michal Luczaj wrote:
>>> Currently vsock's lingering effectively boils down to waiting (or timing
>>> out) until packets are consumed or dropped by the peer; be it by receiving
>>> the data, closing or shutting down the connection.
>>>
>>> To align with the semantics described in the SO_LINGER section of man
>>> socket(7) and to mimic AF_INET's behaviour more closely, change the logic
>>> of a lingering close(): instead of waiting for all data to be handled,
>>> block until data is considered sent from the vsock's transport point of
>>> view. That is until worker picks the packets for processing and decrements
>>> virtio_vsock_sock::bytes_unsent down to 0.
>>>
>>> Note that such lingering is limited to transports that actually implement
>>> vsock_transport::unsent_bytes() callback. This excludes Hyper-V and VMCI,
>>> under which no lingering would be observed.
>>>
>>> The implementation does not adhere strictly to man page's interpretation of
>>> SO_LINGER: shutdown() will not trigger the lingering. This follows AF_INET.
>>>
>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>> ---
>>> net/vmw_vsock/virtio_transport_common.c | 13 +++++++++++--
>>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>> index 7f7de6d8809655fe522749fbbc9025df71f071bd..aeb7f3794f7cfc251dde878cb44fdcc54814c89c 100644
>>> --- a/net/vmw_vsock/virtio_transport_common.c
>>> +++ b/net/vmw_vsock/virtio_transport_common.c
>>> @@ -1196,12 +1196,21 @@ static void virtio_transport_wait_close(struct sock *sk, long timeout)
>>> {
>>> 	if (timeout) {
>>> 		DEFINE_WAIT_FUNC(wait, woken_wake_function);
>>> +		ssize_t (*unsent)(struct vsock_sock *vsk);
>>> +		struct vsock_sock *vsk = vsock_sk(sk);
>>> +
>>> +		/* Some transports (Hyper-V, VMCI) do not implement
>>> +		 * unsent_bytes. For those, no lingering on close().
>>> +		 */
>>> +		unsent = vsk->transport->unsent_bytes;
>>> +		if (!unsent)
>>> +			return;
>>
>> IIUC if `unsent_bytes` is not implemented, virtio_transport_wait_close 
>> basically does nothing. My concern is that we are breaking the 
>> userspace due to a change in the behavior: Before this patch, with a 
>> vmci/hyper-v transport, this function would wait for SOCK_DONE to be 
>> set, but not anymore.
> 
> Wait, we are in virtio_transport_common.c, why we are talking about 
> Hyper-V and VMCI?
> 
> I asked to check `vsk->transport->unsent_bytes` in the v1, because this 
> code was part of af_vsock.c, but now we are back to virtio code, so I'm 
> confused...

Might your confusion be because of similar names?
vsock_transport::unsent_bytes != virtio_vsock_sock::bytes_unsent

I agree with Luigi, it is a breaking change for userspace depending on a
non-standard behaviour. What's the protocol here; do it anyway, then see if
anyone complains?

As for Hyper-V and VMCI losing the "lingering", do we care? And if we do,
take Hyper-V, is it possible to test any changes without access to
proprietary host/hypervisor?

Re: [PATCH net-next v2 1/3] vsock: Linger on unsent data

[Stefano Garzarella]

On Wed, Apr 23, 2025 at 11:06:33PM +0200, Michal Luczaj wrote:
>On 4/23/25 18:34, Stefano Garzarella wrote:
>> On Wed, Apr 23, 2025 at 05:53:12PM +0200, Luigi Leonardi wrote:
>>> Hi Michal,
>>>
>>> On Mon, Apr 21, 2025 at 11:50:41PM +0200, Michal Luczaj wrote:
>>>> Currently vsock's lingering effectively boils down to waiting (or timing
>>>> out) until packets are consumed or dropped by the peer; be it by receiving
>>>> the data, closing or shutting down the connection.
>>>>
>>>> To align with the semantics described in the SO_LINGER section of man
>>>> socket(7) and to mimic AF_INET's behaviour more closely, change the logic
>>>> of a lingering close(): instead of waiting for all data to be handled,
>>>> block until data is considered sent from the vsock's transport point of
>>>> view. That is until worker picks the packets for processing and decrements
>>>> virtio_vsock_sock::bytes_unsent down to 0.
>>>>
>>>> Note that such lingering is limited to transports that actually implement
>>>> vsock_transport::unsent_bytes() callback. This excludes Hyper-V and VMCI,
>>>> under which no lingering would be observed.
>>>>
>>>> The implementation does not adhere strictly to man page's interpretation of
>>>> SO_LINGER: shutdown() will not trigger the lingering. This follows AF_INET.
>>>>
>>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>>> ---
>>>> net/vmw_vsock/virtio_transport_common.c | 13 +++++++++++--
>>>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>>> index 7f7de6d8809655fe522749fbbc9025df71f071bd..aeb7f3794f7cfc251dde878cb44fdcc54814c89c 100644
>>>> --- a/net/vmw_vsock/virtio_transport_common.c
>>>> +++ b/net/vmw_vsock/virtio_transport_common.c
>>>> @@ -1196,12 +1196,21 @@ static void virtio_transport_wait_close(struct sock *sk, long timeout)
>>>> {
>>>> 	if (timeout) {
>>>> 		DEFINE_WAIT_FUNC(wait, woken_wake_function);
>>>> +		ssize_t (*unsent)(struct vsock_sock *vsk);
>>>> +		struct vsock_sock *vsk = vsock_sk(sk);
>>>> +
>>>> +		/* Some transports (Hyper-V, VMCI) do not implement
>>>> +		 * unsent_bytes. For those, no lingering on close().
>>>> +		 */
>>>> +		unsent = vsk->transport->unsent_bytes;
>>>> +		if (!unsent)
>>>> +			return;
>>>
>>> IIUC if `unsent_bytes` is not implemented, virtio_transport_wait_close
>>> basically does nothing. My concern is that we are breaking the
>>> userspace due to a change in the behavior: Before this patch, with a
>>> vmci/hyper-v transport, this function would wait for SOCK_DONE to be
>>> set, but not anymore.
>>
>> Wait, we are in virtio_transport_common.c, why we are talking about
>> Hyper-V and VMCI?
>>
>> I asked to check `vsk->transport->unsent_bytes` in the v1, because this
>> code was part of af_vsock.c, but now we are back to virtio code, so I'm
>> confused...
>
>Might your confusion be because of similar names?

In v1 this code IIRC was in af_vsock.c, now you pushed back on virtio 
common code, so I still don't understand how 
virtio_transport_wait_close() can be called with vmci or hyper-v 
transports.

Can you provide an example?

>vsock_transport::unsent_bytes != virtio_vsock_sock::bytes_unsent
>
>I agree with Luigi, it is a breaking change for userspace depending on a
>non-standard behaviour. What's the protocol here; do it anyway, then see if
>anyone complains?
>
>As for Hyper-V and VMCI losing the "lingering", do we care? And if we do,
>take Hyper-V, is it possible to test any changes without access to
>proprietary host/hypervisor?
>

Again, how this code can be called when using vmci or hyper-v 
transports?

If we go back on v1 implementation, I can understand it, but with this 
version I really don't understand the scenario.

Stefano

Re: [PATCH net-next v2 1/3] vsock: Linger on unsent data

[Michal Luczaj]

On 4/24/25 09:28, Stefano Garzarella wrote:
> On Wed, Apr 23, 2025 at 11:06:33PM +0200, Michal Luczaj wrote:
>> On 4/23/25 18:34, Stefano Garzarella wrote:
>>> On Wed, Apr 23, 2025 at 05:53:12PM +0200, Luigi Leonardi wrote:
>>>> Hi Michal,
>>>>
>>>> On Mon, Apr 21, 2025 at 11:50:41PM +0200, Michal Luczaj wrote:
>>>>> Currently vsock's lingering effectively boils down to waiting (or timing
>>>>> out) until packets are consumed or dropped by the peer; be it by receiving
>>>>> the data, closing or shutting down the connection.
>>>>>
>>>>> To align with the semantics described in the SO_LINGER section of man
>>>>> socket(7) and to mimic AF_INET's behaviour more closely, change the logic
>>>>> of a lingering close(): instead of waiting for all data to be handled,
>>>>> block until data is considered sent from the vsock's transport point of
>>>>> view. That is until worker picks the packets for processing and decrements
>>>>> virtio_vsock_sock::bytes_unsent down to 0.
>>>>>
>>>>> Note that such lingering is limited to transports that actually implement
>>>>> vsock_transport::unsent_bytes() callback. This excludes Hyper-V and VMCI,
>>>>> under which no lingering would be observed.
>>>>>
>>>>> The implementation does not adhere strictly to man page's interpretation of
>>>>> SO_LINGER: shutdown() will not trigger the lingering. This follows AF_INET.
>>>>>
>>>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>>>> ---
>>>>> net/vmw_vsock/virtio_transport_common.c | 13 +++++++++++--
>>>>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>>>> index 7f7de6d8809655fe522749fbbc9025df71f071bd..aeb7f3794f7cfc251dde878cb44fdcc54814c89c 100644
>>>>> --- a/net/vmw_vsock/virtio_transport_common.c
>>>>> +++ b/net/vmw_vsock/virtio_transport_common.c
>>>>> @@ -1196,12 +1196,21 @@ static void virtio_transport_wait_close(struct sock *sk, long timeout)
>>>>> {
>>>>> 	if (timeout) {
>>>>> 		DEFINE_WAIT_FUNC(wait, woken_wake_function);
>>>>> +		ssize_t (*unsent)(struct vsock_sock *vsk);
>>>>> +		struct vsock_sock *vsk = vsock_sk(sk);
>>>>> +
>>>>> +		/* Some transports (Hyper-V, VMCI) do not implement
>>>>> +		 * unsent_bytes. For those, no lingering on close().
>>>>> +		 */
>>>>> +		unsent = vsk->transport->unsent_bytes;
>>>>> +		if (!unsent)
>>>>> +			return;
>>>>
>>>> IIUC if `unsent_bytes` is not implemented, virtio_transport_wait_close
>>>> basically does nothing. My concern is that we are breaking the
>>>> userspace due to a change in the behavior: Before this patch, with a
>>>> vmci/hyper-v transport, this function would wait for SOCK_DONE to be
>>>> set, but not anymore.
>>>
>>> Wait, we are in virtio_transport_common.c, why we are talking about
>>> Hyper-V and VMCI?
>>>
>>> I asked to check `vsk->transport->unsent_bytes` in the v1, because this
>>> code was part of af_vsock.c, but now we are back to virtio code, so I'm
>>> confused...
>>
>> Might your confusion be because of similar names?
> 
> In v1 this code IIRC was in af_vsock.c, now you pushed back on virtio 
> common code, so I still don't understand how 
> virtio_transport_wait_close() can be called with vmci or hyper-v 
> transports.
> 
> Can you provide an example?

You're right, it was me who was confused. VMCI and Hyper-V have their own
vsock_transport::release callbacks that do not call
virtio_transport_wait_close().

So VMCI and Hyper-V never lingered anyway?

>> vsock_transport::unsent_bytes != virtio_vsock_sock::bytes_unsent
>>
>> I agree with Luigi, it is a breaking change for userspace depending on a
>> non-standard behaviour. What's the protocol here; do it anyway, then see if
>> anyone complains?
>>
>> As for Hyper-V and VMCI losing the "lingering", do we care? And if we do,
>> take Hyper-V, is it possible to test any changes without access to
>> proprietary host/hypervisor?
>>
> 
> Again, how this code can be called when using vmci or hyper-v 
> transports?

It cannot, you're right.

> If we go back on v1 implementation, I can understand it, but with this 
> version I really don't understand the scenario.
> 
> Stefano
> 

Re: [PATCH net-next v2 1/3] vsock: Linger on unsent data

[Stefano Garzarella]

On Thu, 24 Apr 2025 at 09:53, Michal Luczaj <mhal@rbox.co> wrote:
>
> On 4/24/25 09:28, Stefano Garzarella wrote:
> > On Wed, Apr 23, 2025 at 11:06:33PM +0200, Michal Luczaj wrote:
> >> On 4/23/25 18:34, Stefano Garzarella wrote:
> >>> On Wed, Apr 23, 2025 at 05:53:12PM +0200, Luigi Leonardi wrote:
> >>>> Hi Michal,
> >>>>
> >>>> On Mon, Apr 21, 2025 at 11:50:41PM +0200, Michal Luczaj wrote:
> >>>>> Currently vsock's lingering effectively boils down to waiting (or timing
> >>>>> out) until packets are consumed or dropped by the peer; be it by receiving
> >>>>> the data, closing or shutting down the connection.
> >>>>>
> >>>>> To align with the semantics described in the SO_LINGER section of man
> >>>>> socket(7) and to mimic AF_INET's behaviour more closely, change the logic
> >>>>> of a lingering close(): instead of waiting for all data to be handled,
> >>>>> block until data is considered sent from the vsock's transport point of
> >>>>> view. That is until worker picks the packets for processing and decrements
> >>>>> virtio_vsock_sock::bytes_unsent down to 0.
> >>>>>
> >>>>> Note that such lingering is limited to transports that actually implement
> >>>>> vsock_transport::unsent_bytes() callback. This excludes Hyper-V and VMCI,
> >>>>> under which no lingering would be observed.
> >>>>>
> >>>>> The implementation does not adhere strictly to man page's interpretation of
> >>>>> SO_LINGER: shutdown() will not trigger the lingering. This follows AF_INET.
> >>>>>
> >>>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> >>>>> ---
> >>>>> net/vmw_vsock/virtio_transport_common.c | 13 +++++++++++--
> >>>>> 1 file changed, 11 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> >>>>> index 7f7de6d8809655fe522749fbbc9025df71f071bd..aeb7f3794f7cfc251dde878cb44fdcc54814c89c 100644
> >>>>> --- a/net/vmw_vsock/virtio_transport_common.c
> >>>>> +++ b/net/vmw_vsock/virtio_transport_common.c
> >>>>> @@ -1196,12 +1196,21 @@ static void virtio_transport_wait_close(struct sock *sk, long timeout)
> >>>>> {
> >>>>>   if (timeout) {
> >>>>>           DEFINE_WAIT_FUNC(wait, woken_wake_function);
> >>>>> +         ssize_t (*unsent)(struct vsock_sock *vsk);
> >>>>> +         struct vsock_sock *vsk = vsock_sk(sk);
> >>>>> +
> >>>>> +         /* Some transports (Hyper-V, VMCI) do not implement
> >>>>> +          * unsent_bytes. For those, no lingering on close().
> >>>>> +          */
> >>>>> +         unsent = vsk->transport->unsent_bytes;
> >>>>> +         if (!unsent)
> >>>>> +                 return;
> >>>>
> >>>> IIUC if `unsent_bytes` is not implemented, virtio_transport_wait_close
> >>>> basically does nothing. My concern is that we are breaking the
> >>>> userspace due to a change in the behavior: Before this patch, with a
> >>>> vmci/hyper-v transport, this function would wait for SOCK_DONE to be
> >>>> set, but not anymore.
> >>>
> >>> Wait, we are in virtio_transport_common.c, why we are talking about
> >>> Hyper-V and VMCI?
> >>>
> >>> I asked to check `vsk->transport->unsent_bytes` in the v1, because this
> >>> code was part of af_vsock.c, but now we are back to virtio code, so I'm
> >>> confused...
> >>
> >> Might your confusion be because of similar names?
> >
> > In v1 this code IIRC was in af_vsock.c, now you pushed back on virtio
> > common code, so I still don't understand how
> > virtio_transport_wait_close() can be called with vmci or hyper-v
> > transports.
> >
> > Can you provide an example?
>
> You're right, it was me who was confused. VMCI and Hyper-V have their own
> vsock_transport::release callbacks that do not call
> virtio_transport_wait_close().
>
> So VMCI and Hyper-V never lingered anyway?

I think so.

Indeed I was happy with v1, since I think this should be supported by
the vsock core and should not depend on the transport.
But we can do also later.

Stefano

>
> >> vsock_transport::unsent_bytes != virtio_vsock_sock::bytes_unsent
> >>
> >> I agree with Luigi, it is a breaking change for userspace depending on a
> >> non-standard behaviour. What's the protocol here; do it anyway, then see if
> >> anyone complains?
> >>
> >> As for Hyper-V and VMCI losing the "lingering", do we care? And if we do,
> >> take Hyper-V, is it possible to test any changes without access to
> >> proprietary host/hypervisor?
> >>
> >
> > Again, how this code can be called when using vmci or hyper-v
> > transports?
>
> It cannot, you're right.
>
> > If we go back on v1 implementation, I can understand it, but with this
> > version I really don't understand the scenario.
> >
> > Stefano
> >
>

Re: [PATCH net-next v2 1/3] vsock: Linger on unsent data

[Michal Luczaj]

On 4/24/25 10:36, Stefano Garzarella wrote:
> On Thu, 24 Apr 2025 at 09:53, Michal Luczaj <mhal@rbox.co> wrote:
>>
>> On 4/24/25 09:28, Stefano Garzarella wrote:
>>> On Wed, Apr 23, 2025 at 11:06:33PM +0200, Michal Luczaj wrote:
>>>> On 4/23/25 18:34, Stefano Garzarella wrote:
>>>>> On Wed, Apr 23, 2025 at 05:53:12PM +0200, Luigi Leonardi wrote:
>>>>>> Hi Michal,
>>>>>>
>>>>>> On Mon, Apr 21, 2025 at 11:50:41PM +0200, Michal Luczaj wrote:
>>>>>>> Currently vsock's lingering effectively boils down to waiting (or timing
>>>>>>> out) until packets are consumed or dropped by the peer; be it by receiving
>>>>>>> the data, closing or shutting down the connection.
>>>>>>>
>>>>>>> To align with the semantics described in the SO_LINGER section of man
>>>>>>> socket(7) and to mimic AF_INET's behaviour more closely, change the logic
>>>>>>> of a lingering close(): instead of waiting for all data to be handled,
>>>>>>> block until data is considered sent from the vsock's transport point of
>>>>>>> view. That is until worker picks the packets for processing and decrements
>>>>>>> virtio_vsock_sock::bytes_unsent down to 0.
>>>>>>>
>>>>>>> Note that such lingering is limited to transports that actually implement
>>>>>>> vsock_transport::unsent_bytes() callback. This excludes Hyper-V and VMCI,
>>>>>>> under which no lingering would be observed.
>>>>>>>
>>>>>>> The implementation does not adhere strictly to man page's interpretation of
>>>>>>> SO_LINGER: shutdown() will not trigger the lingering. This follows AF_INET.
>>>>>>>
>>>>>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>>>>>> ---
>>>>>>> net/vmw_vsock/virtio_transport_common.c | 13 +++++++++++--
>>>>>>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>>>>>> index 7f7de6d8809655fe522749fbbc9025df71f071bd..aeb7f3794f7cfc251dde878cb44fdcc54814c89c 100644
>>>>>>> --- a/net/vmw_vsock/virtio_transport_common.c
>>>>>>> +++ b/net/vmw_vsock/virtio_transport_common.c
>>>>>>> @@ -1196,12 +1196,21 @@ static void virtio_transport_wait_close(struct sock *sk, long timeout)
>>>>>>> {
>>>>>>>   if (timeout) {
>>>>>>>           DEFINE_WAIT_FUNC(wait, woken_wake_function);
>>>>>>> +         ssize_t (*unsent)(struct vsock_sock *vsk);
>>>>>>> +         struct vsock_sock *vsk = vsock_sk(sk);
>>>>>>> +
>>>>>>> +         /* Some transports (Hyper-V, VMCI) do not implement
>>>>>>> +          * unsent_bytes. For those, no lingering on close().
>>>>>>> +          */
>>>>>>> +         unsent = vsk->transport->unsent_bytes;
>>>>>>> +         if (!unsent)
>>>>>>> +                 return;
>>>>>>
>>>>>> IIUC if `unsent_bytes` is not implemented, virtio_transport_wait_close
>>>>>> basically does nothing. My concern is that we are breaking the
>>>>>> userspace due to a change in the behavior: Before this patch, with a
>>>>>> vmci/hyper-v transport, this function would wait for SOCK_DONE to be
>>>>>> set, but not anymore.
>>>>>
>>>>> Wait, we are in virtio_transport_common.c, why we are talking about
>>>>> Hyper-V and VMCI?
>>>>>
>>>>> I asked to check `vsk->transport->unsent_bytes` in the v1, because this
>>>>> code was part of af_vsock.c, but now we are back to virtio code, so I'm
>>>>> confused...
>>>>
>>>> Might your confusion be because of similar names?
>>>
>>> In v1 this code IIRC was in af_vsock.c, now you pushed back on virtio
>>> common code, so I still don't understand how
>>> virtio_transport_wait_close() can be called with vmci or hyper-v
>>> transports.
>>>
>>> Can you provide an example?
>>
>> You're right, it was me who was confused. VMCI and Hyper-V have their own
>> vsock_transport::release callbacks that do not call
>> virtio_transport_wait_close().
>>
>> So VMCI and Hyper-V never lingered anyway?
> 
> I think so.
> 
> Indeed I was happy with v1, since I think this should be supported by
> the vsock core and should not depend on the transport.
> But we can do also later.

OK, for now let me fix this nonsense in comment and commit message.

But I'll wait for your opinion on [1] (drop, squash, change order of
patches?) before posting v3.

[1]:
https://lore.kernel.org/netdev/20250421-vsock-linger-v2-2-fe9febd64668@rbox.co/

Thanks,
Michal

Re: [PATCH net-next v2 1/3] vsock: Linger on unsent data

[Stefano Garzarella]

On Thu, Apr 24, 2025 at 01:24:59PM +0200, Michal Luczaj wrote:
>On 4/24/25 10:36, Stefano Garzarella wrote:
>> On Thu, 24 Apr 2025 at 09:53, Michal Luczaj <mhal@rbox.co> wrote:
>>> On 4/24/25 09:28, Stefano Garzarella wrote:

[...]

>>> You're right, it was me who was confused. VMCI and Hyper-V have their own
>>> vsock_transport::release callbacks that do not call
>>> virtio_transport_wait_close().
>>>
>>> So VMCI and Hyper-V never lingered anyway?
>>
>> I think so.
>>
>> Indeed I was happy with v1, since I think this should be supported by
>> the vsock core and should not depend on the transport.
>> But we can do also later.
>
>OK, for now let me fix this nonsense in comment and commit message.

Thanks!

>
>But I'll wait for your opinion on [1] (drop, squash, change order of
>patches?) before posting v3.

I'm fine with a second patch to fix the indentation and the order looks 
fine.

BTW I'm thinking if it makes sense to go back on moving the lingering in 
the core. I mean, if `unsent_bytes` is implemented, support linger, if 
not, don't support it, like now.

That said, this should be implemented in another patch (or eventually 
another series if you prefer), so my idea is the following split:
- use unsent_bytes() just in virtio
- move linger support in af_vsock.c (depending on transports 
   implementing unsent_bytes())
- implement unsent_bytes() in other transports (in the future)

WDYT?

Thanks,
Stefano

Re: [PATCH net-next v2 1/3] vsock: Linger on unsent data

[Michal Luczaj]

On 4/28/25 15:56, Stefano Garzarella wrote:
> On Thu, Apr 24, 2025 at 01:24:59PM +0200, Michal Luczaj wrote:
>> On 4/24/25 10:36, Stefano Garzarella wrote:
>>> On Thu, 24 Apr 2025 at 09:53, Michal Luczaj <mhal@rbox.co> wrote:
>>>> On 4/24/25 09:28, Stefano Garzarella wrote:
> 
> [...]
> 
>>>> You're right, it was me who was confused. VMCI and Hyper-V have their own
>>>> vsock_transport::release callbacks that do not call
>>>> virtio_transport_wait_close().
>>>>
>>>> So VMCI and Hyper-V never lingered anyway?
>>>
>>> I think so.
>>>
>>> Indeed I was happy with v1, since I think this should be supported by
>>> the vsock core and should not depend on the transport.
>>> But we can do also later.
>>
>> OK, for now let me fix this nonsense in comment and commit message.
> 
> Thanks!
> 
>>
>> But I'll wait for your opinion on [1] (drop, squash, change order of
>> patches?) before posting v3.
> 
> I'm fine with a second patch to fix the indentation and the order looks 
> fine.
> 
> BTW I'm thinking if it makes sense to go back on moving the lingering in 
> the core. I mean, if `unsent_bytes` is implemented, support linger, if 
> not, don't support it, like now.
> 
> That said, this should be implemented in another patch (or eventually 
> another series if you prefer), so my idea is the following split:
> - use unsent_bytes() just in virtio
> - move linger support in af_vsock.c (depending on transports 
>    implementing unsent_bytes())
> - implement unsent_bytes() in other transports (in the future)
> 
> WDYT?

Sure, makes sense. Even though I'm not certain I understand "use
unsent_bytes() just in virtio" part. Anyway, we can carry the discussion to
v3:
https://lore.kernel.org/netdev/20250430-vsock-linger-v3-0-ddbe73b53457@rbox.co/

Note that I took the liberty to assume unsent_bytes() is always there for
loopback/virtio transports. Check for NULL is introduced when the code is
moved to core. By the end of the series it changes nothing, but I hope it's
a tiny bit more sensible.

Thanks,
Michal

[PATCH net-next v2 2/3] vsock: Reduce indentation in virtio_transport_wait_close()

[Michal Luczaj]

Flatten the function. Remove the nested block by inverting the condition:
return early on !timeout.

No functional change intended.

Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/virtio_transport_common.c | 36 ++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index aeb7f3794f7cfc251dde878cb44fdcc54814c89c..73b6e7b437d950fd1cd1507f7dcc28780bd98a0b 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -1194,28 +1194,28 @@ static void virtio_transport_remove_sock(struct vsock_sock *vsk)
 
 static void virtio_transport_wait_close(struct sock *sk, long timeout)
 {
-	if (timeout) {
-		DEFINE_WAIT_FUNC(wait, woken_wake_function);
-		ssize_t (*unsent)(struct vsock_sock *vsk);
-		struct vsock_sock *vsk = vsock_sk(sk);
+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
+	ssize_t (*unsent)(struct vsock_sock *vsk);
+	struct vsock_sock *vsk = vsock_sk(sk);
 
-		/* Some transports (Hyper-V, VMCI) do not implement
-		 * unsent_bytes. For those, no lingering on close().
-		 */
-		unsent = vsk->transport->unsent_bytes;
-		if (!unsent)
-			return;
+	if (!timeout)
+		return;
+
+	/* Some transports (Hyper-V, VMCI) do not implement unsent_bytes.
+	 * For those, no lingering on close().
+	 */
+	unsent = vsk->transport->unsent_bytes;
+	if (!unsent)
+		return;
 
-		add_wait_queue(sk_sleep(sk), &wait);
+	add_wait_queue(sk_sleep(sk), &wait);
 
-		do {
-			if (sk_wait_event(sk, &timeout, unsent(vsk) == 0,
-					  &wait))
-				break;
-		} while (!signal_pending(current) && timeout);
+	do {
+		if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
+			break;
+	} while (!signal_pending(current) && timeout);
 
-		remove_wait_queue(sk_sleep(sk), &wait);
-	}
+	remove_wait_queue(sk_sleep(sk), &wait);
 }
 
 static void virtio_transport_cancel_close_work(struct vsock_sock *vsk,

-- 
2.49.0

[PATCH net-next v2 3/3] vsock/test: Expand linger test to ensure close() does not misbehave

[Michal Luczaj]

There was an issue with SO_LINGER: instead of blocking until all queued
messages for the socket have been successfully sent (or the linger timeout
has been reached), close() would block until packets were handled by the
peer.

Add a check to alert on close() lingering when it should not.

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

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index d0f6d253ac72d08a957cb81a3c38fcc72bec5a53..82d0bc20dfa75041f04eada1b4310be2f7c3a0c1 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -1788,13 +1788,16 @@ static void test_stream_connect_retry_server(const struct test_opts *opts)
 	close(fd);
 }
 
+#define	LINGER_TIMEOUT	1	/* seconds */
+
 static void test_stream_linger_client(const struct test_opts *opts)
 {
 	struct linger optval = {
 		.l_onoff = 1,
-		.l_linger = 1
+		.l_linger = LINGER_TIMEOUT
 	};
-	int fd;
+	int bytes_unsent, fd;
+	time_t ts;
 
 	fd = vsock_stream_connect(opts->peer_cid, opts->peer_port);
 	if (fd < 0) {
@@ -1807,7 +1810,28 @@ static void test_stream_linger_client(const struct test_opts *opts)
 		exit(EXIT_FAILURE);
 	}
 
+	/* Byte left unread to expose any incorrect behaviour. */
+	send_byte(fd, 1, 0);
+
+	/* Reuse LINGER_TIMEOUT to wait for bytes_unsent == 0. */
+	timeout_begin(LINGER_TIMEOUT);
+	do {
+		if (ioctl(fd, SIOCOUTQ, &bytes_unsent) < 0) {
+			perror("ioctl(SIOCOUTQ)");
+			exit(EXIT_FAILURE);
+		}
+		timeout_check("ioctl(SIOCOUTQ) == 0");
+	} while (bytes_unsent != 0);
+	timeout_end();
+
+	ts = current_nsec();
 	close(fd);
+	if ((current_nsec() - ts) / NSEC_PER_SEC > 0) {
+		fprintf(stderr, "Unexpected lingering on close()\n");
+		exit(EXIT_FAILURE);
+	}
+
+	control_writeln("DONE");
 }
 
 static void test_stream_linger_server(const struct test_opts *opts)
@@ -1820,7 +1844,7 @@ static void test_stream_linger_server(const struct test_opts *opts)
 		exit(EXIT_FAILURE);
 	}
 
-	vsock_wait_remote_close(fd);
+	control_expectln("DONE");
 	close(fd);
 }
 

-- 
2.49.0