Re: [PATCH v3 for-next 3/3] io_uring: support multishot in recvmsg

[Jens Axboe <axboe@kernel.dk>]

On 7/14/22 5:02 AM, Dylan Yudaken wrote:
> Similar to multishot recv, this will require provided buffers to be
> used. However recvmsg is much more complex than recv as it has multiple
> outputs. Specifically flags, name, and control messages.
> 
> Support this by introducing a new struct io_uring_recvmsg_out with 4
> fields. namelen, controllen and flags match the similar out fields in
> msghdr from standard recvmsg(2), payloadlen is the length of the payload
> following the header.
> This struct is placed at the start of the returned buffer. Based on what
> the user specifies in struct msghdr, the next bytes of the buffer will be
> name (the next msg_namelen bytes), and then control (the next
> msg_controllen bytes). The payload will come at the end. The return value
> in the CQE is the total used size of the provided buffer.

Just a few minor nits (some repeat ones too), otherwise looks fine to
me. I can either fold these in while applying, or you can spin a v4. Let
me know!

> +static int io_recvmsg_multishot_overflow(struct io_async_msghdr *iomsg)
> +{
> +	unsigned long hdr;
> +
> +	if (check_add_overflow(sizeof(struct io_uring_recvmsg_out),
> +			       (unsigned long)iomsg->namelen, &hdr))
> +		return -EOVERFLOW;
> +	if (check_add_overflow(hdr, iomsg->controllen, &hdr))
> +		return -EOVERFLOW;
> +	if (hdr > INT_MAX)
> +		return -EOVERFLOW;
> +
> +	return 0;
> +}

Nobody checks the specific value of this helper, so we should either
actually do that, or just make this one return a true/false instead. The
latter makes the most sense to me.

> +static int io_recvmsg_prep_multishot(struct io_async_msghdr *kmsg, struct io_sr_msg *sr,
> +				     void __user **buf, size_t *len)
> +{

The line breaks here are odd, should be at 80 unless there's a good
reason for it to exceed it.

Function reads better now though with the cast.

> +static int io_recvmsg_multishot(
> +	struct socket *sock,
> +	struct io_sr_msg *io,
> +	struct io_async_msghdr *kmsg,
> +	unsigned int flags,
> +	bool *finished)
> +{

This is still formatted badly.

> +		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);

this still would fit nicely on an unbroken line.

-- 
Jens Axboe