linux-kernel - Re: [RFC PATCH v9 19/19] af_vsock: serialize writes to shared socket

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210513140150.ugw6foy742fxan4w@steredhat>
Date:   Thu, 13 May 2021 16:01:50 +0200
From:   Stefano Garzarella <sgarzare@...hat.com>
To:     Arseny Krasnov <arseny.krasnov@...persky.com>
Cc:     Stefan Hajnoczi <stefanha@...hat.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Jason Wang <jasowang@...hat.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jorgen Hansen <jhansen@...are.com>,
        Norbert Slusarek <nslusarek@....net>,
        Andra Paraschiv <andraprs@...zon.com>,
        Colin Ian King <colin.king@...onical.com>,
        kvm@...r.kernel.org, virtualization@...ts.linux-foundation.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        stsp2@...dex.ru, oxffffaa@...il.com
Subject: Re: [RFC PATCH v9 19/19] af_vsock: serialize writes to shared socket

On Sat, May 08, 2021 at 07:37:35PM +0300, Arseny Krasnov wrote:
>This add logic, that serializes write access to single socket
>by multiple threads. It is implemented be adding field with TID
>of current writer. When writer tries to send something, it checks
>that field is -1(free), else it sleep in the same way as waiting
>for free space at peers' side.
>
>Signed-off-by: Arseny Krasnov <arseny.krasnov@...persky.com>
>---
> include/net/af_vsock.h   |  1 +
> net/vmw_vsock/af_vsock.c | 10 +++++++++-
> 2 files changed, 10 insertions(+), 1 deletion(-)

I think you forgot to move this patch at the beginning of the series.
It's important because in this way we can backport to stable branches 
easily.

About the implementation, can't we just add a mutex that we hold until 
we have sent all the payload?

I need to check other implementations like TCP.

>
>diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>index 1747c0b564ef..413343f18e99 100644
>--- a/include/net/af_vsock.h
>+++ b/include/net/af_vsock.h
>@@ -69,6 +69,7 @@ struct vsock_sock {
> 	u64 buffer_size;
> 	u64 buffer_min_size;
> 	u64 buffer_max_size;
>+	pid_t tid_owner;
>
> 	/* Private to transport. */
> 	void *trans;
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index 7790728465f4..1fb4a1860f6d 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -757,6 +757,7 @@ static struct sock *__vsock_create(struct net *net,
> 	vsk->peer_shutdown = 0;
> 	INIT_DELAYED_WORK(&vsk->connect_work, vsock_connect_timeout);
> 	INIT_DELAYED_WORK(&vsk->pending_work, vsock_pending_work);
>+	vsk->tid_owner = -1;
>
> 	psk = parent ? vsock_sk(parent) : NULL;
> 	if (parent) {
>@@ -1765,7 +1766,9 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg,
> 		ssize_t written;
>
> 		add_wait_queue(sk_sleep(sk), &wait);
>-		while (vsock_stream_has_space(vsk) == 0 &&
>+		while ((vsock_stream_has_space(vsk) == 0 ||
>+			(vsk->tid_owner != current->pid &&
>+			 vsk->tid_owner != -1)) &&
> 		       sk->sk_err == 0 &&
> 		       !(sk->sk_shutdown & SEND_SHUTDOWN) &&
> 		       !(vsk->peer_shutdown & RCV_SHUTDOWN)) {
>@@ -1796,6 +1799,8 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg,
> 				goto out_err;
> 			}
> 		}
>+
>+		vsk->tid_owner = current->pid;
> 		remove_wait_queue(sk_sleep(sk), &wait);
>
> 		/* These checks occur both as part of and after the loop
>@@ -1852,7 +1857,10 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg,
> 			err = total_written;
> 	}
> out:
>+	vsk->tid_owner = -1;
> 	release_sock(sk);
>+	sk->sk_write_space(sk);
>+

Is this change related? Can you explain in the commit message why it is 
needed?

> 	return err;
> }
>
>-- 
>2.25.1
>