On 3/16/24 17:01, Jens Axboe wrote:
On 3/16/24 10:57 AM, Pavel Begunkov wrote:
On 3/16/24 16:51, Jens Axboe wrote:
On 3/16/24 10:46 AM, Pavel Begunkov wrote:
On 3/16/24 16:42, Jens Axboe wrote:
On 3/16/24 10:36 AM, Pavel Begunkov wrote:
On 3/16/24 16:36, Jens Axboe wrote:
On 3/16/24 10:32 AM, Pavel Begunkov wrote:
On 3/16/24 16:31, Jens Axboe wrote:
On 3/16/24 10:28 AM, Pavel Begunkov wrote:
On 3/16/24 16:14, Jens Axboe wrote:
On 3/15/24 5:28 PM, Pavel Begunkov wrote:
On 3/15/24 23:25, Jens Axboe wrote:
On 3/15/24 5:19 PM, Pavel Begunkov wrote:
On 3/15/24 23:13, Pavel Begunkov wrote:
On 3/15/24 23:09, Pavel Begunkov wrote:
On 3/15/24 22:48, Jens Axboe wrote:
If we get a request with IOSQE_ASYNC set, then we first run the prep
async handlers. But if we then fail setting it up and want to post
a CQE with -EINVAL, we use ->done_io. This was previously guarded with
REQ_F_PARTIAL_IO, and the normal setup handlers do set it up before any
potential errors, but we need to cover the async setup too.
You can hit io_req_defer_failed() { opdef->fail(); }
off of an early submission failure path where def->prep has
not yet been called, I don't think the patch will fix the
problem.
->fail() handlers are fragile, maybe we should skip them
if def->prep() wasn't called. Not even compile tested:
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 846d67a9c72e..56eed1490571 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
[...]
def->fail(req);
io_req_complete_defer(req);
}
@@ -2201,8 +2201,7 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
}
req->flags |= REQ_F_CREDS;
}
-
- return def->prep(req, sqe);
+ return 0;
}
static __cold int io_submit_fail_init(const struct io_uring_sqe *sqe,
@@ -2250,8 +2249,15 @@ static inline int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
int ret;
ret = io_init_req(ctx, req, sqe);
- if (unlikely(ret))
+ if (unlikely(ret)) {
+fail:
Obvious the diff is crap, but still bugging me enough to write
that the label should've been one line below, otherwise we'd
flag after ->prep as well.
It certainly needs testing :-)
We can go either way - patch up the net thing, or do a proper EARLY_FAIL
and hopefully not have to worry about it again. Do you want to clean it
up, test it, and send it out?
I'd rather leave it to you, I suspect it wouldn't fix the syzbot
report w/o fiddling with done_io as in your patch.
I gave this a shot, but some fail handlers do want to get called. But
Which one and/or which part of it?
send zc
I don't think so. If prep wasn't called there wouldn't be
a notif allocated, and so no F_MORE required. If you take
at the code path it's under REQ_F_NEED_CLEANUP, which is only
set by opcode handlers
I'm not making this up, your test case will literally fail as it doesn't
get to flag MORE for that case. FWIW, this was done with EARLY_FAIL
being flagged, and failing if we fail during or before prep.
Maybe the test is too strict, but your approach is different
from what I mentioned yesterday
- return def->prep(req, sqe);
+ ret = def->prep(req, sqe);
+ if (unlikely(ret)) {
+ req->flags |= REQ_F_EARLY_FAIL;
+ return ret;
+ }
+
+ return 0;
It should only set REQ_F_EARLY_FAIL if we fail
_before_ prep is called
I did try both ways, fails if we just have:
Ok, but the point is that the sendzc's ->fail doesn't
need to be called unless you've done ->prep first.
But it fails, not sure how else to say it.
liburing tests? Which test case? If so, it should be another
Like I mentioned earlier, it's send zc and it's failing the test case
for that. test/send-zerocopy.t.
bug. REQ_F_NEED_CLEANUP is only set by opcodes, if a request is
terminated before ->prep is called, it means it never entered
any of the opdef callbacks and have never seen any of net.c
code, so there should be no REQ_F_NEED_CLEANUP, and so
io_sendrecv_fail() wouldn't try to set F_MORE. I don't know
what's wrong.
Feel free to take a look! I do like the simplicity of the early error
flag.
./send-zerocopy.t works fine
diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
index ea7e5488b3be..de3a2c67c4a7 100644
--- a/include/linux/io_uring_types.h
+++ b/include/linux/io_uring_types.h
@@ -478,6 +478,7 @@ enum {
REQ_F_CAN_POLL_BIT,
REQ_F_BL_EMPTY_BIT,
REQ_F_BL_NO_RECYCLE_BIT,
+ REQ_F_UNPREPPED_FAIL_BIT,
/* not a real bit, just to check we're not overflowing the space */
__REQ_F_LAST_BIT,
@@ -556,6 +557,8 @@ enum {
REQ_F_BL_EMPTY = IO_REQ_FLAG(REQ_F_BL_EMPTY_BIT),
/* don't recycle provided buffers for this request */
REQ_F_BL_NO_RECYCLE = IO_REQ_FLAG(REQ_F_BL_NO_RECYCLE_BIT),
+
+ REQ_F_UNPREPPED_FAIL = IO_REQ_FLAG(REQ_F_UNPREPPED_FAIL_BIT),
};
typedef void (*io_req_tw_func_t)(struct io_kiocb *req, struct io_tw_state *ts);
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 846d67a9c72e..6523fa4c5630 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -993,7 +993,7 @@ void io_req_defer_failed(struct io_kiocb *req, s32 res)
req_set_fail(req);
io_req_set_res(req, res, io_put_kbuf(req, IO_URING_F_UNLOCKED));
- if (def->fail)
+ if (!(req->flags & REQ_F_UNPREPPED_FAIL) && def->fail)
def->fail(req);
io_req_complete_defer(req);
}
@@ -2201,8 +2201,7 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
}
req->flags |= REQ_F_CREDS;
}
-
- return def->prep(req, sqe);
+ return 0;
}
static __cold int io_submit_fail_init(const struct io_uring_sqe *sqe,
@@ -2250,7 +2249,13 @@ static inline int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
int ret;
ret = io_init_req(ctx, req, sqe);
- if (unlikely(ret))
+ if (unlikely(ret)) {
+ req->flags |= REQ_F_UNPREPPED_FAIL;
+ return io_submit_fail_init(sqe, req, ret);
+ }
+
+ ret = def->prep(req, sqe);
+ if (ret)
return io_submit_fail_init(sqe, req, ret);
trace_io_uring_submit_req(req);
--
Pavel Begunkov
