linux-kernel - Re: [RFC PATCH v2 01/13] af_vsock: implement 'vsock_wait

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]

Date:   Mon, 18 Jan 2021 15:51:58 +0100
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>,
        Colin Ian King <colin.king@...onical.com>,
        Andra Paraschiv <andraprs@...zon.com>,
        Jeff Vander Stoep <jeffv@...gle.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 v2 01/13] af_vsock: implement 'vsock_wait_data()'.

On Fri, Jan 15, 2021 at 08:40:25AM +0300, Arseny Krasnov wrote:
>This adds 'vsock_wait_data()' function which is called from user's read
>syscall and waits until new socket data is arrived. It was based on code
>from stream dequeue logic and moved to separate function because it will
>be called both from SOCK_STREAM and SOCK_SEQPACKET receive loops.
>
>Signed-off-by: Arseny Krasnov <arseny.krasnov@...persky.com>
>---
> net/vmw_vsock/af_vsock.c | 47 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index b12d3a322242..af716f5a93a4 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -1822,6 +1822,53 @@ static int vsock_stream_sendmsg(struct socket *sock, struct msghdr *msg,
> 	return err;
> }
>
>+static int vsock_wait_data(struct sock *sk, struct wait_queue_entry *wait,
>+			   long timeout,
>+			   struct vsock_transport_recv_notify_data *recv_data,
>+			   size_t target)
>+{
>+	int err = 0;
>+	struct vsock_sock *vsk;
>+	const struct vsock_transport *transport;

Please be sure that here and in all of the next patches, you follow the 
"Reverse Christmas tree" rule followed in net/ for the local variable 
declarations (order variable declaration lines longest to shortest).

>+
>+	vsk = vsock_sk(sk);
>+	transport = vsk->transport;
>+
>+	if (sk->sk_err != 0 ||
>+	    (sk->sk_shutdown & RCV_SHUTDOWN) ||
>+	    (vsk->peer_shutdown & SEND_SHUTDOWN)) {
>+		finish_wait(sk_sleep(sk), wait);
>+		return -1;
>+	}
>+	/* Don't wait for non-blocking sockets. */
>+	if (timeout == 0) {
>+		err = -EAGAIN;
>+		finish_wait(sk_sleep(sk), wait);
>+		return err;
>+	}
>+
>+	if (recv_data) {
>+		err = transport->notify_recv_pre_block(vsk, target, recv_data);
>+		if (err < 0) {
>+			finish_wait(sk_sleep(sk), wait);
>+			return err;
>+		}
>+	}
>+
>+	release_sock(sk);
>+	timeout = schedule_timeout(timeout);
>+	lock_sock(sk);
>+
>+	if (signal_pending(current)) {
>+		err = sock_intr_errno(timeout);
>+		finish_wait(sk_sleep(sk), wait);
>+	} else if (timeout == 0) {
>+		err = -EAGAIN;
>+		finish_wait(sk_sleep(sk), wait);
>+	}
>+

Since we are calling finish_wait() before return in all path, why not 
doing somethig like this:

out:
	finish_wait(sk_sleep(sk), wait);
>+	return err;
>+}

Then in the error paths you can do:

	err = XXX;
	goto out;

Thanks,
Stefano

>
> static int
> vsock_stream_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>-- 
>2.25.1
>