netdev - Re: [PATCH v1 1/2] io_uring/zcrx: add single shot recvzc

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: <270ce534-d33e-4642-b0dc-87e377025825@gmail.com>
Date: Thu, 20 Feb 2025 13:45:37 +0000
From: Pavel Begunkov <asml.silence@...il.com>
To: David Wei <dw@...idwei.uk>, io-uring@...r.kernel.org,
 netdev@...r.kernel.org
Cc: Jens Axboe <axboe@...nel.dk>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Simon Horman <horms@...nel.org>
Subject: Re: [PATCH v1 1/2] io_uring/zcrx: add single shot recvzc

On 2/18/25 16:57, David Wei wrote:
> Currently only multishot recvzc requests are supported, but sometimes
> there is a need to do a single recv e.g. peeking at some data in the
> socket. Add single shot recvzc requests where IORING_RECV_MULTISHOT is
> _not_ set and the sqe->len field is set to the number of bytes to read
> N.
> 
> There could be multiple completions containing data, like the multishot
> case, since N bytes could be split across multiple frags. This is
> followed by a final completion with res and cflags both set to 0 that
> indicate the completion of the request, or a -res that indicate an
> error.
> 
> Signed-off-by: David Wei <dw@...idwei.uk>
> ---
>   io_uring/net.c  | 26 ++++++++++++++++++--------
>   io_uring/zcrx.c | 17 ++++++++++++++---
>   io_uring/zcrx.h |  2 +-
>   3 files changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/io_uring/net.c b/io_uring/net.c
> index 000dc70d08d0..d3a9aaa52a13 100644
> --- a/io_uring/net.c
> +++ b/io_uring/net.c
> @@ -94,6 +94,7 @@ struct io_recvzc {
>   	struct file			*file;
>   	unsigned			msg_flags;
>   	u16				flags;
> +	u32				len;

Something is up with the types, it's u32, for which you use
UINT_MAX, and later convert to ulong.

>   	struct io_zcrx_ifq		*ifq;
>   };
>   
...
> @@ -1250,6 +1251,9 @@ int io_recvzc_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>   	zc->ifq = req->ctx->ifq;
>   	if (!zc->ifq)
>   		return -EINVAL;
> +	zc->len = READ_ONCE(sqe->len);
> +	if (zc->len == UINT_MAX)
> +		return -EINVAL;
>   
>   	zc->flags = READ_ONCE(sqe->ioprio);
>   	zc->msg_flags = READ_ONCE(sqe->msg_flags);
> @@ -1257,12 +1261,14 @@ int io_recvzc_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>   		return -EINVAL;
>   	if (zc->flags & ~(IORING_RECVSEND_POLL_FIRST | IORING_RECV_MULTISHOT))
>   		return -EINVAL;
> -	/* multishot required */
> -	if (!(zc->flags & IORING_RECV_MULTISHOT))
> -		return -EINVAL;
> -	/* All data completions are posted as aux CQEs. */
> -	req->flags |= REQ_F_APOLL_MULTISHOT;
> -
> +	if (zc->flags & IORING_RECV_MULTISHOT) {
> +		if (zc->len)
> +			return -EINVAL;
> +		/* All data completions are posted as aux CQEs. */
> +		req->flags |= REQ_F_APOLL_MULTISHOT;

If you're posting "aux" cqes you have to set the flag for
synchronisation reasons. We probably can split out a "I want to post
aux cqes" flag, but it seems like you don't actually care about
multishot here but limiting the length, or limiting the length + nowait.

> +	}
> +	if (!zc->len)
> +		zc->len = UINT_MAX;
>   	return 0;
>   }
>   
> @@ -1281,7 +1287,7 @@ int io_recvzc(struct io_kiocb *req, unsigned int issue_flags)
>   		return -ENOTSOCK;
>   
>   	ret = io_zcrx_recv(req, zc->ifq, sock, zc->msg_flags | MSG_DONTWAIT,
> -			   issue_flags);
> +			   issue_flags, zc->len);
>   	if (unlikely(ret <= 0) && ret != -EAGAIN) {
>   		if (ret == -ERESTARTSYS)
>   			ret = -EINTR;
> @@ -1296,6 +1302,10 @@ int io_recvzc(struct io_kiocb *req, unsigned int issue_flags)
>   		return IOU_OK;
>   	}
>   
> +	if (zc->len != UINT_MAX) {
> +		io_req_set_res(req, ret, 0);
> +		return IOU_OK;
> +	}
>   	if (issue_flags & IO_URING_F_MULTISHOT)
>   		return IOU_ISSUE_SKIP_COMPLETE;
>   	return -EAGAIN;
> diff --git a/io_uring/zcrx.c b/io_uring/zcrx.c
> index ea099f746599..834c887743c8 100644
> --- a/io_uring/zcrx.c
> +++ b/io_uring/zcrx.c
> @@ -106,6 +106,7 @@ struct io_zcrx_args {
>   	struct io_zcrx_ifq	*ifq;
>   	struct socket		*sock;
>   	unsigned		nr_skbs;
> +	unsigned long		len;
>   };
>   
>   static const struct memory_provider_ops io_uring_pp_zc_ops;
> @@ -826,6 +827,10 @@ io_zcrx_recv_skb(read_descriptor_t *desc, struct sk_buff *skb,
>   	int i, copy, end, off;
>   	int ret = 0;
>   
> +	if (args->len == 0)
> +		return -EINTR;
> +	len = (args->len != UINT_MAX) ? min_t(size_t, len, args->len) : len;

Just min?

> +
>   	if (unlikely(args->nr_skbs++ > IO_SKBS_PER_CALL_LIMIT))
>   		return -EAGAIN;
>   
> @@ -920,17 +925,21 @@ io_zcrx_recv_skb(read_descriptor_t *desc, struct sk_buff *skb,
>   out:
>   	if (offset == start_off)
>   		return ret;
> +	args->len -= (offset - start_off);

Doesn't it unconditionally change the magic value UINT_MAX
you're trying to preserve?

> +	if (args->len == 0)
> +		desc->count = 0;
>   	return offset - start_off;
>   }
>   
>   static int io_zcrx_tcp_recvmsg(struct io_kiocb *req, struct io_zcrx_ifq *ifq,
>   				struct sock *sk, int flags,
> -				unsigned issue_flags)
> +				unsigned issue_flags, unsigned long len)
>   {
>   	struct io_zcrx_args args = {
>   		.req = req,
>   		.ifq = ifq,
>   		.sock = sk->sk_socket,
> +		.len = len,
>   	};
>   	read_descriptor_t rd_desc = {
>   		.count = 1,
> @@ -956,6 +965,8 @@ static int io_zcrx_tcp_recvmsg(struct io_kiocb *req, struct io_zcrx_ifq *ifq,
>   		ret = IOU_REQUEUE;
>   	} else if (sock_flag(sk, SOCK_DONE)) {
>   		/* Make it to retry until it finally gets 0. */
> +		if (len != UINT_MAX)
> +			goto out;
>   		if (issue_flags & IO_URING_F_MULTISHOT)
>   			ret = IOU_REQUEUE;
>   		else

-- 
Pavel Begunkov