linux-kernel - Re: [PATCH net-next 1/2] vsock: Linger on unsent data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <d16f61d2-df65-41b9-a042-f54466b7e764@rbox.co>
Date: Mon, 21 Apr 2025 23:52:52 +0200
From: Michal Luczaj <mhal@...x.co>
To: Stefano Garzarella <sgarzare@...hat.com>
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
 "Michael S. Tsirkin" <mst@...hat.com>, Jason Wang <jasowang@...hat.com>,
 Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, Eugenio Pérez
 <eperezma@...hat.com>, Stefan Hajnoczi <stefanha@...hat.com>,
 virtualization@...ts.linux.dev, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH net-next 1/2] vsock: Linger on unsent data

On 4/11/25 15:43, Stefano Garzarella wrote:
> On Mon, Apr 07, 2025 at 08:41:43PM +0200, Michal Luczaj wrote:
>> Change the behaviour of a lingering close(): instead of waiting for all
>> data to be consumed, block until data is considered sent, i.e. until worker
>> picks the packets and decrements virtio_vsock_sock::bytes_unsent down to 0.
>>
>> Do linger on shutdown() just as well.
> 
> I think this should go in a separate patch.

As discussed, dropping linger on shutdown() for now.

>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index fc6afbc8d6806a4d98c66abc3af4bd139c583b08..383c6644d047589035c0439c47d1440273e67ea9 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -1013,6 +1013,29 @@ static int vsock_getname(struct socket *sock,
>> 	return err;
>> }
>>
>> +void vsock_linger(struct sock *sk, long timeout)
>> +{
>> +	if (timeout) {
> 
> I would prefer to avoid a whole nested block and return immediately
> in such a case. (It's pre-existing, but since we are moving this code
> I'd fix it).
> 
> 	if (!timeout)
> 		return;

In v2 no code is moved (since no shutdown() lingering), so I'll reduce the
indentation in a separate patch. Let me know if it's not worth the churn
anymore.

>> +		DEFINE_WAIT_FUNC(wait, woken_wake_function);
>> +		ssize_t (*unsent)(struct vsock_sock *vsk);
>> +		struct vsock_sock *vsk = vsock_sk(sk);
>> +
> 
> transport->unsent_bytes can be NULL, this will panic with hyperv or
> vmci transport, especially because we now call this function in
> vsock_shutdown().
> 
> I'd skip that call if transports don't implement it, but please add
> a comment on top of this function about that.

I'm not sure I understand. I am calling it only conditionally, see below.
Nevertheless, I'll add a comment.

>> +		unsent = vsk->transport->unsent_bytes;
>> +		if (!unsent)
>> +			return;
>> +
>> +		add_wait_queue(sk_sleep(sk), &wait);
>> +
>> +		do {
>> +			if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
>> +				break;
>> +		} while (!signal_pending(current) && timeout);
>> +
>> +		remove_wait_queue(sk_sleep(sk), &wait);
>> +	}
>> +}

...

>> -	if (sock_flag(sk, SOCK_DONE)) {
>> +	if (sock_flag(sk, SOCK_DONE))
>> 		return true;
>> -	}
> 
> Please avoid this unrelated changes, if you really want to fix them,
> add another patch in the series where to fix them.
> 
>>
>> 	sock_hold(sk);
>> -	INIT_DELAYED_WORK(&vsk->close_work,
>> -			  virtio_transport_close_timeout);
>> +	INIT_DELAYED_WORK(&vsk->close_work, virtio_transport_close_timeout);
> 
> Ditto.
> 
> These 2 could go together in a single `cleanup` patch, although I
> usually avoid it so that `git blame` makes sense. But if we want to
> make checkpatch happy, that's fine.

All right, dropping these. I've thought, since I was touching this function
and this wasn't a bug fix (and wouldn't be backported), it'd be okay to do
some trivial cleanups along the way.

Here's v2:
https://lore.kernel.org/netdev/20250421-vsock-linger-v2-0-fe9febd64668@rbox.co/

Thanks,
Michal