Message-ID: <6d80d6a3-749a-baa3-8f27-5e6a696d0499@kernel.dk>
Date:   Wed, 13 Jul 2022 06:48:53 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     Dylan Yudaken <dylany@...com>,
        Pavel Begunkov <asml.silence@...il.com>, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
        io-uring@...r.kernel.org
Cc:     netdev@...r.kernel.org, Kernel-team@...com
Subject: Re: [PATCH v2 for-next 3/3] io_uring: support multishot in recvmsg

On 7/13/22 2:23 AM, Dylan Yudaken wrote:
> diff --git a/io_uring/net.c b/io_uring/net.c
> index 5bc3440a8290..56f734acced6 100644
> --- a/io_uring/net.c
> +++ b/io_uring/net.c
> @@ -483,12 +491,15 @@ static inline void io_recv_prep_retry(struct io_kiocb *req)
>  }
>  
>  /*
> - * Finishes io_recv
> + * Finishes io_recv and io_recvmsg.
>   *
>   * Returns true if it is actually finished, or false if it should run
>   * again (for multishot).
>   */
> -static inline bool io_recv_finish(struct io_kiocb *req, int *ret, unsigned int cflags)
> +static inline bool io_recv_finish(struct io_kiocb *req,
> +				  int *ret,
> +				  unsigned int cflags,
> +				  bool multishot_finished)
>  {
>  	if (!(req->flags & REQ_F_APOLL_MULTISHOT)) {

Minor nit, but this should look like:

static inline bool io_recv_finish(struct io_kiocb *req, int *ret,
				  unsigned int cflags, bool mshot_finished)
> @@ -518,6 +529,104 @@ static inline bool io_recv_finish(struct io_kiocb *req, int *ret, unsigned int c
>  	return true;
>  }
>  
> +static int io_recvmsg_prep_multishot(
> +	struct io_async_msghdr *kmsg,
> +	struct io_sr_msg *sr,
> +	void __user **buf,
> +	size_t *len)
> +{

Ditto on the function formating.

> +	unsigned long used = 0;
> +
> +	if (*len < sizeof(struct io_uring_recvmsg_out))
> +		return -EFAULT;
> +	used += sizeof(struct io_uring_recvmsg_out);
> +
> +	if (kmsg->namelen) {
> +		if (kmsg->namelen + used > *len)
> +			return -EFAULT;
> +		used += kmsg->namelen;
> +	}
> +	if (kmsg->controllen) {
> +		if (kmsg->controllen + used > *len)
> +			return -EFAULT;
> +		kmsg->msg.msg_control_user = (void *)((unsigned long)*buf + used);
> +		kmsg->msg.msg_controllen = kmsg->controllen;
> +		used += kmsg->controllen;
> +	}
> +	if (used >= UINT_MAX)
> +		return -EOVERFLOW;
> +
> +	sr->buf = *buf; /* stash for later copy */
> +	*buf = (void *)((unsigned long)*buf + used);
> +	*len -= used;
> +	kmsg->payloadlen = *len;
> +	return 0;
> +}

Not sure if it's just me, but the *buf and casting is really hard to
read here. Can we make that any clearer? Maybe cast to an unsigned long
* at the top of change the buf argument to be that?

> +struct io_recvmsg_multishot_hdr {
> +	struct io_uring_recvmsg_out msg;
> +	struct sockaddr_storage addr;
> +} __packed;

This __packed shouldn't be necessary, and I'm always a bit wary of
adding it on kernel structures as if it's really needed, then we're most
likely doing something wrong (and things will run slower, notably on
some archs). Looks like you have a BUILD_BUG_ON() for this too, so we'd
catch any potential issues here upfront.

> +static int io_recvmsg_multishot(
> +	struct socket *sock,
> +	struct io_sr_msg *io,
> +	struct io_async_msghdr *kmsg,
> +	unsigned int flags,
> +	bool *finished)
> +{
> +	int err;
> +	int copy_len;
> +	struct io_recvmsg_multishot_hdr hdr;
> +
> +	if (kmsg->namelen)
> +		kmsg->msg.msg_name = &hdr.addr;
> +	kmsg->msg.msg_flags = flags & (MSG_CMSG_CLOEXEC|MSG_CMSG_COMPAT);
> +	kmsg->msg.msg_namelen = 0;
> +
> +	if (sock->file->f_flags & O_NONBLOCK)
> +		flags |= MSG_DONTWAIT;
> +
> +	err = sock_recvmsg(sock, &kmsg->msg, flags);
> +	*finished = err <= 0;
> +	if (err < 0)
> +		return err;
> +
> +	hdr.msg = (struct io_uring_recvmsg_out) {
> +		.controllen = kmsg->controllen - kmsg->msg.msg_controllen,
> +		.flags = kmsg->msg.msg_flags & ~MSG_CMSG_COMPAT
> +	};
> +
> +	hdr.msg.payloadlen = err;
> +	if (err > kmsg->payloadlen)
> +		err = kmsg->payloadlen;
> +
> +	copy_len = sizeof(struct io_uring_recvmsg_out);
> +	if (kmsg->msg.msg_namelen > kmsg->namelen)
> +		copy_len += kmsg->namelen;
> +	else
> +		copy_len += kmsg->msg.msg_namelen;
> +
> +	/*
> +	 *      "fromlen shall refer to the value before truncation.."
> +	 *                      1003.1g
> +	 */
> +	hdr.msg.namelen = kmsg->msg.msg_namelen;
> +
> +	/* ensure that there is no gap between hdr and sockaddr_storage */
> +	BUILD_BUG_ON(offsetof(struct io_recvmsg_multishot_hdr, addr) !=
> +		     sizeof(struct io_uring_recvmsg_out));
> +	if (copy_to_user(io->buf, &hdr, copy_len)) {
> +		*finished = true;
> +		return -EFAULT;
> +	}
> +
> +	return sizeof(struct io_uring_recvmsg_out) +
> +		kmsg->namelen +
> +		kmsg->controllen +
> +		err;
> +}

	return sizeof(struct io_uring_recvmsg_out) + kmsg->namelen +
			kmsg->controllen + err;

would be closer to the kernel style.

In general I'm not a big fan of the bool pointer 'finished'. But I also
don't have a good suggestion on how to make it cleaner, so... Would be
nice if we could just have an error return (< 0), and then return >= 0
in two variants for MSHOT_OK and MSHOT_TERMINATE or something.

> @@ -527,6 +636,7 @@ int io_recvmsg(struct io_kiocb *req, unsigned int issue_flags)
>  	unsigned flags;
>  	int ret, min_ret = 0;
>  	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
> +	bool multishot_finished = true;
>  
>  	sock = sock_from_file(req->file);
>  	if (unlikely(!sock))
> @@ -545,16 +655,29 @@ int io_recvmsg(struct io_kiocb *req, unsigned int issue_flags)
>  	    (sr->flags & IORING_RECVSEND_POLL_FIRST))
>  		return io_setup_async_msg(req, kmsg, issue_flags);
>  
> +retry_multishot:
>  	if (io_do_buffer_select(req)) {
>  		void __user *buf;
> +		size_t len = sr->len;
>  
> -		buf = io_buffer_select(req, &sr->len, issue_flags);
> +		buf = io_buffer_select(req, &len, issue_flags);
>  		if (!buf)
>  			return -ENOBUFS;
> +
> +		if (req->flags & REQ_F_APOLL_MULTISHOT) {
> +			ret = io_recvmsg_prep_multishot(kmsg, sr,
> +							&buf, &len);
> +
			ret = io_recvmsg_prep_multishot(kmsg, sr, &buf, &len);

Apart from these nits, looks pretty good to me.

-- 
Jens Axboe

Powered by blists - more mailing lists