netdev - Re: [PATCH v1 12/15] io_uring/zcrx: add io

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: <703c9d90-bca1-4ee7-b1f3-0cfeaf38ef8f@kernel.dk>
Date: Wed, 9 Oct 2024 12:28:08 -0600
From: Jens Axboe <axboe@...nel.dk>
To: David Wei <dw@...idwei.uk>, io-uring@...r.kernel.org,
 netdev@...r.kernel.org
Cc: Pavel Begunkov <asml.silence@...il.com>, Jakub Kicinski
 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jesper Dangaard Brouer <hawk@...nel.org>, David Ahern <dsahern@...nel.org>,
 Mina Almasry <almasrymina@...gle.com>
Subject: Re: [PATCH v1 12/15] io_uring/zcrx: add io_recvzc request

> diff --git a/io_uring/net.c b/io_uring/net.c
> index d08abcca89cc..482e138d2994 100644
> --- a/io_uring/net.c
> +++ b/io_uring/net.c
> @@ -1193,6 +1201,76 @@ int io_recv(struct io_kiocb *req, unsigned int issue_flags)
>  	return ret;
>  }
>  
> +int io_recvzc_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
> +{
> +	struct io_recvzc *zc = io_kiocb_to_cmd(req, struct io_recvzc);
> +	unsigned ifq_idx;
> +
> +	if (unlikely(sqe->file_index || sqe->addr2 || sqe->addr ||
> +		     sqe->len || sqe->addr3))
> +		return -EINVAL;
> +
> +	ifq_idx = READ_ONCE(sqe->zcrx_ifq_idx);
> +	if (ifq_idx != 0)
> +		return -EINVAL;
> +	zc->ifq = req->ctx->ifq;
> +	if (!zc->ifq)
> +		return -EINVAL;

This is read and assigned to 'zc' here, but then the issue handler does
it again? I'm assuming that at some point we'll have ifq selection here,
and then the issue handler will just use zc->ifq. So this part should
probably remain, and the issue side just use zc->ifq?

> +	/* All data completions are posted as aux CQEs. */
> +	req->flags |= REQ_F_APOLL_MULTISHOT;

This puzzles me a bit...

> +	zc->flags = READ_ONCE(sqe->ioprio);
> +	zc->msg_flags = READ_ONCE(sqe->msg_flags);
> +	if (zc->msg_flags)
> +		return -EINVAL;

Maybe allow MSG_DONTWAIT at least? You already pass that in anyway.

> +	if (zc->flags & ~(IORING_RECVSEND_POLL_FIRST | IORING_RECV_MULTISHOT))
> +		return -EINVAL;
> +
> +
> +#ifdef CONFIG_COMPAT
> +	if (req->ctx->compat)
> +		zc->msg_flags |= MSG_CMSG_COMPAT;
> +#endif
> +	return 0;
> +}

Heh, we could probably just return -EINVAL for that case, but since this
is all we need, fine.

> +
> +int io_recvzc(struct io_kiocb *req, unsigned int issue_flags)
> +{
> +	struct io_recvzc *zc = io_kiocb_to_cmd(req, struct io_recvzc);
> +	struct io_zcrx_ifq *ifq;
> +	struct socket *sock;
> +	int ret;
> +
> +	if (!(req->flags & REQ_F_POLLED) &&
> +	    (zc->flags & IORING_RECVSEND_POLL_FIRST))
> +		return -EAGAIN;
> +
> +	sock = sock_from_file(req->file);
> +	if (unlikely(!sock))
> +		return -ENOTSOCK;
> +	ifq = req->ctx->ifq;
> +	if (!ifq)
> +		return -EINVAL;

	irq = zc->ifq;

and then that check can go away too, as it should already have been
errored at prep time if this wasn't valid.

> +static bool io_zcrx_queue_cqe(struct io_kiocb *req, struct net_iov *niov,
> +			      struct io_zcrx_ifq *ifq, int off, int len)
> +{
> +	struct io_uring_zcrx_cqe *rcqe;
> +	struct io_zcrx_area *area;
> +	struct io_uring_cqe *cqe;
> +	u64 offset;
> +
> +	if (!io_defer_get_uncommited_cqe(req->ctx, &cqe))
> +		return false;
> +
> +	cqe->user_data = req->cqe.user_data;
> +	cqe->res = len;
> +	cqe->flags = IORING_CQE_F_MORE;
> +
> +	area = io_zcrx_iov_to_area(niov);
> +	offset = off + (net_iov_idx(niov) << PAGE_SHIFT);
> +	rcqe = (struct io_uring_zcrx_cqe *)(cqe + 1);
> +	rcqe->off = offset + ((u64)area->area_id << IORING_ZCRX_AREA_SHIFT);
> +	memset(&rcqe->__pad, 0, sizeof(rcqe->__pad));

Just do

	rcqe->__pad = 0;

since it's a single field.

Rest looks fine to me.

-- 
Jens Axboe