[PATCH v3 0/4] forbid fix {SQ,IO}POLL

[PATCH v3 0/4] forbid fix {SQ,IO}POLL

[Pavel Begunkov]

The first one adds checks {SQPOLL,IOPOLL}. IOPOLL check can be
moved in the common path later, or rethinked entirely, e.g.
not io_iopoll_req_issued()'ed for unsupported opcodes.

3 others are just cleanups on top.


v2: add IOPOLL to the whole bunch of opcodes in [1/4].
    dirty and effective.
v3: sent wrong set in v2, re-sending right one 

Pavel Begunkov (4):
  io_uring: fix {SQ,IO}POLL with unsupported opcodes
  io_uring: do build_open_how() only once
  io_uring: deduplicate io_openat{,2}_prep()
  io_uring: move send/recv IOPOLL check into prep

 fs/io_uring.c | 94 ++++++++++++++++++++++++++-------------------------
 1 file changed, 48 insertions(+), 46 deletions(-)

-- 
2.24.0

[PATCH v3 1/4] io_uring: fix {SQ,IO}POLL with unsupported opcodes

[Pavel Begunkov]

IORING_SETUP_IOPOLL is defined only for read/write, other opcodes should
be disallowed, otherwise it'll get an error as below. Also refuse
open/cloes with SQPOLL, as the polling thread wouldn't know which file
table to use.

RIP: 0010:io_iopoll_getevents+0x111/0x5a0
Call Trace:
 ? _raw_spin_unlock_irqrestore+0x24/0x40
 ? do_send_sig_info+0x64/0x90
 io_iopoll_reap_events.part.0+0x5e/0xa0
 io_ring_ctx_wait_and_kill+0x132/0x1c0
 io_uring_release+0x20/0x30
 __fput+0xcd/0x230
 ____fput+0xe/0x10
 task_work_run+0x67/0xa0
 do_exit+0x353/0xb10
 ? handle_mm_fault+0xd4/0x200
 ? syscall_trace_enter+0x18c/0x2c0
 do_group_exit+0x43/0xa0
 __x64_sys_exit_group+0x18/0x20
 do_syscall_64+0x60/0x1e0
 entry_SYSCALL_64_after_hwframe+0x44/0xa9

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 732ec73ec3c0..fc55c44dcafe 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2765,6 +2765,8 @@ static int __io_splice_prep(struct io_kiocb *req,
 
 	if (req->flags & REQ_F_NEED_CLEANUP)
 		return 0;
+	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
+		return -EINVAL;
 
 	sp->file_in = NULL;
 	sp->len = READ_ONCE(sqe->len);
@@ -2965,6 +2967,8 @@ static int io_fallocate_prep(struct io_kiocb *req,
 {
 	if (sqe->ioprio || sqe->buf_index || sqe->rw_flags)
 		return -EINVAL;
+	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
+		return -EINVAL;
 
 	req->sync.off = READ_ONCE(sqe->off);
 	req->sync.len = READ_ONCE(sqe->addr);
@@ -2990,6 +2994,8 @@ static int io_openat_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	const char __user *fname;
 	int ret;
 
+	if (unlikely(req->ctx->flags & (IORING_SETUP_IOPOLL|IORING_SETUP_SQPOLL)))
+		return -EINVAL;
 	if (sqe->ioprio || sqe->buf_index)
 		return -EINVAL;
 	if (req->flags & REQ_F_FIXED_FILE)
@@ -3023,6 +3029,8 @@ static int io_openat2_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	size_t len;
 	int ret;
 
+	if (unlikely(req->ctx->flags & (IORING_SETUP_IOPOLL|IORING_SETUP_SQPOLL)))
+		return -EINVAL;
 	if (sqe->ioprio || sqe->buf_index)
 		return -EINVAL;
 	if (req->flags & REQ_F_FIXED_FILE)
@@ -3105,6 +3113,8 @@ static int io_remove_buffers_prep(struct io_kiocb *req,
 	struct io_provide_buf *p = &req->pbuf;
 	u64 tmp;
 
+	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
+		return -EINVAL;
 	if (sqe->ioprio || sqe->rw_flags || sqe->addr || sqe->len || sqe->off)
 		return -EINVAL;
 
@@ -3174,6 +3184,8 @@ static int io_provide_buffers_prep(struct io_kiocb *req,
 	struct io_provide_buf *p = &req->pbuf;
 	u64 tmp;
 
+	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
+		return -EINVAL;
 	if (sqe->ioprio || sqe->rw_flags)
 		return -EINVAL;
 
@@ -3262,6 +3274,8 @@ static int io_epoll_ctl_prep(struct io_kiocb *req,
 #if defined(CONFIG_EPOLL)
 	if (sqe->ioprio || sqe->buf_index)
 		return -EINVAL;
+	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
+		return -EINVAL;
 
 	req->epoll.epfd = READ_ONCE(sqe->fd);
 	req->epoll.op = READ_ONCE(sqe->len);
@@ -3306,6 +3320,8 @@ static int io_madvise_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 #if defined(CONFIG_ADVISE_SYSCALLS) && defined(CONFIG_MMU)
 	if (sqe->ioprio || sqe->buf_index || sqe->off)
 		return -EINVAL;
+	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
+		return -EINVAL;
 
 	req->madvise.addr = READ_ONCE(sqe->addr);
 	req->madvise.len = READ_ONCE(sqe->len);
@@ -3340,6 +3356,8 @@ static int io_fadvise_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 {
 	if (sqe->ioprio || sqe->buf_index || sqe->addr)
 		return -EINVAL;
+	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
+		return -EINVAL;
 
 	req->fadvise.offset = READ_ONCE(sqe->off);
 	req->fadvise.len = READ_ONCE(sqe->len);
@@ -3373,6 +3391,8 @@ static int io_fadvise(struct io_kiocb *req, bool force_nonblock)
 
 static int io_statx_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 {
+	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
+		return -EINVAL;
 	if (sqe->ioprio || sqe->buf_index)
 		return -EINVAL;
 	if (req->flags & REQ_F_FIXED_FILE)
@@ -3417,6 +3437,8 @@ static int io_close_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	 */
 	req->work.flags |= IO_WQ_WORK_NO_CANCEL;
 
+	if (unlikely(req->ctx->flags & (IORING_SETUP_IOPOLL|IORING_SETUP_SQPOLL)))
+		return -EINVAL;
 	if (sqe->ioprio || sqe->off || sqe->addr || sqe->len ||
 	    sqe->rw_flags || sqe->buf_index)
 		return -EINVAL;
@@ -4906,6 +4928,8 @@ static int io_files_update_prep(struct io_kiocb *req,
 {
 	if (sqe->flags || sqe->ioprio || sqe->rw_flags)
 		return -EINVAL;
+	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
+		return -EINVAL;
 
 	req->files_update.offset = READ_ONCE(sqe->off);
 	req->files_update.nr_args = READ_ONCE(sqe->len);
-- 
2.24.0

[PATCH v3 2/4] io_uring: do build_open_how() only once

[Pavel Begunkov]

build_open_how() is just adjusting open_flags/mode. Do it once during
prep. It looks better than storing raw values for the future.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index fc55c44dcafe..e3cd914557ae 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2992,6 +2992,7 @@ static int io_fallocate(struct io_kiocb *req, bool force_nonblock)
 static int io_openat_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 {
 	const char __user *fname;
+	u64 flags, mode;
 	int ret;
 
 	if (unlikely(req->ctx->flags & (IORING_SETUP_IOPOLL|IORING_SETUP_SQPOLL)))
@@ -3003,13 +3004,14 @@ static int io_openat_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	if (req->flags & REQ_F_NEED_CLEANUP)
 		return 0;
 
-	req->open.dfd = READ_ONCE(sqe->fd);
-	req->open.how.mode = READ_ONCE(sqe->len);
-	fname = u64_to_user_ptr(READ_ONCE(sqe->addr));
-	req->open.how.flags = READ_ONCE(sqe->open_flags);
+	mode = READ_ONCE(sqe->len);
+	flags = READ_ONCE(sqe->open_flags);
 	if (force_o_largefile())
-		req->open.how.flags |= O_LARGEFILE;
+		flags |= O_LARGEFILE;
+	req->open.how = build_open_how(flags, mode);
 
+	req->open.dfd = READ_ONCE(sqe->fd);
+	fname = u64_to_user_ptr(READ_ONCE(sqe->addr));
 	req->open.filename = getname(fname);
 	if (IS_ERR(req->open.filename)) {
 		ret = PTR_ERR(req->open.filename);
@@ -3103,7 +3105,6 @@ static int io_openat2(struct io_kiocb *req, bool force_nonblock)
 
 static int io_openat(struct io_kiocb *req, bool force_nonblock)
 {
-	req->open.how = build_open_how(req->open.how.flags, req->open.how.mode);
 	return io_openat2(req, force_nonblock);
 }
 
-- 
2.24.0

[PATCH v3 3/4] io_uring: deduplicate io_openat{,2}_prep()

[Pavel Begunkov]

io_openat_prep() and io_openat2_prep() are identical except for how
struct open_how is built. Deduplicate it with a helper.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 55 ++++++++++++++++++---------------------------------
 1 file changed, 19 insertions(+), 36 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index e3cd914557ae..134627cbe86b 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2989,26 +2989,21 @@ static int io_fallocate(struct io_kiocb *req, bool force_nonblock)
 	return 0;
 }
 
-static int io_openat_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
+static int __io_openat_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 {
 	const char __user *fname;
-	u64 flags, mode;
 	int ret;
 
 	if (unlikely(req->ctx->flags & (IORING_SETUP_IOPOLL|IORING_SETUP_SQPOLL)))
 		return -EINVAL;
-	if (sqe->ioprio || sqe->buf_index)
+	if (unlikely(sqe->ioprio || sqe->buf_index))
 		return -EINVAL;
-	if (req->flags & REQ_F_FIXED_FILE)
+	if (unlikely(req->flags & REQ_F_FIXED_FILE))
 		return -EBADF;
-	if (req->flags & REQ_F_NEED_CLEANUP)
-		return 0;
 
-	mode = READ_ONCE(sqe->len);
-	flags = READ_ONCE(sqe->open_flags);
-	if (force_o_largefile())
-		flags |= O_LARGEFILE;
-	req->open.how = build_open_how(flags, mode);
+	/* open.how should be already initialised */
+	if (!(req->open.how.flags & O_PATH) && force_o_largefile())
+		req->open.how.flags |= O_LARGEFILE;
 
 	req->open.dfd = READ_ONCE(sqe->fd);
 	fname = u64_to_user_ptr(READ_ONCE(sqe->addr));
@@ -3018,33 +3013,33 @@ static int io_openat_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 		req->open.filename = NULL;
 		return ret;
 	}
-
 	req->open.nofile = rlimit(RLIMIT_NOFILE);
 	req->flags |= REQ_F_NEED_CLEANUP;
 	return 0;
 }
 
+static int io_openat_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
+{
+	u64 flags, mode;
+
+	if (req->flags & REQ_F_NEED_CLEANUP)
+		return 0;
+	mode = READ_ONCE(sqe->len);
+	flags = READ_ONCE(sqe->open_flags);
+	req->open.how = build_open_how(flags, mode);
+	return __io_openat_prep(req, sqe);
+}
+
 static int io_openat2_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 {
 	struct open_how __user *how;
-	const char __user *fname;
 	size_t len;
 	int ret;
 
-	if (unlikely(req->ctx->flags & (IORING_SETUP_IOPOLL|IORING_SETUP_SQPOLL)))
-		return -EINVAL;
-	if (sqe->ioprio || sqe->buf_index)
-		return -EINVAL;
-	if (req->flags & REQ_F_FIXED_FILE)
-		return -EBADF;
 	if (req->flags & REQ_F_NEED_CLEANUP)
 		return 0;
-
-	req->open.dfd = READ_ONCE(sqe->fd);
-	fname = u64_to_user_ptr(READ_ONCE(sqe->addr));
 	how = u64_to_user_ptr(READ_ONCE(sqe->addr2));
 	len = READ_ONCE(sqe->len);
-
 	if (len < OPEN_HOW_SIZE_VER0)
 		return -EINVAL;
 
@@ -3053,19 +3048,7 @@ static int io_openat2_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	if (ret)
 		return ret;
 
-	if (!(req->open.how.flags & O_PATH) && force_o_largefile())
-		req->open.how.flags |= O_LARGEFILE;
-
-	req->open.filename = getname(fname);
-	if (IS_ERR(req->open.filename)) {
-		ret = PTR_ERR(req->open.filename);
-		req->open.filename = NULL;
-		return ret;
-	}
-
-	req->open.nofile = rlimit(RLIMIT_NOFILE);
-	req->flags |= REQ_F_NEED_CLEANUP;
-	return 0;
+	return __io_openat_prep(req, sqe);
 }
 
 static int io_openat2(struct io_kiocb *req, bool force_nonblock)
-- 
2.24.0

[PATCH v3 4/4] io_uring: move send/recv IOPOLL check into prep

[Pavel Begunkov]

Fail recv/send in case of IORING_SETUP_IOPOLL earlier during prep,
so it'd be done only once. Removes duplication as well

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 134627cbe86b..dee59c34acb3 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3555,6 +3555,9 @@ static int io_sendmsg_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	struct io_async_ctx *io = req->io;
 	int ret;
 
+	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
+		return -EINVAL;
+
 	sr->msg_flags = READ_ONCE(sqe->msg_flags);
 	sr->msg = u64_to_user_ptr(READ_ONCE(sqe->addr));
 	sr->len = READ_ONCE(sqe->len);
@@ -3584,9 +3587,6 @@ static int io_sendmsg(struct io_kiocb *req, bool force_nonblock)
 	struct socket *sock;
 	int ret;
 
-	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
-		return -EINVAL;
-
 	sock = sock_from_file(req->file, &ret);
 	if (sock) {
 		struct io_async_ctx io;
@@ -3640,9 +3640,6 @@ static int io_send(struct io_kiocb *req, bool force_nonblock)
 	struct socket *sock;
 	int ret;
 
-	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
-		return -EINVAL;
-
 	sock = sock_from_file(req->file, &ret);
 	if (sock) {
 		struct io_sr_msg *sr = &req->sr_msg;
@@ -3795,6 +3792,9 @@ static int io_recvmsg_prep(struct io_kiocb *req,
 	struct io_async_ctx *io = req->io;
 	int ret;
 
+	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
+		return -EINVAL;
+
 	sr->msg_flags = READ_ONCE(sqe->msg_flags);
 	sr->msg = u64_to_user_ptr(READ_ONCE(sqe->addr));
 	sr->len = READ_ONCE(sqe->len);
@@ -3823,9 +3823,6 @@ static int io_recvmsg(struct io_kiocb *req, bool force_nonblock)
 	struct socket *sock;
 	int ret, cflags = 0;
 
-	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
-		return -EINVAL;
-
 	sock = sock_from_file(req->file, &ret);
 	if (sock) {
 		struct io_buffer *kbuf;
@@ -3887,9 +3884,6 @@ static int io_recv(struct io_kiocb *req, bool force_nonblock)
 	struct socket *sock;
 	int ret, cflags = 0;
 
-	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
-		return -EINVAL;
-
 	sock = sock_from_file(req->file, &ret);
 	if (sock) {
 		struct io_sr_msg *sr = &req->sr_msg;
-- 
2.24.0

Re: [PATCH v3 0/4] forbid fix {SQ,IO}POLL

[Pavel Begunkov]

Not sure how this strange cv subject got copy-pasted, but
hopefully it's clear what it does from the description.

On 03/06/2020 18:03, Pavel Begunkov wrote:
> The first one adds checks {SQPOLL,IOPOLL}. IOPOLL check can be
> moved in the common path later, or rethinked entirely, e.g.
> not io_iopoll_req_issued()'ed for unsupported opcodes.
> 
> 3 others are just cleanups on top.
> 
> 
> v2: add IOPOLL to the whole bunch of opcodes in [1/4].
>     dirty and effective.
> v3: sent wrong set in v2, re-sending right one 
> 
> Pavel Begunkov (4):
>   io_uring: fix {SQ,IO}POLL with unsupported opcodes
>   io_uring: do build_open_how() only once
>   io_uring: deduplicate io_openat{,2}_prep()
>   io_uring: move send/recv IOPOLL check into prep
> 
>  fs/io_uring.c | 94 ++++++++++++++++++++++++++-------------------------
>  1 file changed, 48 insertions(+), 46 deletions(-)
> 

-- 
Pavel Begunkov

Re: [PATCH v3 0/4] forbid fix {SQ,IO}POLL

[Jens Axboe]

On 6/3/20 9:03 AM, Pavel Begunkov wrote:
> The first one adds checks {SQPOLL,IOPOLL}. IOPOLL check can be
> moved in the common path later, or rethinked entirely, e.g.
> not io_iopoll_req_issued()'ed for unsupported opcodes.
> 
> 3 others are just cleanups on top.
> 
> 
> v2: add IOPOLL to the whole bunch of opcodes in [1/4].
>     dirty and effective.
> v3: sent wrong set in v2, re-sending right one 
> 
> Pavel Begunkov (4):
>   io_uring: fix {SQ,IO}POLL with unsupported opcodes
>   io_uring: do build_open_how() only once
>   io_uring: deduplicate io_openat{,2}_prep()
>   io_uring: move send/recv IOPOLL check into prep
> 
>  fs/io_uring.c | 94 ++++++++++++++++++++++++++-------------------------
>  1 file changed, 48 insertions(+), 46 deletions(-)

Thanks, applied.

-- 
Jens Axboe

Re: [PATCH v3 0/4] forbid fix {SQ,IO}POLL

[Jens Axboe]

On 6/3/20 12:51 PM, Jens Axboe wrote:
> On 6/3/20 9:03 AM, Pavel Begunkov wrote:
>> The first one adds checks {SQPOLL,IOPOLL}. IOPOLL check can be
>> moved in the common path later, or rethinked entirely, e.g.
>> not io_iopoll_req_issued()'ed for unsupported opcodes.
>>
>> 3 others are just cleanups on top.
>>
>>
>> v2: add IOPOLL to the whole bunch of opcodes in [1/4].
>>     dirty and effective.
>> v3: sent wrong set in v2, re-sending right one 
>>
>> Pavel Begunkov (4):
>>   io_uring: fix {SQ,IO}POLL with unsupported opcodes
>>   io_uring: do build_open_how() only once
>>   io_uring: deduplicate io_openat{,2}_prep()
>>   io_uring: move send/recv IOPOLL check into prep
>>
>>  fs/io_uring.c | 94 ++++++++++++++++++++++++++-------------------------
>>  1 file changed, 48 insertions(+), 46 deletions(-)
> 
> Thanks, applied.

#1 goes too far, provide/remove buffers is fine with iopoll. I'll
going to edit the patch.

-- 
Jens Axboe

Re: [PATCH v3 0/4] forbid fix {SQ,IO}POLL

[Pavel Begunkov]

On 04/06/2020 20:06, Jens Axboe wrote:
> On 6/3/20 12:51 PM, Jens Axboe wrote:
>> On 6/3/20 9:03 AM, Pavel Begunkov wrote:
>>> The first one adds checks {SQPOLL,IOPOLL}. IOPOLL check can be
>>> moved in the common path later, or rethinked entirely, e.g.
>>> not io_iopoll_req_issued()'ed for unsupported opcodes.
>>>
>>> 3 others are just cleanups on top.
>>>
>>>
>>> v2: add IOPOLL to the whole bunch of opcodes in [1/4].
>>>     dirty and effective.
>>> v3: sent wrong set in v2, re-sending right one 
>>>
>>> Pavel Begunkov (4):
>>>   io_uring: fix {SQ,IO}POLL with unsupported opcodes
>>>   io_uring: do build_open_how() only once
>>>   io_uring: deduplicate io_openat{,2}_prep()
>>>   io_uring: move send/recv IOPOLL check into prep
>>>
>>>  fs/io_uring.c | 94 ++++++++++++++++++++++++++-------------------------
>>>  1 file changed, 48 insertions(+), 46 deletions(-)
>>
>> Thanks, applied.
> 
> #1 goes too far, provide/remove buffers is fine with iopoll. I'll
> going to edit the patch.

Conceptually it should work, but from a quick look:

- io_provide_buffers() drops a ref from req->refs, which should've
been used by iopoll*. E.g. io_complete_rw_iopoll() doesn't do that.

- it doesn't set REQ_F_IOPOLL_COMPLETED, thus iopoll* side will
call req->file->iopoll().

-- 
Pavel Begunkov

Re: [PATCH v3 0/4] forbid fix {SQ,IO}POLL

[Jens Axboe]

On 6/4/20 1:22 PM, Pavel Begunkov wrote:
> On 04/06/2020 20:06, Jens Axboe wrote:
>> On 6/3/20 12:51 PM, Jens Axboe wrote:
>>> On 6/3/20 9:03 AM, Pavel Begunkov wrote:
>>>> The first one adds checks {SQPOLL,IOPOLL}. IOPOLL check can be
>>>> moved in the common path later, or rethinked entirely, e.g.
>>>> not io_iopoll_req_issued()'ed for unsupported opcodes.
>>>>
>>>> 3 others are just cleanups on top.
>>>>
>>>>
>>>> v2: add IOPOLL to the whole bunch of opcodes in [1/4].
>>>>     dirty and effective.
>>>> v3: sent wrong set in v2, re-sending right one 
>>>>
>>>> Pavel Begunkov (4):
>>>>   io_uring: fix {SQ,IO}POLL with unsupported opcodes
>>>>   io_uring: do build_open_how() only once
>>>>   io_uring: deduplicate io_openat{,2}_prep()
>>>>   io_uring: move send/recv IOPOLL check into prep
>>>>
>>>>  fs/io_uring.c | 94 ++++++++++++++++++++++++++-------------------------
>>>>  1 file changed, 48 insertions(+), 46 deletions(-)
>>>
>>> Thanks, applied.
>>
>> #1 goes too far, provide/remove buffers is fine with iopoll. I'll
>> going to edit the patch.
> 
> Conceptually it should work, but from a quick look:
> 
> - io_provide_buffers() drops a ref from req->refs, which should've
> been used by iopoll*. E.g. io_complete_rw_iopoll() doesn't do that.
> 
> - it doesn't set REQ_F_IOPOLL_COMPLETED, thus iopoll* side will
> call req->file->iopoll().

We don't poll for provide/remove buffers, or file update. The
completion is done inline. The REQ_F_IOPOLL_COMPLETED and friends
is only applicable on read/writes.

-- 
Jens Axboe

Re: [PATCH v3 0/4] forbid fix {SQ,IO}POLL

[Pavel Begunkov]

On 04/06/2020 22:52, Jens Axboe wrote:
> On 6/4/20 1:22 PM, Pavel Begunkov wrote:
>> On 04/06/2020 20:06, Jens Axboe wrote:
>>> On 6/3/20 12:51 PM, Jens Axboe wrote:
>>>> On 6/3/20 9:03 AM, Pavel Begunkov wrote:
>>>>> The first one adds checks {SQPOLL,IOPOLL}. IOPOLL check can be
>>>>> moved in the common path later, or rethinked entirely, e.g.
>>>>> not io_iopoll_req_issued()'ed for unsupported opcodes.
>>>>>
>>>>> 3 others are just cleanups on top.
>>>>>
>>>>>
>>>>> v2: add IOPOLL to the whole bunch of opcodes in [1/4].
>>>>>     dirty and effective.
>>>>> v3: sent wrong set in v2, re-sending right one 
>>>>>
>>>>> Pavel Begunkov (4):
>>>>>   io_uring: fix {SQ,IO}POLL with unsupported opcodes
>>>>>   io_uring: do build_open_how() only once
>>>>>   io_uring: deduplicate io_openat{,2}_prep()
>>>>>   io_uring: move send/recv IOPOLL check into prep
>>>>>
>>>>>  fs/io_uring.c | 94 ++++++++++++++++++++++++++-------------------------
>>>>>  1 file changed, 48 insertions(+), 46 deletions(-)
>>>>
>>>> Thanks, applied.
>>>
>>> #1 goes too far, provide/remove buffers is fine with iopoll. I'll
>>> going to edit the patch.
>>
>> Conceptually it should work, but from a quick look:
>>
>> - io_provide_buffers() drops a ref from req->refs, which should've
>> been used by iopoll*. E.g. io_complete_rw_iopoll() doesn't do that.
>>
>> - it doesn't set REQ_F_IOPOLL_COMPLETED, thus iopoll* side will
>> call req->file->iopoll().
> 
> We don't poll for provide/remove buffers, or file update. The
> completion is done inline. The REQ_F_IOPOLL_COMPLETED and friends
> is only applicable on read/writes.
> 

1. Let io_provide_buffers() succeeds, putting a ref and returning 0

2. io_issue_sqe() on the way back do IORING_SETUP_IOPOLL check,
where it calls io_iopoll_req_issued(req)

3. io_iopoll_req_issued() unconditionally adds the req into ->poll_list

4. io_do_iopoll() checks the req, doesn't find it flagged with
REQ_F_IOPOLL_COMPLETED, and tries req->file->iopoll().


Do I miss something? Just did a quick and dirty test, which segfaulted.
Not certain about it though.

-- 
Pavel Begunkov

Re: [PATCH v3 0/4] forbid fix {SQ,IO}POLL

[Jens Axboe]

On 6/4/20 2:12 PM, Pavel Begunkov wrote:
> On 04/06/2020 22:52, Jens Axboe wrote:
>> On 6/4/20 1:22 PM, Pavel Begunkov wrote:
>>> On 04/06/2020 20:06, Jens Axboe wrote:
>>>> On 6/3/20 12:51 PM, Jens Axboe wrote:
>>>>> On 6/3/20 9:03 AM, Pavel Begunkov wrote:
>>>>>> The first one adds checks {SQPOLL,IOPOLL}. IOPOLL check can be
>>>>>> moved in the common path later, or rethinked entirely, e.g.
>>>>>> not io_iopoll_req_issued()'ed for unsupported opcodes.
>>>>>>
>>>>>> 3 others are just cleanups on top.
>>>>>>
>>>>>>
>>>>>> v2: add IOPOLL to the whole bunch of opcodes in [1/4].
>>>>>>     dirty and effective.
>>>>>> v3: sent wrong set in v2, re-sending right one 
>>>>>>
>>>>>> Pavel Begunkov (4):
>>>>>>   io_uring: fix {SQ,IO}POLL with unsupported opcodes
>>>>>>   io_uring: do build_open_how() only once
>>>>>>   io_uring: deduplicate io_openat{,2}_prep()
>>>>>>   io_uring: move send/recv IOPOLL check into prep
>>>>>>
>>>>>>  fs/io_uring.c | 94 ++++++++++++++++++++++++++-------------------------
>>>>>>  1 file changed, 48 insertions(+), 46 deletions(-)
>>>>>
>>>>> Thanks, applied.
>>>>
>>>> #1 goes too far, provide/remove buffers is fine with iopoll. I'll
>>>> going to edit the patch.
>>>
>>> Conceptually it should work, but from a quick look:
>>>
>>> - io_provide_buffers() drops a ref from req->refs, which should've
>>> been used by iopoll*. E.g. io_complete_rw_iopoll() doesn't do that.
>>>
>>> - it doesn't set REQ_F_IOPOLL_COMPLETED, thus iopoll* side will
>>> call req->file->iopoll().
>>
>> We don't poll for provide/remove buffers, or file update. The
>> completion is done inline. The REQ_F_IOPOLL_COMPLETED and friends
>> is only applicable on read/writes.
>>
> 
> 1. Let io_provide_buffers() succeeds, putting a ref and returning 0
> 
> 2. io_issue_sqe() on the way back do IORING_SETUP_IOPOLL check,
> where it calls io_iopoll_req_issued(req)

Only if req->file is valid, which it isn't for these non-file requests.

> 
> 3. io_iopoll_req_issued() unconditionally adds the req into ->poll_list
> 
> 4. io_do_iopoll() checks the req, doesn't find it flagged with
> REQ_F_IOPOLL_COMPLETED, and tries req->file->iopoll().
> 
> 
> Do I miss something? Just did a quick and dirty test, which segfaulted.
> Not certain about it though.
> 


-- 
Jens Axboe

Re: [PATCH v3 0/4] forbid fix {SQ,IO}POLL

[Pavel Begunkov]

On 04/06/2020 23:17, Jens Axboe wrote:
> On 6/4/20 2:12 PM, Pavel Begunkov wrote:
>> On 04/06/2020 22:52, Jens Axboe wrote:
>>> On 6/4/20 1:22 PM, Pavel Begunkov wrote:
>>>> On 04/06/2020 20:06, Jens Axboe wrote:
>>>>> On 6/3/20 12:51 PM, Jens Axboe wrote:
>>>>>> On 6/3/20 9:03 AM, Pavel Begunkov wrote:
>>>>>>> The first one adds checks {SQPOLL,IOPOLL}. IOPOLL check can be
>>>>>>> moved in the common path later, or rethinked entirely, e.g.
>>>>>>> not io_iopoll_req_issued()'ed for unsupported opcodes.
>>>>>>>
>>>>>>> 3 others are just cleanups on top.
>>>>>>>
>>>>>>>
>>>>>>> v2: add IOPOLL to the whole bunch of opcodes in [1/4].
>>>>>>>     dirty and effective.
>>>>>>> v3: sent wrong set in v2, re-sending right one 
>>>>>>>
>>>>>>> Pavel Begunkov (4):
>>>>>>>   io_uring: fix {SQ,IO}POLL with unsupported opcodes
>>>>>>>   io_uring: do build_open_how() only once
>>>>>>>   io_uring: deduplicate io_openat{,2}_prep()
>>>>>>>   io_uring: move send/recv IOPOLL check into prep
>>>>>>>
>>>>>>>  fs/io_uring.c | 94 ++++++++++++++++++++++++++-------------------------
>>>>>>>  1 file changed, 48 insertions(+), 46 deletions(-)
>>>>>>
>>>>>> Thanks, applied.
>>>>>
>>>>> #1 goes too far, provide/remove buffers is fine with iopoll. I'll
>>>>> going to edit the patch.
>>>>
>>>> Conceptually it should work, but from a quick look:
>>>>
>>>> - io_provide_buffers() drops a ref from req->refs, which should've
>>>> been used by iopoll*. E.g. io_complete_rw_iopoll() doesn't do that.
>>>>
>>>> - it doesn't set REQ_F_IOPOLL_COMPLETED, thus iopoll* side will
>>>> call req->file->iopoll().
>>>
>>> We don't poll for provide/remove buffers, or file update. The
>>> completion is done inline. The REQ_F_IOPOLL_COMPLETED and friends
>>> is only applicable on read/writes.
>>>
>>
>> 1. Let io_provide_buffers() succeeds, putting a ref and returning 0
>>
>> 2. io_issue_sqe() on the way back do IORING_SETUP_IOPOLL check,
>> where it calls io_iopoll_req_issued(req)
> 
> Only if req->file is valid, which it isn't for these non-file requests.

Ok, it looks like I miss your commit doing ->file check there.
Now sure how it slipped even though it's marked v5.7-rc7.


>>
>> 3. io_iopoll_req_issued() unconditionally adds the req into ->poll_list
>>
>> 4. io_do_iopoll() checks the req, doesn't find it flagged with
>> REQ_F_IOPOLL_COMPLETED, and tries req->file->iopoll().
>>
>>
>> Do I miss something? Just did a quick and dirty test, which segfaulted.
>> Not certain about it though.
>>
> 
> 

-- 
Pavel Begunkov