| 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
| ||
|
Message-ID: <597c1bfc-f8ab-d513-4916-dbd93b05e66a@gmail.com>
Date: Thu, 6 Jan 2022 23:01:59 +0530
From: Praveen Kumar <kpraveen.lkml@...il.com>
To: Ammar Faizi <ammarfaizi2@...il.com>, Jens Axboe <axboe@...nel.dk>
Cc: io-uring Mailing List <io-uring@...r.kernel.org>,
Pavel Begunkov <asml.silence@...il.com>,
"David S . Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, Nugra <richiisei@...il.com>
Subject: Re: [RFC PATCH v3 3/3] io_uring: Add `sendto(2)` and `recvfrom(2)`
support
On 30-12-2021 23:22, Ammar Faizi wrote:
> This adds sendto(2) and recvfrom(2) support for io_uring.
>
> New opcodes:
> IORING_OP_SENDTO
> IORING_OP_RECVFROM
>
> Cc: Nugra <richiisei@...il.com>
> Tested-by: Nugra <richiisei@...il.com>
> Link: https://github.com/axboe/liburing/issues/397
> Signed-off-by: Ammar Faizi <ammarfaizi2@...il.com>
> ---
>
> v3:
> - Fix build error when CONFIG_NET is undefined should be done in
> the first patch, not this patch.
>
> - Add Tested-by tag from Nugra.
>
> v2:
> - In `io_recvfrom()`, mark the error check of `move_addr_to_user()`
> call as unlikely.
>
> - Fix build error when CONFIG_NET is undefined.
>
> - Added Nugra to CC list (tester).
> ---
> fs/io_uring.c | 80 +++++++++++++++++++++++++++++++++--
> include/uapi/linux/io_uring.h | 2 +
> 2 files changed, 78 insertions(+), 4 deletions(-)
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 7adcb591398f..3726958f8f58 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -575,7 +575,15 @@ struct io_sr_msg {
> union {
> struct compat_msghdr __user *umsg_compat;
> struct user_msghdr __user *umsg;
> - void __user *buf;
> +
> + struct {
> + void __user *buf;
> + struct sockaddr __user *addr;
> + union {
> + int sendto_addr_len;
> + int __user *recvfrom_addr_len;
> + };
> + };
> };
> int msg_flags;
> int bgid;
> @@ -1133,6 +1141,19 @@ static const struct io_op_def io_op_defs[] = {
> .needs_file = 1
> },
> [IORING_OP_GETXATTR] = {},
> + [IORING_OP_SENDTO] = {
> + .needs_file = 1,
> + .unbound_nonreg_file = 1,
> + .pollout = 1,
> + .audit_skip = 1,
> + },
> + [IORING_OP_RECVFROM] = {
> + .needs_file = 1,
> + .unbound_nonreg_file = 1,
> + .pollin = 1,
> + .buffer_select = 1,
> + .audit_skip = 1,
> + },
> };
>
> /* requests with any of those set should undergo io_disarm_next() */
> @@ -5216,12 +5237,24 @@ static int io_sendmsg_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
> if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
> return -EINVAL;
>
> + /*
> + * For IORING_OP_SEND{,TO}, the assignment to @sr->umsg
> + * is equivalent to an assignment to @sr->buf.
> + */
> sr->umsg = u64_to_user_ptr(READ_ONCE(sqe->addr));
> +
> sr->len = READ_ONCE(sqe->len);
> sr->msg_flags = READ_ONCE(sqe->msg_flags) | MSG_NOSIGNAL;
> if (sr->msg_flags & MSG_DONTWAIT)
> req->flags |= REQ_F_NOWAIT;
>
> + if (req->opcode == IORING_OP_SENDTO) {
> + sr->addr = u64_to_user_ptr(READ_ONCE(sqe->addr2));
> + sr->sendto_addr_len = READ_ONCE(sqe->addr3);
> + } else {
> + sr->addr = (struct sockaddr __user *) NULL;
Let's have sendto_addr_len = 0
> + }
> +
> #ifdef CONFIG_COMPAT
> if (req->ctx->compat)
> sr->msg_flags |= MSG_CMSG_COMPAT;
> @@ -5275,6 +5308,7 @@ static int io_sendmsg(struct io_kiocb *req, unsigned int issue_flags)
>
> static int io_sendto(struct io_kiocb *req, unsigned int issue_flags)
> {
> + struct sockaddr_storage address;
> struct io_sr_msg *sr = &req->sr_msg;
> struct msghdr msg;
> struct iovec iov;
> @@ -5291,10 +5325,20 @@ static int io_sendto(struct io_kiocb *req, unsigned int issue_flags)
> if (unlikely(ret))
> return ret;
>
> - msg.msg_name = NULL;
> +
> msg.msg_control = NULL;
> msg.msg_controllen = 0;
> - msg.msg_namelen = 0;
> + if (sr->addr) {
> + ret = move_addr_to_kernel(sr->addr, sr->sendto_addr_len,
> + &address);
> + if (unlikely(ret < 0))
> + goto fail;
> + msg.msg_name = (struct sockaddr *) &address;
> + msg.msg_namelen = sr->sendto_addr_len;
> + } else {
> + msg.msg_name = NULL;
> + msg.msg_namelen = 0;
> + }
>
> flags = req->sr_msg.msg_flags;
> if (issue_flags & IO_URING_F_NONBLOCK)
> @@ -5309,6 +5353,7 @@ static int io_sendto(struct io_kiocb *req, unsigned int issue_flags)
> return -EAGAIN;
> if (ret == -ERESTARTSYS)
> ret = -EINTR;
> + fail:
> req_set_fail(req);
I think there is a problem with "fail" goto statement. Not getting full clarity on this change. With latest kernel, I see req_set_fail(req) inside if check, which I don't see here. Can you please resend the patch on latest kernel version. Thanks.
> }
> __io_req_complete(req, issue_flags, ret, 0);
> @@ -5427,13 +5472,25 @@ static int io_recvmsg_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
> if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
> return -EINVAL;
>
> + /*
> + * For IORING_OP_RECV{,FROM}, the assignment to @sr->umsg
> + * is equivalent to an assignment to @sr->buf.
> + */
> sr->umsg = u64_to_user_ptr(READ_ONCE(sqe->addr));
> +
> sr->len = READ_ONCE(sqe->len);
> sr->bgid = READ_ONCE(sqe->buf_group);
> sr->msg_flags = READ_ONCE(sqe->msg_flags) | MSG_NOSIGNAL;
> if (sr->msg_flags & MSG_DONTWAIT)
> req->flags |= REQ_F_NOWAIT;
>
> + if (req->opcode == IORING_OP_RECVFROM) {
> + sr->addr = u64_to_user_ptr(READ_ONCE(sqe->addr2));
> + sr->recvfrom_addr_len = u64_to_user_ptr(READ_ONCE(sqe->addr3));
> + } else {
> + sr->addr = (struct sockaddr __user *) NULL;
I think recvfrom_addr_len should also be pointed to NULL, instead of garbage for this case.
> + }
> +
> #ifdef CONFIG_COMPAT
> if (req->ctx->compat)
> sr->msg_flags |= MSG_CMSG_COMPAT;
> @@ -5509,6 +5566,7 @@ static int io_recvfrom(struct io_kiocb *req, unsigned int issue_flags)
> struct iovec iov;
> unsigned flags;
> int ret, min_ret = 0;
> + struct sockaddr_storage address;
> bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
>
> sock = sock_from_file(req->file);
> @@ -5526,7 +5584,7 @@ static int io_recvfrom(struct io_kiocb *req, unsigned int issue_flags)
> if (unlikely(ret))
> goto out_free;
>
> - msg.msg_name = NULL;
> + msg.msg_name = sr->addr ? (struct sockaddr *) &address : NULL;
> msg.msg_control = NULL;
> msg.msg_controllen = 0;
> msg.msg_namelen = 0;
I think namelen should also be updated ?
> @@ -5540,6 +5598,16 @@ static int io_recvfrom(struct io_kiocb *req, unsigned int issue_flags)
> min_ret = iov_iter_count(&msg.msg_iter);
>
> ret = sock_recvmsg(sock, &msg, flags);
> +
> + if (ret >= 0 && sr->addr != NULL) {
> + int tmp;
> +
> + tmp = move_addr_to_user(&address, msg.msg_namelen, sr->addr,
> + sr->recvfrom_addr_len);
> + if (unlikely(tmp < 0))
> + ret = tmp;
> + }
> +
> out_free:
> if (ret < min_ret) {
> if (ret == -EAGAIN && force_nonblock)
> @@ -6778,9 +6846,11 @@ static int io_req_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
> case IORING_OP_SYNC_FILE_RANGE:
> return io_sfr_prep(req, sqe);
> case IORING_OP_SENDMSG:
> + case IORING_OP_SENDTO:
> case IORING_OP_SEND:
> return io_sendmsg_prep(req, sqe);
> case IORING_OP_RECVMSG:
> + case IORING_OP_RECVFROM:
> case IORING_OP_RECV:
> return io_recvmsg_prep(req, sqe);
> case IORING_OP_CONNECT:
> @@ -7060,12 +7130,14 @@ static int io_issue_sqe(struct io_kiocb *req, unsigned int issue_flags)
> case IORING_OP_SENDMSG:
> ret = io_sendmsg(req, issue_flags);
> break;
> + case IORING_OP_SENDTO:
> case IORING_OP_SEND:
> ret = io_sendto(req, issue_flags);
> break;
> case IORING_OP_RECVMSG:
> ret = io_recvmsg(req, issue_flags);
> break;
> + case IORING_OP_RECVFROM:
> case IORING_OP_RECV:
> ret = io_recvfrom(req, issue_flags);
> break;
> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> index efc7ac9b3a6b..a360069d1e8e 100644
> --- a/include/uapi/linux/io_uring.h
> +++ b/include/uapi/linux/io_uring.h
> @@ -150,6 +150,8 @@ enum {
> IORING_OP_SETXATTR,
> IORING_OP_FGETXATTR,
> IORING_OP_GETXATTR,
> + IORING_OP_SENDTO,
> + IORING_OP_RECVFROM,
>
> /* this goes last, obviously */
> IORING_OP_LAST,
Regards,
~Praveen.
Powered by blists - more mailing lists