From 65d43023171edc0d27208f6ac7a1a73732950cf7 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Fri, 26 Feb 2021 09:50:55 -0700 Subject: io-wq: wait for worker startup when forking a new one We need to have our worker count updated before continuing, to avoid cases where we repeatedly think we need a new worker, but a fork is already in progress. Signed-off-by: Jens Axboe --- fs/io-wq.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'fs') diff --git a/fs/io-wq.c b/fs/io-wq.c index 44e20248805a..965022fe9961 100644 --- a/fs/io-wq.c +++ b/fs/io-wq.c @@ -56,6 +56,7 @@ struct io_worker { const struct cred *saved_creds; struct completion ref_done; + struct completion started; struct rcu_head rcu; }; @@ -267,6 +268,7 @@ static void io_worker_start(struct io_worker *worker) { worker->flags |= (IO_WORKER_F_UP | IO_WORKER_F_RUNNING); io_wqe_inc_running(worker); + complete(&worker->started); } /* @@ -644,6 +646,7 @@ static bool create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index) worker->wqe = wqe; spin_lock_init(&worker->lock); init_completion(&worker->ref_done); + init_completion(&worker->started); refcount_inc(&wq->refs); @@ -656,6 +659,7 @@ static bool create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index) kfree(worker); return false; } + wait_for_completion(&worker->started); return true; } -- cgit From fb3a1f6c745ccd896afadf6e2d6f073e871d38ba Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Fri, 26 Feb 2021 09:47:20 -0700 Subject: io-wq: have manager wait for all workers to exit Instead of having to wait separately on workers and manager, just have the manager wait on the workers. We use an atomic_t for the reference here, as we need to start at 0 and allow increment from that. Since the number of workers is naturally capped by the allowed nr of processes, and that uses an int, there is no risk of overflow. Signed-off-by: Jens Axboe --- fs/io-wq.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) (limited to 'fs') diff --git a/fs/io-wq.c b/fs/io-wq.c index 965022fe9961..1d01edada8aa 100644 --- a/fs/io-wq.c +++ b/fs/io-wq.c @@ -120,6 +120,9 @@ struct io_wq { refcount_t refs; struct completion done; + atomic_t worker_refs; + struct completion worker_done; + struct hlist_node cpuhp_node; pid_t task_pid; @@ -189,7 +192,8 @@ static void io_worker_exit(struct io_worker *worker) raw_spin_unlock_irq(&wqe->lock); kfree_rcu(worker, rcu); - io_wq_put(wqe->wq); + if (atomic_dec_and_test(&wqe->wq->worker_refs)) + complete(&wqe->wq->worker_done); } static inline bool io_wqe_run_queue(struct io_wqe *wqe) @@ -648,14 +652,15 @@ static bool create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index) init_completion(&worker->ref_done); init_completion(&worker->started); - refcount_inc(&wq->refs); + atomic_inc(&wq->worker_refs); if (index == IO_WQ_ACCT_BOUND) pid = io_wq_fork_thread(task_thread_bound, worker); else pid = io_wq_fork_thread(task_thread_unbound, worker); if (pid < 0) { - io_wq_put(wq); + if (atomic_dec_and_test(&wq->worker_refs)) + complete(&wq->worker_done); kfree(worker); return false; } @@ -736,6 +741,7 @@ static int io_wq_manager(void *data) { struct io_wq *wq = data; char buf[TASK_COMM_LEN]; + int node; sprintf(buf, "iou-mgr-%d", wq->task_pid); set_task_comm(current, buf); @@ -753,6 +759,15 @@ static int io_wq_manager(void *data) } while (!test_bit(IO_WQ_BIT_EXIT, &wq->state)); io_wq_check_workers(wq); + + rcu_read_lock(); + for_each_node(node) + io_wq_for_each_worker(wq->wqes[node], io_wq_worker_wake, NULL); + rcu_read_unlock(); + + /* we might not ever have created any workers */ + if (atomic_read(&wq->worker_refs)) + wait_for_completion(&wq->worker_done); wq->manager = NULL; io_wq_put(wq); do_exit(0); @@ -796,6 +811,7 @@ static int io_wq_fork_manager(struct io_wq *wq) if (wq->manager) return 0; + reinit_completion(&wq->worker_done); clear_bit(IO_WQ_BIT_EXIT, &wq->state); refcount_inc(&wq->refs); current->flags |= PF_IO_WORKER; @@ -1050,6 +1066,9 @@ struct io_wq *io_wq_create(unsigned bounded, struct io_wq_data *data) init_completion(&wq->done); refcount_set(&wq->refs, 1); + init_completion(&wq->worker_done); + atomic_set(&wq->worker_refs, 0); + ret = io_wq_fork_manager(wq); if (!ret) return wq; @@ -1077,11 +1096,6 @@ static void io_wq_destroy(struct io_wq *wq) if (wq->manager) wake_up_process(wq->manager); - rcu_read_lock(); - for_each_node(node) - io_wq_for_each_worker(wq->wqes[node], io_wq_worker_wake, NULL); - rcu_read_unlock(); - spin_lock_irq(&wq->hash->wait.lock); for_each_node(node) { struct io_wqe *wqe = wq->wqes[node]; -- cgit From 613eeb600e3e636a1d3b3711dddaf2b134d5a32c Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Fri, 26 Feb 2021 09:52:02 -0700 Subject: io-wq: don't ask for a new worker if we're exiting If we're in the process of shutting down the async context, then don't create new workers if we already have at least the fixed one. Signed-off-by: Jens Axboe --- fs/io-wq.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'fs') diff --git a/fs/io-wq.c b/fs/io-wq.c index 1d01edada8aa..2f9d7ee12ee1 100644 --- a/fs/io-wq.c +++ b/fs/io-wq.c @@ -673,6 +673,8 @@ static inline bool io_wqe_need_worker(struct io_wqe *wqe, int index) { struct io_wqe_acct *acct = &wqe->acct[index]; + if (acct->nr_workers && test_bit(IO_WQ_BIT_EXIT, &wqe->wq->state)) + return false; /* if we have available workers or no work, no need */ if (!hlist_nulls_empty(&wqe->free_list) || !io_wqe_run_queue(wqe)) return false; -- cgit From dbf996202e28c6b1eb30afad534abe45a691499e Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Fri, 26 Feb 2021 09:53:17 -0700 Subject: io-wq: rename wq->done completion to wq->started This is a leftover from a different use cases, it's used to wait for the manager to startup. Rename it as such. Signed-off-by: Jens Axboe --- fs/io-wq.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'fs') diff --git a/fs/io-wq.c b/fs/io-wq.c index 2f9d7ee12ee1..1e5b41614bd6 100644 --- a/fs/io-wq.c +++ b/fs/io-wq.c @@ -118,7 +118,7 @@ struct io_wq { struct io_wq_hash *hash; refcount_t refs; - struct completion done; + struct completion started; atomic_t worker_refs; struct completion worker_done; @@ -750,7 +750,7 @@ static int io_wq_manager(void *data) current->flags |= PF_IO_WORKER; wq->manager = current; - complete(&wq->done); + complete(&wq->started); do { set_current_state(TASK_INTERRUPTIBLE); @@ -820,7 +820,7 @@ static int io_wq_fork_manager(struct io_wq *wq) ret = io_wq_fork_thread(io_wq_manager, wq); current->flags &= ~PF_IO_WORKER; if (ret >= 0) { - wait_for_completion(&wq->done); + wait_for_completion(&wq->started); return 0; } @@ -1065,7 +1065,7 @@ struct io_wq *io_wq_create(unsigned bounded, struct io_wq_data *data) } wq->task_pid = current->pid; - init_completion(&wq->done); + init_completion(&wq->started); refcount_set(&wq->refs, 1); init_completion(&wq->worker_done); -- cgit From d364d9e5db41678b77ed95c41e3ccaad9ab99ba0 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Fri, 26 Feb 2021 09:56:32 -0700 Subject: io-wq: wait for manager exit on wq destroy The manager waits for the workers, hence the manager is always valid if workers are running. Now also have wq destroy wait for the manager on exit, so we now everything is gone. Signed-off-by: Jens Axboe --- fs/io-wq.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'fs') diff --git a/fs/io-wq.c b/fs/io-wq.c index 1e5b41614bd6..7a1d51c1aca9 100644 --- a/fs/io-wq.c +++ b/fs/io-wq.c @@ -119,6 +119,7 @@ struct io_wq { refcount_t refs; struct completion started; + struct completion exited; atomic_t worker_refs; struct completion worker_done; @@ -771,6 +772,7 @@ static int io_wq_manager(void *data) if (atomic_read(&wq->worker_refs)) wait_for_completion(&wq->worker_done); wq->manager = NULL; + complete(&wq->exited); io_wq_put(wq); do_exit(0); } @@ -1066,6 +1068,7 @@ struct io_wq *io_wq_create(unsigned bounded, struct io_wq_data *data) wq->task_pid = current->pid; init_completion(&wq->started); + init_completion(&wq->exited); refcount_set(&wq->refs, 1); init_completion(&wq->worker_done); @@ -1095,8 +1098,10 @@ static void io_wq_destroy(struct io_wq *wq) cpuhp_state_remove_instance_nocalls(io_wq_online, &wq->cpuhp_node); set_bit(IO_WQ_BIT_EXIT, &wq->state); - if (wq->manager) + if (wq->manager) { wake_up_process(wq->manager); + wait_for_completion(&wq->exited); + } spin_lock_irq(&wq->hash->wait.lock); for_each_node(node) { -- cgit From 470ec4ed8c91b4db398ad607c700e9ce88365202 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Fri, 26 Feb 2021 10:20:34 -0700 Subject: io-wq: fix double put of 'wq' in error path We are already freeing the wq struct in both spots, so don't put it and get it freed twice. Reported-by: syzbot+7bf785eedca35ca05501@syzkaller.appspotmail.com Fixes: 4fb6ac326204 ("io-wq: improve manager/worker handling over exec") Signed-off-by: Jens Axboe --- fs/io-wq.c | 2 -- 1 file changed, 2 deletions(-) (limited to 'fs') diff --git a/fs/io-wq.c b/fs/io-wq.c index 7a1d51c1aca9..f0b7e9ff63fa 100644 --- a/fs/io-wq.c +++ b/fs/io-wq.c @@ -826,7 +826,6 @@ static int io_wq_fork_manager(struct io_wq *wq) return 0; } - io_wq_put(wq); return ret; } @@ -1078,7 +1077,6 @@ struct io_wq *io_wq_create(unsigned bounded, struct io_wq_data *data) if (!ret) return wq; - io_wq_put(wq); io_wq_put_hash(data->hash); err: cpuhp_state_remove_instance_nocalls(io_wq_online, &wq->cpuhp_node); -- cgit From e54945ae947fb881212a4b97d5599a01bba6ad06 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Fri, 26 Feb 2021 11:27:15 -0700 Subject: io_uring: SQPOLL stop error handling fixes If we fail to fork an SQPOLL worker, we can hit cancel, and hence attempted thread stop, with the thread already being stopped. Ensure we check for that. Also guard thread stop fully by the sqd mutex, just like we do for park. Signed-off-by: Jens Axboe --- fs/io_uring.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index 4a088581b0f2..d55c9ab6314a 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -6793,9 +6793,9 @@ static int io_sq_thread(void *data) ctx->sqo_exec = 1; io_ring_set_wakeup_flag(ctx); } - mutex_unlock(&sqd->lock); complete(&sqd->exited); + mutex_unlock(&sqd->lock); do_exit(0); } @@ -7118,13 +7118,19 @@ static bool io_sq_thread_park(struct io_sq_data *sqd) static void io_sq_thread_stop(struct io_sq_data *sqd) { - if (!sqd->thread) + if (test_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state)) return; - - set_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state); - WARN_ON_ONCE(test_bit(IO_SQ_THREAD_SHOULD_PARK, &sqd->state)); - wake_up_process(sqd->thread); - wait_for_completion(&sqd->exited); + mutex_lock(&sqd->lock); + if (sqd->thread) { + set_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state); + WARN_ON_ONCE(test_bit(IO_SQ_THREAD_SHOULD_PARK, &sqd->state)); + wake_up_process(sqd->thread); + mutex_unlock(&sqd->lock); + wait_for_completion(&sqd->exited); + WARN_ON_ONCE(sqd->thread); + } else { + mutex_unlock(&sqd->lock); + } } static void io_put_sq_data(struct io_sq_data *sqd) @@ -8867,6 +8873,11 @@ static void io_uring_cancel_sqpoll(struct io_ring_ctx *ctx) if (!io_sq_thread_park(sqd)) return; tctx = ctx->sq_data->thread->io_uring; + /* can happen on fork/alloc failure, just ignore that state */ + if (!tctx) { + io_sq_thread_unpark(sqd); + return; + } atomic_inc(&tctx->in_idle); do { -- cgit From ba50a036f23c44608b1d903c34644a1acd5d21fa Mon Sep 17 00:00:00 2001 From: Pavel Begunkov Date: Fri, 26 Feb 2021 15:47:56 +0000 Subject: io_uring: run fallback on cancellation io_uring_try_cancel_requests() matches not only current's requests, but also of other exiting tasks, so we need to actively cancel them and not just wait, especially since the function can be called on flush during do_exit() -> exit_files(). Even if it's not a problem for now, it's much nicer to know that the function tries to cancel everything it can. Signed-off-by: Pavel Begunkov Signed-off-by: Jens Axboe --- fs/io_uring.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index d55c9ab6314a..9d6696ff5748 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -8518,9 +8518,10 @@ static int io_remove_personalities(int id, void *p, void *data) return 0; } -static void io_run_ctx_fallback(struct io_ring_ctx *ctx) +static bool io_run_ctx_fallback(struct io_ring_ctx *ctx) { struct callback_head *work, *head, *next; + bool executed = false; do { do { @@ -8537,7 +8538,10 @@ static void io_run_ctx_fallback(struct io_ring_ctx *ctx) work = next; cond_resched(); } while (work); + executed = true; } while (1); + + return executed; } static void io_ring_exit_work(struct work_struct *work) @@ -8677,6 +8681,7 @@ static void io_uring_try_cancel_requests(struct io_ring_ctx *ctx, ret |= io_poll_remove_all(ctx, task, files); ret |= io_kill_timeouts(ctx, task, files); ret |= io_run_task_work(); + ret |= io_run_ctx_fallback(ctx); io_cqring_overflow_flush(ctx, true, task, files); if (!ret) break; -- cgit From 8629397e6e2753bb4cc62ba48a12e1d4d912b6a4 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Fri, 26 Feb 2021 13:46:49 -0700 Subject: io_uring: don't use complete_all() on SQPOLL thread exit We want to reuse this completion, and a single complete should do just fine. Ensure that we park ourselves first if requested, as that is what lead to the initial deadlock in this area. If we've got someone attempting to park us, then we can't proceed without having them finish first. Fixes: 37d1e2e3642e ("io_uring: move SQPOLL thread io-wq forked worker") Signed-off-by: Jens Axboe --- fs/io_uring.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index 9d6696ff5748..904bf0fecc36 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -6783,10 +6783,13 @@ static int io_sq_thread(void *data) io_run_task_work(); + if (io_sq_thread_should_park(sqd)) + io_sq_thread_parkme(sqd); + /* * Clear thread under lock so that concurrent parks work correctly */ - complete_all(&sqd->completion); + complete(&sqd->completion); mutex_lock(&sqd->lock); sqd->thread = NULL; list_for_each_entry(ctx, &sqd->ctx_list, sqd_list) { -- cgit From afcc4015d1bf5659b8c722aff679e9b8c41ee156 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Fri, 26 Feb 2021 13:48:19 -0700 Subject: io-wq: provide an io_wq_put_and_exit() helper If we put the io-wq from io_uring, we really want it to exit. Provide a helper that does that for us. Couple that with not having the manager hold a reference to the 'wq' and the normal SQPOLL exit will tear down the io-wq context appropriate. On the io-wq side, our wq context is per task, so only the task itself is manipulating ->manager and hence it's safe to check and clear without any extra locking. We just need to ensure that the manager task stays around, in case it exits. Signed-off-by: Jens Axboe --- fs/io-wq.c | 29 +++++++++++++++++++---------- fs/io-wq.h | 1 + fs/io_uring.c | 2 +- 3 files changed, 21 insertions(+), 11 deletions(-) (limited to 'fs') diff --git a/fs/io-wq.c b/fs/io-wq.c index f0b7e9ff63fa..1407ba74ffc3 100644 --- a/fs/io-wq.c +++ b/fs/io-wq.c @@ -749,7 +749,7 @@ static int io_wq_manager(void *data) sprintf(buf, "iou-mgr-%d", wq->task_pid); set_task_comm(current, buf); current->flags |= PF_IO_WORKER; - wq->manager = current; + wq->manager = get_task_struct(current); complete(&wq->started); @@ -771,9 +771,7 @@ static int io_wq_manager(void *data) /* we might not ever have created any workers */ if (atomic_read(&wq->worker_refs)) wait_for_completion(&wq->worker_done); - wq->manager = NULL; complete(&wq->exited); - io_wq_put(wq); do_exit(0); } @@ -816,8 +814,6 @@ static int io_wq_fork_manager(struct io_wq *wq) return 0; reinit_completion(&wq->worker_done); - clear_bit(IO_WQ_BIT_EXIT, &wq->state); - refcount_inc(&wq->refs); current->flags |= PF_IO_WORKER; ret = io_wq_fork_thread(io_wq_manager, wq); current->flags &= ~PF_IO_WORKER; @@ -1089,6 +1085,16 @@ err_wq: return ERR_PTR(ret); } +static void io_wq_destroy_manager(struct io_wq *wq) +{ + if (wq->manager) { + wake_up_process(wq->manager); + wait_for_completion(&wq->exited); + put_task_struct(wq->manager); + wq->manager = NULL; + } +} + static void io_wq_destroy(struct io_wq *wq) { int node; @@ -1096,10 +1102,7 @@ static void io_wq_destroy(struct io_wq *wq) cpuhp_state_remove_instance_nocalls(io_wq_online, &wq->cpuhp_node); set_bit(IO_WQ_BIT_EXIT, &wq->state); - if (wq->manager) { - wake_up_process(wq->manager); - wait_for_completion(&wq->exited); - } + io_wq_destroy_manager(wq); spin_lock_irq(&wq->hash->wait.lock); for_each_node(node) { @@ -1112,7 +1115,6 @@ static void io_wq_destroy(struct io_wq *wq) io_wq_put_hash(wq->hash); kfree(wq->wqes); kfree(wq); - } void io_wq_put(struct io_wq *wq) @@ -1121,6 +1123,13 @@ void io_wq_put(struct io_wq *wq) io_wq_destroy(wq); } +void io_wq_put_and_exit(struct io_wq *wq) +{ + set_bit(IO_WQ_BIT_EXIT, &wq->state); + io_wq_destroy_manager(wq); + io_wq_put(wq); +} + static bool io_wq_worker_affinity(struct io_worker *worker, void *data) { struct task_struct *task = worker->task; diff --git a/fs/io-wq.h b/fs/io-wq.h index b6ca12b60c35..f6ef433df8a8 100644 --- a/fs/io-wq.h +++ b/fs/io-wq.h @@ -114,6 +114,7 @@ struct io_wq_data { struct io_wq *io_wq_create(unsigned bounded, struct io_wq_data *data); void io_wq_put(struct io_wq *wq); +void io_wq_put_and_exit(struct io_wq *wq); void io_wq_enqueue(struct io_wq *wq, struct io_wq_work *work); void io_wq_hash_work(struct io_wq_work *work, void *val); diff --git a/fs/io_uring.c b/fs/io_uring.c index 904bf0fecc36..cb65e54c1b09 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -8857,7 +8857,7 @@ void __io_uring_files_cancel(struct files_struct *files) if (files) { io_uring_remove_task_files(tctx); if (tctx->io_wq) { - io_wq_put(tctx->io_wq); + io_wq_put_and_exit(tctx->io_wq); tctx->io_wq = NULL; } } -- cgit From 1d5f360dd1a3c04e00a52af74dd84fdb0e1d454b Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Fri, 26 Feb 2021 14:54:16 -0700 Subject: io_uring: fix race condition in task_work add and clear We clear the bit marking the ctx task_work as active after having run the queued work, but we really should be clearing it before. Otherwise we can hit a tiny race ala: CPU0 CPU1 io_task_work_add() tctx_task_work() run_work add_to_list test_and_set_bit clear_bit already set and CPU0 will return thinking the task_work is queued, while in reality it's already being run. If we hit the condition after __tctx_task_work() found no more work, but before we've cleared the bit, then we'll end up thinking it's queued and will be run. In reality it is queued, but we didn't queue the ctx task_work to ensure that it gets run. Fixes: 7cbf1722d5fc ("io_uring: provide FIFO ordering for task_work") Signed-off-by: Jens Axboe --- fs/io_uring.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index cb65e54c1b09..83973f6b3c0a 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -1893,10 +1893,10 @@ static void tctx_task_work(struct callback_head *cb) { struct io_uring_task *tctx = container_of(cb, struct io_uring_task, task_work); + clear_bit(0, &tctx->task_state); + while (__tctx_task_work(tctx)) cond_resched(); - - clear_bit(0, &tctx->task_state); } static int io_task_work_add(struct task_struct *tsk, struct io_kiocb *req, -- cgit From ef8eaa4e65facb1f51a64dbb4f5500134622c67c Mon Sep 17 00:00:00 2001 From: Pavel Begunkov Date: Sat, 27 Feb 2021 11:16:45 +0000 Subject: io_uring: warn on not destroyed io-wq Make sure that we killed an io-wq by the time a task is dead. Signed-off-by: Pavel Begunkov Signed-off-by: Jens Axboe --- fs/io_uring.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index 83973f6b3c0a..796b6d1f72f9 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -7843,6 +7843,8 @@ void __io_uring_free(struct task_struct *tsk) struct io_uring_task *tctx = tsk->io_uring; WARN_ON_ONCE(!xa_empty(&tctx->xa)); + WARN_ON_ONCE(tctx->io_wq); + percpu_counter_destroy(&tctx->inflight); kfree(tctx); tsk->io_uring = NULL; -- cgit From 8452d4a674b0e59bd53baef0b30b018690dde594 Mon Sep 17 00:00:00 2001 From: Pavel Begunkov Date: Sat, 27 Feb 2021 11:16:46 +0000 Subject: io_uring: destroy io-wq on exec Destroy current's io-wq backend and tctx on __io_uring_task_cancel(), aka exec(). Looks it's not strictly necessary, because it will be done at some point when the task dies and changes of creds/files/etc. are handled, but better to do that earlier to free io-wq and not potentially lock previous mm and other resources for the time being. It's safe to do because we wait for all requests of the current task to complete, so no request will use tctx afterwards. Note, that io_uring_files_cancel() may leave some requests for later reaping, so it leaves tctx intact, that's ok as the task is dying anyway. Signed-off-by: Pavel Begunkov Signed-off-by: Jens Axboe --- fs/io_uring.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index 796b6d1f72f9..73d1bd8db1bb 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -8835,13 +8835,17 @@ static void io_uring_del_task_file(struct file *file) fput(file); } -static void io_uring_remove_task_files(struct io_uring_task *tctx) +static void io_uring_clean_tctx(struct io_uring_task *tctx) { struct file *file; unsigned long index; xa_for_each(&tctx->xa, index, file) io_uring_del_task_file(file); + if (tctx->io_wq) { + io_wq_put_and_exit(tctx->io_wq); + tctx->io_wq = NULL; + } } void __io_uring_files_cancel(struct files_struct *files) @@ -8856,13 +8860,8 @@ void __io_uring_files_cancel(struct files_struct *files) io_uring_cancel_task_requests(file->private_data, files); atomic_dec(&tctx->in_idle); - if (files) { - io_uring_remove_task_files(tctx); - if (tctx->io_wq) { - io_wq_put_and_exit(tctx->io_wq); - tctx->io_wq = NULL; - } - } + if (files) + io_uring_clean_tctx(tctx); } static s64 tctx_inflight(struct io_uring_task *tctx) @@ -8954,7 +8953,9 @@ void __io_uring_task_cancel(void) atomic_dec(&tctx->in_idle); - io_uring_remove_task_files(tctx); + io_uring_clean_tctx(tctx); + /* all current's requests should be gone, we can kill tctx */ + __io_uring_free(current); } static int io_uring_flush(struct file *file, void *data) -- cgit From 4010fec41fd9fc5ca6956b958d14b32e41aded48 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Sat, 27 Feb 2021 15:04:18 -0700 Subject: io_uring: remove unused argument 'tsk' from io_req_caches_free() We prune the full cache regardless, get rid of the dead argument. Signed-off-by: Jens Axboe --- fs/io_uring.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index 73d1bd8db1bb..acbf3c7264b5 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -8395,7 +8395,7 @@ static void io_req_cache_free(struct list_head *list, struct task_struct *tsk) } } -static void io_req_caches_free(struct io_ring_ctx *ctx, struct task_struct *tsk) +static void io_req_caches_free(struct io_ring_ctx *ctx) { struct io_submit_state *submit_state = &ctx->submit_state; struct io_comp_state *cs = &ctx->submit_state.comp; @@ -8455,7 +8455,7 @@ static void io_ring_ctx_free(struct io_ring_ctx *ctx) percpu_ref_exit(&ctx->refs); free_uid(ctx->user); - io_req_caches_free(ctx, NULL); + io_req_caches_free(ctx); if (ctx->hash_map) io_wq_put_hash(ctx->hash_map); kfree(ctx->cancel_hash); @@ -8969,7 +8969,7 @@ static int io_uring_flush(struct file *file, void *data) if (fatal_signal_pending(current) || (current->flags & PF_EXITING)) { io_uring_cancel_task_requests(ctx, NULL); - io_req_caches_free(ctx, current); + io_req_caches_free(ctx); } io_run_ctx_fallback(ctx); -- cgit From 1575f21a09206e914b81dace0add693346d97594 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Sat, 27 Feb 2021 15:20:49 -0700 Subject: io_uring: kill unnecessary REQ_F_WORK_INITIALIZED checks We're no longer checking anything that requires the work item to be initialized, as we're not carrying any file related state there. Signed-off-by: Jens Axboe --- fs/io_uring.c | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index acbf3c7264b5..1dd30a15ea6a 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -1080,8 +1080,6 @@ static bool io_match_task(struct io_kiocb *head, return true; io_for_each_link(req, head) { - if (!(req->flags & REQ_F_WORK_INITIALIZED)) - continue; if (req->file && req->file->f_op == &io_uring_fops) return true; if (req->task->files == files) @@ -1800,15 +1798,7 @@ static void io_fail_links(struct io_kiocb *req) trace_io_uring_fail_link(req, link); io_cqring_fill_event(link, -ECANCELED); - /* - * It's ok to free under spinlock as they're not linked anymore, - * but avoid REQ_F_WORK_INITIALIZED because it may deadlock on - * work.fs->lock. - */ - if (link->flags & REQ_F_WORK_INITIALIZED) - io_put_req_deferred(link, 2); - else - io_double_put_req(link); + io_put_req_deferred(link, 2); link = nxt; } io_commit_cqring(ctx); -- cgit From 5730b27e84fdb37353c7cc2b11c24a4f9d73626e Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Sat, 27 Feb 2021 15:57:30 -0700 Subject: io_uring: move cred assignment into io_issue_sqe() If we move it in there, then we no longer have to care about it in io-wq. This means we can drop the cred handling in io-wq, and we can drop the REQ_F_WORK_INITIALIZED flag and async init functions as that was the last user of it since we moved to the new workers. Then we can also drop io_wq_work->creds, and just hold the personality u16 in there instead. Suggested-by: Pavel Begunkov Signed-off-by: Jens Axboe --- fs/io-wq.c | 26 --------------------- fs/io-wq.h | 2 +- fs/io_uring.c | 75 +++++++++++++++++------------------------------------------ 3 files changed, 22 insertions(+), 81 deletions(-) (limited to 'fs') diff --git a/fs/io-wq.c b/fs/io-wq.c index 1407ba74ffc3..946826beefe6 100644 --- a/fs/io-wq.c +++ b/fs/io-wq.c @@ -52,9 +52,6 @@ struct io_worker { struct io_wq_work *cur_work; spinlock_t lock; - const struct cred *cur_creds; - const struct cred *saved_creds; - struct completion ref_done; struct completion started; @@ -180,11 +177,6 @@ static void io_worker_exit(struct io_worker *worker) worker->flags = 0; preempt_enable(); - if (worker->saved_creds) { - revert_creds(worker->saved_creds); - worker->cur_creds = worker->saved_creds = NULL; - } - raw_spin_lock_irq(&wqe->lock); if (flags & IO_WORKER_F_FREE) hlist_nulls_del_rcu(&worker->nulls_node); @@ -326,10 +318,6 @@ static void __io_worker_idle(struct io_wqe *wqe, struct io_worker *worker) worker->flags |= IO_WORKER_F_FREE; hlist_nulls_add_head_rcu(&worker->nulls_node, &wqe->free_list); } - if (worker->saved_creds) { - revert_creds(worker->saved_creds); - worker->cur_creds = worker->saved_creds = NULL; - } } static inline unsigned int io_get_work_hash(struct io_wq_work *work) @@ -404,18 +392,6 @@ static void io_flush_signals(void) } } -static void io_wq_switch_creds(struct io_worker *worker, - struct io_wq_work *work) -{ - const struct cred *old_creds = override_creds(work->creds); - - worker->cur_creds = work->creds; - if (worker->saved_creds) - put_cred(old_creds); /* creds set by previous switch */ - else - worker->saved_creds = old_creds; -} - static void io_assign_current_work(struct io_worker *worker, struct io_wq_work *work) { @@ -465,8 +441,6 @@ get_next: unsigned int hash = io_get_work_hash(work); next_hashed = wq_next_work(work); - if (work->creds && worker->cur_creds != work->creds) - io_wq_switch_creds(worker, work); wq->do_work(work); io_assign_current_work(worker, NULL); diff --git a/fs/io-wq.h b/fs/io-wq.h index f6ef433df8a8..42f0be64a84d 100644 --- a/fs/io-wq.h +++ b/fs/io-wq.h @@ -79,8 +79,8 @@ static inline void wq_list_del(struct io_wq_work_list *list, struct io_wq_work { struct io_wq_work_node list; - const struct cred *creds; unsigned flags; + unsigned short personality; }; static inline struct io_wq_work *wq_next_work(struct io_wq_work *work) diff --git a/fs/io_uring.c b/fs/io_uring.c index 1dd30a15ea6a..d48be0ccc590 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -688,7 +688,6 @@ enum { REQ_F_POLLED_BIT, REQ_F_BUFFER_SELECTED_BIT, REQ_F_NO_FILE_TABLE_BIT, - REQ_F_WORK_INITIALIZED_BIT, REQ_F_LTIMEOUT_ACTIVE_BIT, REQ_F_COMPLETE_INLINE_BIT, @@ -730,8 +729,6 @@ enum { REQ_F_BUFFER_SELECTED = BIT(REQ_F_BUFFER_SELECTED_BIT), /* doesn't need file table for this request */ REQ_F_NO_FILE_TABLE = BIT(REQ_F_NO_FILE_TABLE_BIT), - /* io_wq_work is initialized */ - REQ_F_WORK_INITIALIZED = BIT(REQ_F_WORK_INITIALIZED_BIT), /* linked timeout is active, i.e. prepared by link's head */ REQ_F_LTIMEOUT_ACTIVE = BIT(REQ_F_LTIMEOUT_ACTIVE_BIT), /* completion is deferred through io_comp_state */ @@ -1094,24 +1091,6 @@ static inline void req_set_fail_links(struct io_kiocb *req) req->flags |= REQ_F_FAIL_LINK; } -static inline void __io_req_init_async(struct io_kiocb *req) -{ - memset(&req->work, 0, sizeof(req->work)); - req->flags |= REQ_F_WORK_INITIALIZED; -} - -/* - * Note: must call io_req_init_async() for the first time you - * touch any members of io_wq_work. - */ -static inline void io_req_init_async(struct io_kiocb *req) -{ - if (req->flags & REQ_F_WORK_INITIALIZED) - return; - - __io_req_init_async(req); -} - static void io_ring_ctx_ref_free(struct percpu_ref *ref) { struct io_ring_ctx *ctx = container_of(ref, struct io_ring_ctx, refs); @@ -1196,13 +1175,6 @@ static bool req_need_defer(struct io_kiocb *req, u32 seq) static void io_req_clean_work(struct io_kiocb *req) { - if (!(req->flags & REQ_F_WORK_INITIALIZED)) - return; - - if (req->work.creds) { - put_cred(req->work.creds); - req->work.creds = NULL; - } if (req->flags & REQ_F_INFLIGHT) { struct io_ring_ctx *ctx = req->ctx; struct io_uring_task *tctx = req->task->io_uring; @@ -1215,8 +1187,6 @@ static void io_req_clean_work(struct io_kiocb *req) if (atomic_read(&tctx->in_idle)) wake_up(&tctx->wait); } - - req->flags &= ~REQ_F_WORK_INITIALIZED; } static void io_req_track_inflight(struct io_kiocb *req) @@ -1224,7 +1194,6 @@ static void io_req_track_inflight(struct io_kiocb *req) struct io_ring_ctx *ctx = req->ctx; if (!(req->flags & REQ_F_INFLIGHT)) { - io_req_init_async(req); req->flags |= REQ_F_INFLIGHT; spin_lock_irq(&ctx->inflight_lock); @@ -1238,8 +1207,6 @@ static void io_prep_async_work(struct io_kiocb *req) const struct io_op_def *def = &io_op_defs[req->opcode]; struct io_ring_ctx *ctx = req->ctx; - io_req_init_async(req); - if (req->flags & REQ_F_FORCE_ASYNC) req->work.flags |= IO_WQ_WORK_CONCURRENT; @@ -1250,8 +1217,6 @@ static void io_prep_async_work(struct io_kiocb *req) if (def->unbound_nonreg_file) req->work.flags |= IO_WQ_WORK_UNBOUND; } - if (!req->work.creds) - req->work.creds = get_current_cred(); } static void io_prep_async_link(struct io_kiocb *req) @@ -3578,7 +3543,6 @@ static int __io_splice_prep(struct io_kiocb *req, * Splice operation will be punted aync, and here need to * modify io_wq_work.flags, so initialize io_wq_work firstly. */ - io_req_init_async(req); req->work.flags |= IO_WQ_WORK_UNBOUND; } @@ -5935,8 +5899,22 @@ static void __io_clean_op(struct io_kiocb *req) static int io_issue_sqe(struct io_kiocb *req, unsigned int issue_flags) { struct io_ring_ctx *ctx = req->ctx; + const struct cred *creds = NULL; int ret; + if (req->work.personality) { + const struct cred *new_creds; + + if (!(issue_flags & IO_URING_F_NONBLOCK)) + mutex_lock(&ctx->uring_lock); + new_creds = idr_find(&ctx->personality_idr, req->work.personality); + if (!(issue_flags & IO_URING_F_NONBLOCK)) + mutex_unlock(&ctx->uring_lock); + if (!new_creds) + return -EINVAL; + creds = override_creds(new_creds); + } + switch (req->opcode) { case IORING_OP_NOP: ret = io_nop(req, issue_flags); @@ -6043,6 +6021,9 @@ static int io_issue_sqe(struct io_kiocb *req, unsigned int issue_flags) break; } + if (creds) + revert_creds(creds); + if (ret) return ret; @@ -6206,18 +6187,10 @@ static struct io_kiocb *io_prep_linked_timeout(struct io_kiocb *req) static void __io_queue_sqe(struct io_kiocb *req) { struct io_kiocb *linked_timeout = io_prep_linked_timeout(req); - const struct cred *old_creds = NULL; int ret; - if ((req->flags & REQ_F_WORK_INITIALIZED) && req->work.creds && - req->work.creds != current_cred()) - old_creds = override_creds(req->work.creds); - ret = io_issue_sqe(req, IO_URING_F_NONBLOCK|IO_URING_F_COMPLETE_DEFER); - if (old_creds) - revert_creds(old_creds); - /* * We async punt it if the file wasn't marked NOWAIT, or if the file * doesn't support non-blocking read/write attempts @@ -6304,7 +6277,7 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req, { struct io_submit_state *state; unsigned int sqe_flags; - int id, ret = 0; + int ret = 0; req->opcode = READ_ONCE(sqe->opcode); /* same numerical values with corresponding REQ_F_*, safe to copy */ @@ -6336,15 +6309,9 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req, !io_op_defs[req->opcode].buffer_select) return -EOPNOTSUPP; - id = READ_ONCE(sqe->personality); - if (id) { - __io_req_init_async(req); - req->work.creds = idr_find(&ctx->personality_idr, id); - if (unlikely(!req->work.creds)) - return -EINVAL; - get_cred(req->work.creds); - } - + req->work.list.next = NULL; + req->work.flags = 0; + req->work.personality = READ_ONCE(sqe->personality); state = &ctx->submit_state; /* -- cgit From 914390bcfdd6351a4d308da7f43294476ea7d3bf Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Sun, 28 Feb 2021 12:22:37 -0700 Subject: io_uring: kill unnecessary io_run_ctx_fallback() in io_ring_exit_work() We already run the fallback task_work in io_uring_try_cancel_requests(), no need to duplicate at ring exit explicitly. Signed-off-by: Jens Axboe --- fs/io_uring.c | 1 - 1 file changed, 1 deletion(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index d48be0ccc590..7cad82b51eca 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -8519,7 +8519,6 @@ static void io_ring_exit_work(struct work_struct *work) */ do { io_uring_try_cancel_requests(ctx, NULL, NULL); - io_run_ctx_fallback(ctx); } while (!wait_for_completion_timeout(&ctx->ref_comp, HZ/20)); io_ring_ctx_free(ctx); } -- cgit From 0d30b3e7eea94cc818fadf2ac0dd189c616028f8 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Sun, 28 Feb 2021 12:23:27 -0700 Subject: io_uring: kill io_uring_flush() This was always a weird work-around or file referencing, and we don't need it anymore. Get rid of it. Signed-off-by: Jens Axboe --- fs/io_uring.c | 47 ----------------------------------------------- 1 file changed, 47 deletions(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index 7cad82b51eca..766df21769a8 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -8914,52 +8914,6 @@ void __io_uring_task_cancel(void) __io_uring_free(current); } -static int io_uring_flush(struct file *file, void *data) -{ - struct io_uring_task *tctx = current->io_uring; - struct io_ring_ctx *ctx = file->private_data; - - /* Ignore helper thread files exit */ - if (current->flags & PF_IO_WORKER) - return 0; - - if (fatal_signal_pending(current) || (current->flags & PF_EXITING)) { - io_uring_cancel_task_requests(ctx, NULL); - io_req_caches_free(ctx); - } - - io_run_ctx_fallback(ctx); - - if (!tctx) - return 0; - - /* we should have cancelled and erased it before PF_EXITING */ - WARN_ON_ONCE((current->flags & PF_EXITING) && - xa_load(&tctx->xa, (unsigned long)file)); - - /* - * fput() is pending, will be 2 if the only other ref is our potential - * task file note. If the task is exiting, drop regardless of count. - */ - if (atomic_long_read(&file->f_count) != 2) - return 0; - - if (ctx->flags & IORING_SETUP_SQPOLL) { - /* there is only one file note, which is owned by sqo_task */ - WARN_ON_ONCE(ctx->sqo_task != current && - xa_load(&tctx->xa, (unsigned long)file)); - /* sqo_dead check is for when this happens after cancellation */ - WARN_ON_ONCE(ctx->sqo_task == current && !ctx->sqo_dead && - !xa_load(&tctx->xa, (unsigned long)file)); - - io_disable_sqo_submit(ctx); - } - - if (!(ctx->flags & IORING_SETUP_SQPOLL) || ctx->sqo_task == current) - io_uring_del_task_file(file); - return 0; -} - static void *io_uring_validate_mmap_request(struct file *file, loff_t pgoff, size_t sz) { @@ -9291,7 +9245,6 @@ static void io_uring_show_fdinfo(struct seq_file *m, struct file *f) static const struct file_operations io_uring_fops = { .release = io_uring_release, - .flush = io_uring_flush, .mmap = io_uring_mmap, #ifndef CONFIG_MMU .get_unmapped_area = io_uring_nommu_get_unmapped_area, -- cgit From 2c32395d8111037ae2cb8cab883e80bcdbb70713 Mon Sep 17 00:00:00 2001 From: Pavel Begunkov Date: Sun, 28 Feb 2021 22:04:53 +0000 Subject: io_uring: fix __tctx_task_work() ctx race There is an unlikely but possible race using a freed context. That's because req->task_work.func() can free a request, but we won't necessarily find a completion in submit_state.comp and so all ctx refs may be put by the time we do mutex_lock(&ctx->uring_ctx); There are several reasons why it can miss going through submit_state.comp: 1) req->task_work.func() didn't complete it itself, but punted to iowq (e.g. reissue) and it got freed later, or a similar situation with it overflowing and getting flushed by someone else, or being submitted to IRQ completion, 2) As we don't hold the uring_lock, someone else can do io_submit_flush_completions() and put our ref. 3) Bugs and code obscurities, e.g. failing to propagate issue_flags properly. One example is as follows CPU1 | CPU2 ======================================================================= @req->task_work.func() | -> @req overflwed, | so submit_state.comp,nr==0 | | flush overflows, and free @req | ctx refs == 0, free it ctx is dead, but we do | lock + flush + unlock | So take a ctx reference for each new ctx we see in __tctx_task_work(), and do release it until we do all our flushing. Fixes: 65453d1efbd2 ("io_uring: enable req cache for task_work items") Reported-by: syzbot+a157ac7c03a56397f553@syzkaller.appspotmail.com Signed-off-by: Pavel Begunkov [axboe: fold in my one-liner and fix ref mismatch] Signed-off-by: Jens Axboe --- fs/io_uring.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index 766df21769a8..62a73543ab86 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -1800,6 +1800,18 @@ static inline struct io_kiocb *io_req_find_next(struct io_kiocb *req) return __io_req_find_next(req); } +static void ctx_flush_and_put(struct io_ring_ctx *ctx) +{ + if (!ctx) + return; + if (ctx->submit_state.comp.nr) { + mutex_lock(&ctx->uring_lock); + io_submit_flush_completions(&ctx->submit_state.comp, ctx); + mutex_unlock(&ctx->uring_lock); + } + percpu_ref_put(&ctx->refs); +} + static bool __tctx_task_work(struct io_uring_task *tctx) { struct io_ring_ctx *ctx = NULL; @@ -1817,30 +1829,20 @@ static bool __tctx_task_work(struct io_uring_task *tctx) node = list.first; while (node) { struct io_wq_work_node *next = node->next; - struct io_ring_ctx *this_ctx; struct io_kiocb *req; req = container_of(node, struct io_kiocb, io_task_work.node); - this_ctx = req->ctx; - req->task_work.func(&req->task_work); - node = next; - - if (!ctx) { - ctx = this_ctx; - } else if (ctx != this_ctx) { - mutex_lock(&ctx->uring_lock); - io_submit_flush_completions(&ctx->submit_state.comp, ctx); - mutex_unlock(&ctx->uring_lock); - ctx = this_ctx; + if (req->ctx != ctx) { + ctx_flush_and_put(ctx); + ctx = req->ctx; + percpu_ref_get(&ctx->refs); } - } - if (ctx && ctx->submit_state.comp.nr) { - mutex_lock(&ctx->uring_lock); - io_submit_flush_completions(&ctx->submit_state.comp, ctx); - mutex_unlock(&ctx->uring_lock); + req->task_work.func(&req->task_work); + node = next; } + ctx_flush_and_put(ctx); return list.first != NULL; } -- cgit From 28c4721b80a702462fb77373c23428ee698fa5dd Mon Sep 17 00:00:00 2001 From: Pavel Begunkov Date: Sun, 28 Feb 2021 22:04:54 +0000 Subject: io_uring: replace cmpxchg in fallback with xchg io_run_ctx_fallback() can use xchg() instead of cmpxchg(). It's simpler and faster. Signed-off-by: Pavel Begunkov Signed-off-by: Jens Axboe --- fs/io_uring.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index 62a73543ab86..57c7833ba62b 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -8484,15 +8484,11 @@ static int io_remove_personalities(int id, void *p, void *data) static bool io_run_ctx_fallback(struct io_ring_ctx *ctx) { - struct callback_head *work, *head, *next; + struct callback_head *work, *next; bool executed = false; do { - do { - head = NULL; - work = READ_ONCE(ctx->exit_task_work); - } while (cmpxchg(&ctx->exit_task_work, work, head) != work); - + work = xchg(&ctx->exit_task_work, NULL); if (!work) break; -- cgit From 3ebba796fa251d042be42b929a2d916ee5c34a49 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Sun, 28 Feb 2021 15:32:18 -0700 Subject: io_uring: ensure that SQPOLL thread is started for exit If we create it in a disabled state because IORING_SETUP_R_DISABLED is set on ring creation, we need to ensure that we've kicked the thread if we're exiting before it's been explicitly disabled. Otherwise we can run into a deadlock where exit is waiting go park the SQPOLL thread, but the SQPOLL thread itself is waiting to get a signal to start. That results in the below trace of both tasks hung, waiting on each other: INFO: task syz-executor458:8401 blocked for more than 143 seconds. Not tainted 5.11.0-next-20210226-syzkaller #0 "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. task:syz-executor458 state:D stack:27536 pid: 8401 ppid: 8400 flags:0x00004004 Call Trace: context_switch kernel/sched/core.c:4324 [inline] __schedule+0x90c/0x21a0 kernel/sched/core.c:5075 schedule+0xcf/0x270 kernel/sched/core.c:5154 schedule_timeout+0x1db/0x250 kernel/time/timer.c:1868 do_wait_for_common kernel/sched/completion.c:85 [inline] __wait_for_common kernel/sched/completion.c:106 [inline] wait_for_common kernel/sched/completion.c:117 [inline] wait_for_completion+0x168/0x270 kernel/sched/completion.c:138 io_sq_thread_park fs/io_uring.c:7115 [inline] io_sq_thread_park+0xd5/0x130 fs/io_uring.c:7103 io_uring_cancel_task_requests+0x24c/0xd90 fs/io_uring.c:8745 __io_uring_files_cancel+0x110/0x230 fs/io_uring.c:8840 io_uring_files_cancel include/linux/io_uring.h:47 [inline] do_exit+0x299/0x2a60 kernel/exit.c:780 do_group_exit+0x125/0x310 kernel/exit.c:922 __do_sys_exit_group kernel/exit.c:933 [inline] __se_sys_exit_group kernel/exit.c:931 [inline] __x64_sys_exit_group+0x3a/0x50 kernel/exit.c:931 do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46 entry_SYSCALL_64_after_hwframe+0x44/0xae RIP: 0033:0x43e899 RSP: 002b:00007ffe89376d48 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7 RAX: ffffffffffffffda RBX: 00000000004af2f0 RCX: 000000000043e899 RDX: 000000000000003c RSI: 00000000000000e7 RDI: 0000000000000000 RBP: 0000000000000000 R08: ffffffffffffffc0 R09: 0000000010000000 R10: 0000000000008011 R11: 0000000000000246 R12: 00000000004af2f0 R13: 0000000000000001 R14: 0000000000000000 R15: 0000000000000001 INFO: task iou-sqp-8401:8402 can't die for more than 143 seconds. task:iou-sqp-8401 state:D stack:30272 pid: 8402 ppid: 8400 flags:0x00004004 Call Trace: context_switch kernel/sched/core.c:4324 [inline] __schedule+0x90c/0x21a0 kernel/sched/core.c:5075 schedule+0xcf/0x270 kernel/sched/core.c:5154 schedule_timeout+0x1db/0x250 kernel/time/timer.c:1868 do_wait_for_common kernel/sched/completion.c:85 [inline] __wait_for_common kernel/sched/completion.c:106 [inline] wait_for_common kernel/sched/completion.c:117 [inline] wait_for_completion+0x168/0x270 kernel/sched/completion.c:138 io_sq_thread+0x27d/0x1ae0 fs/io_uring.c:6717 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294 INFO: task iou-sqp-8401:8402 blocked for more than 143 seconds. Reported-by: syzbot+fb5458330b4442f2090d@syzkaller.appspotmail.com Signed-off-by: Jens Axboe --- fs/io_uring.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index 57c7833ba62b..94b1a0f48fed 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -7916,6 +7916,7 @@ static void io_sq_offload_start(struct io_ring_ctx *ctx) { struct io_sq_data *sqd = ctx->sq_data; + ctx->flags &= ~IORING_SETUP_R_DISABLED; if (ctx->flags & IORING_SETUP_SQPOLL) complete(&sqd->startup); } @@ -8692,6 +8693,8 @@ static void io_disable_sqo_submit(struct io_ring_ctx *ctx) { mutex_lock(&ctx->uring_lock); ctx->sqo_dead = 1; + if (ctx->flags & IORING_SETUP_R_DISABLED) + io_sq_offload_start(ctx); mutex_unlock(&ctx->uring_lock); /* make sure callers enter the ring to get error */ @@ -9637,10 +9640,7 @@ static int io_register_enable_rings(struct io_ring_ctx *ctx) if (ctx->restrictions.registered) ctx->restricted = 1; - ctx->flags &= ~IORING_SETUP_R_DISABLED; - io_sq_offload_start(ctx); - return 0; } -- cgit From 1c3b3e6527e57156bf4082f11c2151957560fe6a Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Sun, 28 Feb 2021 16:07:30 -0700 Subject: io_uring: ignore double poll add on the same waitqueue head syzbot reports a deadlock, attempting to lock the same spinlock twice: ============================================ WARNING: possible recursive locking detected 5.11.0-syzkaller #0 Not tainted -------------------------------------------- swapper/1/0 is trying to acquire lock: ffff88801b2b1130 (&runtime->sleep){..-.}-{2:2}, at: spin_lock include/linux/spinlock.h:354 [inline] ffff88801b2b1130 (&runtime->sleep){..-.}-{2:2}, at: io_poll_double_wake+0x25f/0x6a0 fs/io_uring.c:4960 but task is already holding lock: ffff88801b2b3130 (&runtime->sleep){..-.}-{2:2}, at: __wake_up_common_lock+0xb4/0x130 kernel/sched/wait.c:137 other info that might help us debug this: Possible unsafe locking scenario: CPU0 ---- lock(&runtime->sleep); lock(&runtime->sleep); *** DEADLOCK *** May be due to missing lock nesting notation 2 locks held by swapper/1/0: #0: ffff888147474908 (&group->lock){..-.}-{2:2}, at: _snd_pcm_stream_lock_irqsave+0x9f/0xd0 sound/core/pcm_native.c:170 #1: ffff88801b2b3130 (&runtime->sleep){..-.}-{2:2}, at: __wake_up_common_lock+0xb4/0x130 kernel/sched/wait.c:137 stack backtrace: CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.11.0-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:79 [inline] dump_stack+0xfa/0x151 lib/dump_stack.c:120 print_deadlock_bug kernel/locking/lockdep.c:2829 [inline] check_deadlock kernel/locking/lockdep.c:2872 [inline] validate_chain kernel/locking/lockdep.c:3661 [inline] __lock_acquire.cold+0x14c/0x3b4 kernel/locking/lockdep.c:4900 lock_acquire kernel/locking/lockdep.c:5510 [inline] lock_acquire+0x1ab/0x730 kernel/locking/lockdep.c:5475 __raw_spin_lock include/linux/spinlock_api_smp.h:142 [inline] _raw_spin_lock+0x2a/0x40 kernel/locking/spinlock.c:151 spin_lock include/linux/spinlock.h:354 [inline] io_poll_double_wake+0x25f/0x6a0 fs/io_uring.c:4960 __wake_up_common+0x147/0x650 kernel/sched/wait.c:108 __wake_up_common_lock+0xd0/0x130 kernel/sched/wait.c:138 snd_pcm_update_state+0x46a/0x540 sound/core/pcm_lib.c:203 snd_pcm_update_hw_ptr0+0xa75/0x1a50 sound/core/pcm_lib.c:464 snd_pcm_period_elapsed+0x160/0x250 sound/core/pcm_lib.c:1805 dummy_hrtimer_callback+0x94/0x1b0 sound/drivers/dummy.c:378 __run_hrtimer kernel/time/hrtimer.c:1519 [inline] __hrtimer_run_queues+0x609/0xe40 kernel/time/hrtimer.c:1583 hrtimer_run_softirq+0x17b/0x360 kernel/time/hrtimer.c:1600 __do_softirq+0x29b/0x9f6 kernel/softirq.c:345 invoke_softirq kernel/softirq.c:221 [inline] __irq_exit_rcu kernel/softirq.c:422 [inline] irq_exit_rcu+0x134/0x200 kernel/softirq.c:434 sysvec_apic_timer_interrupt+0x93/0xc0 arch/x86/kernel/apic/apic.c:1100 asm_sysvec_apic_timer_interrupt+0x12/0x20 arch/x86/include/asm/idtentry.h:632 RIP: 0010:native_save_fl arch/x86/include/asm/irqflags.h:29 [inline] RIP: 0010:arch_local_save_flags arch/x86/include/asm/irqflags.h:70 [inline] RIP: 0010:arch_irqs_disabled arch/x86/include/asm/irqflags.h:137 [inline] RIP: 0010:acpi_safe_halt drivers/acpi/processor_idle.c:111 [inline] RIP: 0010:acpi_idle_do_entry+0x1c9/0x250 drivers/acpi/processor_idle.c:516 Code: dd 38 6e f8 84 db 75 ac e8 54 32 6e f8 e8 0f 1c 74 f8 e9 0c 00 00 00 e8 45 32 6e f8 0f 00 2d 4e 4a c5 00 e8 39 32 6e f8 fb f4 <9c> 5b 81 e3 00 02 00 00 fa 31 ff 48 89 de e8 14 3a 6e f8 48 85 db RSP: 0018:ffffc90000d47d18 EFLAGS: 00000293 RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 RDX: ffff8880115c3780 RSI: ffffffff89052537 RDI: 0000000000000000 RBP: ffff888141127064 R08: 0000000000000001 R09: 0000000000000001 R10: ffffffff81794168 R11: 0000000000000000 R12: 0000000000000001 R13: ffff888141127000 R14: ffff888141127064 R15: ffff888143331804 acpi_idle_enter+0x361/0x500 drivers/acpi/processor_idle.c:647 cpuidle_enter_state+0x1b1/0xc80 drivers/cpuidle/cpuidle.c:237 cpuidle_enter+0x4a/0xa0 drivers/cpuidle/cpuidle.c:351 call_cpuidle kernel/sched/idle.c:158 [inline] cpuidle_idle_call kernel/sched/idle.c:239 [inline] do_idle+0x3e1/0x590 kernel/sched/idle.c:300 cpu_startup_entry+0x14/0x20 kernel/sched/idle.c:397 start_secondary+0x274/0x350 arch/x86/kernel/smpboot.c:272 secondary_startup_64_no_verify+0xb0/0xbb which is due to the driver doing poll_wait() twice on the same wait_queue_head. That is perfectly valid, but from checking the rest of the kernel tree, it's the only driver that does this. We can handle this just fine, we just need to ignore the second addition as we'll get woken just fine on the first one. Cc: stable@vger.kernel.org # 5.8+ Fixes: 18bceab101ad ("io_uring: allow POLL_ADD with double poll_wait() users") Reported-by: syzbot+28abd693db9e92c160d8@syzkaller.appspotmail.com Signed-off-by: Jens Axboe --- fs/io_uring.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index 94b1a0f48fed..eb4bc8967178 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -4959,6 +4959,9 @@ static void __io_queue_proc(struct io_poll_iocb *poll, struct io_poll_table *pt, pt->error = -EINVAL; return; } + /* double add on the same waitqueue head, ignore */ + if (poll->head == head) + return; poll = kmalloc(sizeof(*poll), GFP_ATOMIC); if (!poll) { pt->error = -ENOMEM; -- cgit From 70aacfe66136809d7f080f89c492c278298719f4 Mon Sep 17 00:00:00 2001 From: Pavel Begunkov Date: Mon, 1 Mar 2021 13:02:15 +0000 Subject: io_uring: kill sqo_dead and sqo submission halting As SQPOLL task doesn't poke into ->sqo_task anymore, there is no need to kill the sqo when the master task exits. Before it was necessary to avoid races accessing sqo_task->files with removing them. Signed-off-by: Pavel Begunkov [axboe: don't forget to enable SQPOLL before exit, if started disabled] Signed-off-by: Jens Axboe --- fs/io_uring.c | 45 ++++++++------------------------------------- 1 file changed, 8 insertions(+), 37 deletions(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index eb4bc8967178..6090a380e903 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -338,7 +338,6 @@ struct io_ring_ctx { unsigned int drain_next: 1; unsigned int eventfd_async: 1; unsigned int restricted: 1; - unsigned int sqo_dead: 1; unsigned int sqo_exec: 1; /* @@ -1967,7 +1966,7 @@ static void __io_req_task_submit(struct io_kiocb *req) /* ctx stays valid until unlock, even if we drop all ours ctx->refs */ mutex_lock(&ctx->uring_lock); - if (!ctx->sqo_dead && !(current->flags & PF_EXITING) && !current->in_execve) + if (!(current->flags & PF_EXITING) && !current->in_execve) __io_queue_sqe(req); else __io_req_task_cancel(req, -EFAULT); @@ -6578,8 +6577,7 @@ static int __io_sq_thread(struct io_ring_ctx *ctx, bool cap_entries) if (!list_empty(&ctx->iopoll_list)) io_do_iopoll(ctx, &nr_events, 0); - if (to_submit && !ctx->sqo_dead && - likely(!percpu_ref_is_dying(&ctx->refs))) + if (to_submit && likely(!percpu_ref_is_dying(&ctx->refs))) ret = io_submit_sqes(ctx, to_submit); mutex_unlock(&ctx->uring_lock); } @@ -7818,7 +7816,7 @@ static int io_sq_thread_fork(struct io_sq_data *sqd, struct io_ring_ctx *ctx) clear_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state); reinit_completion(&sqd->completion); - ctx->sqo_dead = ctx->sqo_exec = 0; + ctx->sqo_exec = 0; sqd->task_pid = current->pid; current->flags |= PF_IO_WORKER; ret = io_wq_fork_thread(io_sq_thread, sqd); @@ -8529,10 +8527,6 @@ static void io_ring_ctx_wait_and_kill(struct io_ring_ctx *ctx) { mutex_lock(&ctx->uring_lock); percpu_ref_kill(&ctx->refs); - - if (WARN_ON_ONCE((ctx->flags & IORING_SETUP_SQPOLL) && !ctx->sqo_dead)) - ctx->sqo_dead = 1; - /* if force is set, the ring is going away. always drop after that */ ctx->cq_overflow_flushed = 1; if (ctx->rings) @@ -8692,19 +8686,6 @@ static void io_uring_cancel_files(struct io_ring_ctx *ctx, } } -static void io_disable_sqo_submit(struct io_ring_ctx *ctx) -{ - mutex_lock(&ctx->uring_lock); - ctx->sqo_dead = 1; - if (ctx->flags & IORING_SETUP_R_DISABLED) - io_sq_offload_start(ctx); - mutex_unlock(&ctx->uring_lock); - - /* make sure callers enter the ring to get error */ - if (ctx->rings) - io_ring_set_wakeup_flag(ctx); -} - /* * We need to iteratively cancel requests, in case a request has dependent * hard links. These persist even for failure of cancelations, hence keep @@ -8717,7 +8698,11 @@ static void io_uring_cancel_task_requests(struct io_ring_ctx *ctx, bool did_park = false; if ((ctx->flags & IORING_SETUP_SQPOLL) && ctx->sq_data) { - io_disable_sqo_submit(ctx); + /* never started, nothing to cancel */ + if (ctx->flags & IORING_SETUP_R_DISABLED) { + io_sq_offload_start(ctx); + return; + } did_park = io_sq_thread_park(ctx->sq_data); if (did_park) { task = ctx->sq_data->thread; @@ -8838,7 +8823,6 @@ static void io_uring_cancel_sqpoll(struct io_ring_ctx *ctx) if (!sqd) return; - io_disable_sqo_submit(ctx); if (!io_sq_thread_park(sqd)) return; tctx = ctx->sq_data->thread->io_uring; @@ -8883,7 +8867,6 @@ void __io_uring_task_cancel(void) /* make sure overflow events are dropped */ atomic_inc(&tctx->in_idle); - /* trigger io_disable_sqo_submit() */ if (tctx->sqpoll) { struct file *file; unsigned long index; @@ -8996,22 +8979,14 @@ static int io_sqpoll_wait_sq(struct io_ring_ctx *ctx) do { if (!io_sqring_full(ctx)) break; - prepare_to_wait(&ctx->sqo_sq_wait, &wait, TASK_INTERRUPTIBLE); - if (unlikely(ctx->sqo_dead)) { - ret = -EOWNERDEAD; - goto out; - } - if (!io_sqring_full(ctx)) break; - schedule(); } while (!signal_pending(current)); finish_wait(&ctx->sqo_sq_wait, &wait); -out: return ret; } @@ -9093,8 +9068,6 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit, ctx->sqo_exec = 0; } ret = -EOWNERDEAD; - if (unlikely(ctx->sqo_dead)) - goto out; if (flags & IORING_ENTER_SQ_WAKEUP) wake_up(&ctx->sq_data->wait); if (flags & IORING_ENTER_SQ_WAIT) { @@ -9466,7 +9439,6 @@ static int io_uring_create(unsigned entries, struct io_uring_params *p, */ ret = io_uring_install_fd(ctx, file); if (ret < 0) { - io_disable_sqo_submit(ctx); /* fput will clean it up */ fput(file); return ret; @@ -9475,7 +9447,6 @@ static int io_uring_create(unsigned entries, struct io_uring_params *p, trace_io_uring_create(ret, ctx, p->sq_entries, p->cq_entries, p->flags); return ret; err: - io_disable_sqo_submit(ctx); io_ring_ctx_wait_and_kill(ctx); return ret; } -- cgit From 16270893d71219816513a255e6c3163bc7224ce4 Mon Sep 17 00:00:00 2001 From: Pavel Begunkov Date: Mon, 1 Mar 2021 13:02:16 +0000 Subject: io_uring: remove sqo_task Now, sqo_task is used only for a warning that is not interesting anymore since sqo_dead is gone, remove all of that including ctx->sqo_task. Signed-off-by: Pavel Begunkov Signed-off-by: Jens Axboe --- fs/io_uring.c | 10 ---------- 1 file changed, 10 deletions(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index 6090a380e903..f060dcc1cc86 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -379,11 +379,6 @@ struct io_ring_ctx { struct io_rings *rings; - /* - * For SQPOLL usage - */ - struct task_struct *sqo_task; - /* Only used for accounting purposes */ struct mm_struct *mm_account; @@ -8747,10 +8742,6 @@ static int io_uring_add_task_file(struct io_ring_ctx *ctx, struct file *file) fput(file); return ret; } - - /* one and only SQPOLL file note, held by sqo_task */ - WARN_ON_ONCE((ctx->flags & IORING_SETUP_SQPOLL) && - current != ctx->sqo_task); } tctx->last = file; } @@ -9376,7 +9367,6 @@ static int io_uring_create(unsigned entries, struct io_uring_params *p, ctx->compat = in_compat_syscall(); if (!capable(CAP_IPC_LOCK)) ctx->user = get_uid(current_user()); - ctx->sqo_task = current; /* * This is just grabbed for accounting purposes. When a process exits, -- cgit From dc7bbc9ef361bea331bf5258a35abcdef619d44d Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Mon, 1 Mar 2021 09:09:56 -0700 Subject: io-wq: fix error path leak of buffered write hash map The 'err' path should include the hash put, we already grabbed a reference once we get that far. Fixes: e941894eae31 ("io-wq: make buffered file write hashed work map per-ctx") Signed-off-by: Jens Axboe --- fs/io-wq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs') diff --git a/fs/io-wq.c b/fs/io-wq.c index 946826beefe6..dc430381b694 100644 --- a/fs/io-wq.c +++ b/fs/io-wq.c @@ -1047,8 +1047,8 @@ struct io_wq *io_wq_create(unsigned bounded, struct io_wq_data *data) if (!ret) return wq; - io_wq_put_hash(data->hash); err: + io_wq_put_hash(data->hash); cpuhp_state_remove_instance_nocalls(io_wq_online, &wq->cpuhp_node); for_each_node(node) kfree(wq->wqes[node]); -- cgit From 3e6a0d3c7571ce3ed0d25c5c32543a54a7ebcd75 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Mon, 1 Mar 2021 13:56:00 -0700 Subject: io_uring: fix -EAGAIN retry with IOPOLL We no longer revert the iovec on -EIOCBQUEUED, see commit ab2125df921d, and this started causing issues for IOPOLL on devies that run out of request slots. Turns out what outside of needing a revert for those, we also had a bug where we didn't properly setup retry inside the submission path. That could cause re-import of the iovec, if any, and that could lead to spurious results if the application had those allocated on the stack. Catch -EAGAIN retry and make the iovec stable for IOPOLL, just like we do for !IOPOLL retries. Cc: # 5.9+ Reported-by: Abaci Robot Reported-by: Xiaoguang Wang Signed-off-by: Jens Axboe --- fs/io_uring.c | 36 +++++++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index f060dcc1cc86..361befaae28f 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -2423,23 +2423,32 @@ static bool io_resubmit_prep(struct io_kiocb *req) return false; return !io_setup_async_rw(req, iovec, inline_vecs, &iter, false); } -#endif -static bool io_rw_reissue(struct io_kiocb *req) +static bool io_rw_should_reissue(struct io_kiocb *req) { -#ifdef CONFIG_BLOCK umode_t mode = file_inode(req->file)->i_mode; + struct io_ring_ctx *ctx = req->ctx; if (!S_ISBLK(mode) && !S_ISREG(mode)) return false; - if ((req->flags & REQ_F_NOWAIT) || io_wq_current_is_worker()) + if ((req->flags & REQ_F_NOWAIT) || (io_wq_current_is_worker() && + !(ctx->flags & IORING_SETUP_IOPOLL))) return false; /* * If ref is dying, we might be running poll reap from the exit work. * Don't attempt to reissue from that path, just let it fail with * -EAGAIN. */ - if (percpu_ref_is_dying(&req->ctx->refs)) + if (percpu_ref_is_dying(&ctx->refs)) + return false; + return true; +} +#endif + +static bool io_rw_reissue(struct io_kiocb *req) +{ +#ifdef CONFIG_BLOCK + if (!io_rw_should_reissue(req)) return false; lockdep_assert_held(&req->ctx->uring_lock); @@ -2482,6 +2491,19 @@ static void io_complete_rw_iopoll(struct kiocb *kiocb, long res, long res2) { struct io_kiocb *req = container_of(kiocb, struct io_kiocb, rw.kiocb); +#ifdef CONFIG_BLOCK + /* Rewind iter, if we have one. iopoll path resubmits as usual */ + if (res == -EAGAIN && io_rw_should_reissue(req)) { + struct io_async_rw *rw = req->async_data; + + if (rw) + iov_iter_revert(&rw->iter, + req->result - iov_iter_count(&rw->iter)); + else if (!io_resubmit_prep(req)) + res = -EIO; + } +#endif + if (kiocb->ki_flags & IOCB_WRITE) kiocb_end_write(req); @@ -3230,6 +3252,8 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags) ret = io_iter_do_read(req, iter); if (ret == -EIOCBQUEUED) { + if (req->async_data) + iov_iter_revert(iter, io_size - iov_iter_count(iter)); goto out_free; } else if (ret == -EAGAIN) { /* IOPOLL retry should happen for io-wq threads */ @@ -3361,6 +3385,8 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags) /* no retry on NONBLOCK nor RWF_NOWAIT */ if (ret2 == -EAGAIN && (req->flags & REQ_F_NOWAIT)) goto done; + if (ret2 == -EIOCBQUEUED && req->async_data) + iov_iter_revert(iter, io_size - iov_iter_count(iter)); if (!force_nonblock || ret2 != -EAGAIN) { /* IOPOLL retry should happen for io-wq threads */ if ((req->ctx->flags & IORING_SETUP_IOPOLL) && ret2 == -EAGAIN) -- cgit From 64c7212391e778949aa3055fb3863439417ddba9 Mon Sep 17 00:00:00 2001 From: Pavel Begunkov Date: Mon, 1 Mar 2021 18:20:45 +0000 Subject: io_uring: choose right tctx->io_wq for try cancel When we cancel SQPOLL, @task in io_uring_try_cancel_requests() will differ from current. Use the right tctx from passed in @task, and don't forget that it can be NULL when the io_uring ctx exits. Signed-off-by: Pavel Begunkov Signed-off-by: Jens Axboe --- fs/io_uring.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index 361befaae28f..d6c5465b372b 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -8636,7 +8636,8 @@ static void io_uring_try_cancel_requests(struct io_ring_ctx *ctx, struct files_struct *files) { struct io_task_cancel cancel = { .task = task, .files = files, }; - struct io_uring_task *tctx = current->io_uring; + struct task_struct *tctx_task = task ?: current; + struct io_uring_task *tctx = tctx_task->io_uring; while (1) { enum io_wq_cancel cret; -- cgit From f85c310ac376ce81a954507315ff11be4ddbf214 Mon Sep 17 00:00:00 2001 From: Pavel Begunkov Date: Mon, 1 Mar 2021 18:20:46 +0000 Subject: io_uring: inline io_req_clean_work() Inline io_req_clean_work(), less code and easier to analyse tctx dependencies and refs usage. Signed-off-by: Pavel Begunkov Signed-off-by: Jens Axboe --- fs/io_uring.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index d6c5465b372b..afa0e91488e6 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -1167,22 +1167,6 @@ static bool req_need_defer(struct io_kiocb *req, u32 seq) return false; } -static void io_req_clean_work(struct io_kiocb *req) -{ - if (req->flags & REQ_F_INFLIGHT) { - struct io_ring_ctx *ctx = req->ctx; - struct io_uring_task *tctx = req->task->io_uring; - unsigned long flags; - - spin_lock_irqsave(&ctx->inflight_lock, flags); - list_del(&req->inflight_entry); - spin_unlock_irqrestore(&ctx->inflight_lock, flags); - req->flags &= ~REQ_F_INFLIGHT; - if (atomic_read(&tctx->in_idle)) - wake_up(&tctx->wait); - } -} - static void io_req_track_inflight(struct io_kiocb *req) { struct io_ring_ctx *ctx = req->ctx; @@ -1671,7 +1655,19 @@ static void io_dismantle_req(struct io_kiocb *req) io_put_file(req, req->file, (req->flags & REQ_F_FIXED_FILE)); if (req->fixed_rsrc_refs) percpu_ref_put(req->fixed_rsrc_refs); - io_req_clean_work(req); + + if (req->flags & REQ_F_INFLIGHT) { + struct io_ring_ctx *ctx = req->ctx; + struct io_uring_task *tctx = req->task->io_uring; + unsigned long flags; + + spin_lock_irqsave(&ctx->inflight_lock, flags); + list_del(&req->inflight_entry); + spin_unlock_irqrestore(&ctx->inflight_lock, flags); + req->flags &= ~REQ_F_INFLIGHT; + if (atomic_read(&tctx->in_idle)) + wake_up(&tctx->wait); + } } static inline void io_put_task(struct task_struct *task, int nr) -- cgit From ebf936670721be805a9cb87781a5ee9271ba4633 Mon Sep 17 00:00:00 2001 From: Pavel Begunkov Date: Mon, 1 Mar 2021 18:20:47 +0000 Subject: io_uring: inline __io_queue_async_work() __io_queue_async_work() is only called from io_queue_async_work(), inline it. Signed-off-by: Pavel Begunkov Signed-off-by: Jens Axboe --- fs/io_uring.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index afa0e91488e6..840b73db9c3d 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -1205,7 +1205,7 @@ static void io_prep_async_link(struct io_kiocb *req) io_prep_async_work(cur); } -static struct io_kiocb *__io_queue_async_work(struct io_kiocb *req) +static void io_queue_async_work(struct io_kiocb *req) { struct io_ring_ctx *ctx = req->ctx; struct io_kiocb *link = io_prep_linked_timeout(req); @@ -1216,18 +1216,9 @@ static struct io_kiocb *__io_queue_async_work(struct io_kiocb *req) trace_io_uring_queue_async_work(ctx, io_wq_is_hashed(&req->work), req, &req->work, req->flags); - io_wq_enqueue(tctx->io_wq, &req->work); - return link; -} - -static void io_queue_async_work(struct io_kiocb *req) -{ - struct io_kiocb *link; - /* init ->work of the whole link before punting */ io_prep_async_link(req); - link = __io_queue_async_work(req); - + io_wq_enqueue(tctx->io_wq, &req->work); if (link) io_queue_linked_timeout(link); } -- cgit From b23fcf477f85164f3b33b2e8c2c99b2ec61ba902 Mon Sep 17 00:00:00 2001 From: Pavel Begunkov Date: Mon, 1 Mar 2021 18:20:48 +0000 Subject: io_uring: remove extra in_idle wake up io_dismantle_req() is always followed by io_put_task(), which already do proper in_idle wake ups, so we can skip waking the owner task in io_dismantle_req(). The rules are simpler now, do io_put_task() shortly after ending a request, and it will be fine. Signed-off-by: Pavel Begunkov Signed-off-by: Jens Axboe --- fs/io_uring.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index 840b73db9c3d..26e83cabf3bf 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -1649,18 +1649,16 @@ static void io_dismantle_req(struct io_kiocb *req) if (req->flags & REQ_F_INFLIGHT) { struct io_ring_ctx *ctx = req->ctx; - struct io_uring_task *tctx = req->task->io_uring; unsigned long flags; spin_lock_irqsave(&ctx->inflight_lock, flags); list_del(&req->inflight_entry); spin_unlock_irqrestore(&ctx->inflight_lock, flags); req->flags &= ~REQ_F_INFLIGHT; - if (atomic_read(&tctx->in_idle)) - wake_up(&tctx->wait); } } +/* must to be called somewhat shortly after putting a request */ static inline void io_put_task(struct task_struct *task, int nr) { struct io_uring_task *tctx = task->io_uring; -- cgit From e4b4a13f494120c475580927864cc1dd96f595d1 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Mon, 1 Mar 2021 18:36:25 -0700 Subject: io_uring: ensure that threads freeze on suspend Alex reports that his system fails to suspend using 5.12-rc1, with the following dump: [ 240.650300] PM: suspend entry (deep) [ 240.650748] Filesystems sync: 0.000 seconds [ 240.725605] Freezing user space processes ... [ 260.739483] Freezing of tasks failed after 20.013 seconds (3 tasks refusing to freeze, wq_busy=0): [ 260.739497] task:iou-mgr-446 state:S stack: 0 pid: 516 ppid: 439 flags:0x00004224 [ 260.739504] Call Trace: [ 260.739507] ? sysvec_apic_timer_interrupt+0xb/0x81 [ 260.739515] ? pick_next_task_fair+0x197/0x1cde [ 260.739519] ? sysvec_reschedule_ipi+0x2f/0x6a [ 260.739522] ? asm_sysvec_reschedule_ipi+0x12/0x20 [ 260.739525] ? __schedule+0x57/0x6d6 [ 260.739529] ? del_timer_sync+0xb9/0x115 [ 260.739533] ? schedule+0x63/0xd5 [ 260.739536] ? schedule_timeout+0x219/0x356 [ 260.739540] ? __next_timer_interrupt+0xf1/0xf1 [ 260.739544] ? io_wq_manager+0x73/0xb1 [ 260.739549] ? io_wq_create+0x262/0x262 [ 260.739553] ? ret_from_fork+0x22/0x30 [ 260.739557] task:iou-mgr-517 state:S stack: 0 pid: 522 ppid: 439 flags:0x00004224 [ 260.739561] Call Trace: [ 260.739563] ? sysvec_apic_timer_interrupt+0xb/0x81 [ 260.739566] ? pick_next_task_fair+0x16f/0x1cde [ 260.739569] ? sysvec_apic_timer_interrupt+0xb/0x81 [ 260.739571] ? asm_sysvec_apic_timer_interrupt+0x12/0x20 [ 260.739574] ? __schedule+0x5b7/0x6d6 [ 260.739578] ? del_timer_sync+0x70/0x115 [ 260.739581] ? schedule_timeout+0x211/0x356 [ 260.739585] ? __next_timer_interrupt+0xf1/0xf1 [ 260.739588] ? io_wq_check_workers+0x15/0x11f [ 260.739592] ? io_wq_manager+0x69/0xb1 [ 260.739596] ? io_wq_create+0x262/0x262 [ 260.739600] ? ret_from_fork+0x22/0x30 [ 260.739603] task:iou-wrk-517 state:S stack: 0 pid: 523 ppid: 439 flags:0x00004224 [ 260.739607] Call Trace: [ 260.739609] ? __schedule+0x5b7/0x6d6 [ 260.739614] ? schedule+0x63/0xd5 [ 260.739617] ? schedule_timeout+0x219/0x356 [ 260.739621] ? __next_timer_interrupt+0xf1/0xf1 [ 260.739624] ? task_thread.isra.0+0x148/0x3af [ 260.739628] ? task_thread_unbound+0xa/0xa [ 260.739632] ? task_thread_bound+0x7/0x7 [ 260.739636] ? ret_from_fork+0x22/0x30 [ 260.739647] OOM killer enabled. [ 260.739648] Restarting tasks ... done. [ 260.740077] PM: suspend exit Play nice and ensure that any thread we create will call try_to_freeze() at an opportune time so that memory suspend can proceed. For the io-wq worker threads, mark them as PF_NOFREEZE. They could potentially be blocked for a long time. Reported-by: Alex Xu (Hello71) Tested-by: Alex Xu (Hello71) Signed-off-by: Jens Axboe --- fs/io-wq.c | 3 +++ fs/io_uring.c | 5 ++--- 2 files changed, 5 insertions(+), 3 deletions(-) (limited to 'fs') diff --git a/fs/io-wq.c b/fs/io-wq.c index dc430381b694..acffc85d1a93 100644 --- a/fs/io-wq.c +++ b/fs/io-wq.c @@ -16,6 +16,7 @@ #include #include #include +#include #include "../kernel/sched/sched.h" #include "io-wq.h" @@ -263,6 +264,7 @@ static void io_wqe_dec_running(struct io_worker *worker) static void io_worker_start(struct io_worker *worker) { + current->flags |= PF_NOFREEZE; worker->flags |= (IO_WORKER_F_UP | IO_WORKER_F_RUNNING); io_wqe_inc_running(worker); complete(&worker->started); @@ -731,6 +733,7 @@ static int io_wq_manager(void *data) set_current_state(TASK_INTERRUPTIBLE); io_wq_check_workers(wq); schedule_timeout(HZ); + try_to_freeze(); if (fatal_signal_pending(current)) set_bit(IO_WQ_BIT_EXIT, &wq->state); } while (!test_bit(IO_WQ_BIT_EXIT, &wq->state)); diff --git a/fs/io_uring.c b/fs/io_uring.c index 26e83cabf3bf..b1734efdc7e8 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -74,13 +74,11 @@ #include #include #include -#include #include #include #include #include -#include -#include +#include #define CREATE_TRACE_POINTS #include @@ -6736,6 +6734,7 @@ static int io_sq_thread(void *data) io_ring_set_wakeup_flag(ctx); schedule(); + try_to_freeze(); list_for_each_entry(ctx, &sqd->ctx_list, sqd_list) io_ring_clear_wakeup_flag(ctx); } -- cgit From f01272541d2cd7b7f24909d63ea2b028a6a66293 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Wed, 3 Mar 2021 15:47:04 -0700 Subject: io-wq: ensure all pending work is canceled on exit If we race on shutting down the io-wq, then we should ensure that any work that was queued after workers shutdown is canceled. Harden the add work check a bit too, checking for IO_WQ_BIT_EXIT and cancel if it's set. Add a WARN_ON() for having any work before we kill the io-wq context. Reported-by: syzbot+91b4b56ead187d35c9d3@syzkaller.appspotmail.com Signed-off-by: Jens Axboe --- fs/io-wq.c | 42 +++++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-) (limited to 'fs') diff --git a/fs/io-wq.c b/fs/io-wq.c index acffc85d1a93..19f18389ead2 100644 --- a/fs/io-wq.c +++ b/fs/io-wq.c @@ -129,6 +129,17 @@ struct io_wq { static enum cpuhp_state io_wq_online; +struct io_cb_cancel_data { + work_cancel_fn *fn; + void *data; + int nr_running; + int nr_pending; + bool cancel_all; +}; + +static void io_wqe_cancel_pending_work(struct io_wqe *wqe, + struct io_cb_cancel_data *match); + static bool io_worker_get(struct io_worker *worker) { return refcount_inc_not_zero(&worker->ref); @@ -713,6 +724,23 @@ static void io_wq_check_workers(struct io_wq *wq) } } +static bool io_wq_work_match_all(struct io_wq_work *work, void *data) +{ + return true; +} + +static void io_wq_cancel_pending(struct io_wq *wq) +{ + struct io_cb_cancel_data match = { + .fn = io_wq_work_match_all, + .cancel_all = true, + }; + int node; + + for_each_node(node) + io_wqe_cancel_pending_work(wq->wqes[node], &match); +} + /* * Manager thread. Tasked with creating new workers, if we need them. */ @@ -748,6 +776,8 @@ static int io_wq_manager(void *data) /* we might not ever have created any workers */ if (atomic_read(&wq->worker_refs)) wait_for_completion(&wq->worker_done); + + io_wq_cancel_pending(wq); complete(&wq->exited); do_exit(0); } @@ -809,7 +839,8 @@ static void io_wqe_enqueue(struct io_wqe *wqe, struct io_wq_work *work) unsigned long flags; /* Can only happen if manager creation fails after exec */ - if (unlikely(io_wq_fork_manager(wqe->wq))) { + if (io_wq_fork_manager(wqe->wq) || + test_bit(IO_WQ_BIT_EXIT, &wqe->wq->state)) { work->flags |= IO_WQ_WORK_CANCEL; wqe->wq->do_work(work); return; @@ -845,14 +876,6 @@ void io_wq_hash_work(struct io_wq_work *work, void *val) work->flags |= (IO_WQ_WORK_HASHED | (bit << IO_WQ_HASH_SHIFT)); } -struct io_cb_cancel_data { - work_cancel_fn *fn; - void *data; - int nr_running; - int nr_pending; - bool cancel_all; -}; - static bool io_wq_worker_cancel(struct io_worker *worker, void *data) { struct io_cb_cancel_data *match = data; @@ -1086,6 +1109,7 @@ static void io_wq_destroy(struct io_wq *wq) struct io_wqe *wqe = wq->wqes[node]; list_del_init(&wqe->wait.entry); + WARN_ON_ONCE(!wq_list_empty(&wqe->work_list)); kfree(wqe); } spin_unlock_irq(&wq->hash->wait.lock); -- cgit From b05a1bcd40184f12f2cd87db79e871aa8c17563f Mon Sep 17 00:00:00 2001 From: Pavel Begunkov Date: Thu, 4 Mar 2021 13:59:24 +0000 Subject: io_uring: cancel-match based on flags Instead of going into request internals, like checking req->file->f_op, do match them based on REQ_F_INFLIGHT, it's set only when we want it to be reliably cancelled. Signed-off-by: Pavel Begunkov Signed-off-by: Jens Axboe --- fs/io_uring.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index b1734efdc7e8..fb4abea1e5d6 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -703,7 +703,7 @@ enum { /* fail rest of links */ REQ_F_FAIL_LINK = BIT(REQ_F_FAIL_LINK_BIT), - /* on inflight list */ + /* on inflight list, should be cancelled and waited on exit reliably */ REQ_F_INFLIGHT = BIT(REQ_F_INFLIGHT_BIT), /* read/write uses file position */ REQ_F_CUR_POS = BIT(REQ_F_CUR_POS_BIT), @@ -1069,7 +1069,7 @@ static bool io_match_task(struct io_kiocb *head, return true; io_for_each_link(req, head) { - if (req->file && req->file->f_op == &io_uring_fops) + if (req->flags & REQ_F_INFLIGHT) return true; if (req->task->files == files) return true; -- cgit From dd59a3d595cc10230ded4c8b727b096e16bceeb5 Mon Sep 17 00:00:00 2001 From: Pavel Begunkov Date: Thu, 4 Mar 2021 13:59:25 +0000 Subject: io_uring: reliably cancel linked timeouts Linked timeouts are fired asynchronously (i.e. soft-irq), and use generic cancellation paths to do its stuff, including poking into io-wq. The problem is that it's racy to access tctx->io_wq, as io_uring_task_cancel() and others may be happening at this exact moment. Mark linked timeouts with REQ_F_INLIFGHT for now, making sure there are no timeouts before io-wq destraction. Signed-off-by: Pavel Begunkov Signed-off-by: Jens Axboe --- fs/io_uring.c | 1 + 1 file changed, 1 insertion(+) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index fb4abea1e5d6..e55369555e5c 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -5500,6 +5500,7 @@ static int io_timeout_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe, data->mode = io_translate_timeout_mode(flags); hrtimer_init(&data->timer, CLOCK_MONOTONIC, data->mode); + io_req_track_inflight(req); return 0; } -- cgit From 46fe18b16c4656969347fc0a3d83a034e47d9119 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Thu, 4 Mar 2021 12:39:36 -0700 Subject: io_uring: move to using create_io_thread() This allows us to do task creation and setup without needing to use completions to try and synchronize with the starting thread. Get rid of the old io_wq_fork_thread() wrapper, and the 'wq' and 'worker' startup completion events - we can now do setup before the task is running. Signed-off-by: Jens Axboe --- fs/io-wq.c | 123 +++++++++++++++++----------------------------------------- fs/io-wq.h | 2 - fs/io_uring.c | 38 +++++++++--------- 3 files changed, 54 insertions(+), 109 deletions(-) (limited to 'fs') diff --git a/fs/io-wq.c b/fs/io-wq.c index 19f18389ead2..d7cfe8fd282a 100644 --- a/fs/io-wq.c +++ b/fs/io-wq.c @@ -54,7 +54,6 @@ struct io_worker { spinlock_t lock; struct completion ref_done; - struct completion started; struct rcu_head rcu; }; @@ -116,7 +115,6 @@ struct io_wq { struct io_wq_hash *hash; refcount_t refs; - struct completion started; struct completion exited; atomic_t worker_refs; @@ -199,6 +197,7 @@ static void io_worker_exit(struct io_worker *worker) kfree_rcu(worker, rcu); if (atomic_dec_and_test(&wqe->wq->worker_refs)) complete(&wqe->wq->worker_done); + do_exit(0); } static inline bool io_wqe_run_queue(struct io_wqe *wqe) @@ -273,14 +272,6 @@ static void io_wqe_dec_running(struct io_worker *worker) io_wqe_wake_worker(wqe, acct); } -static void io_worker_start(struct io_worker *worker) -{ - current->flags |= PF_NOFREEZE; - worker->flags |= (IO_WORKER_F_UP | IO_WORKER_F_RUNNING); - io_wqe_inc_running(worker); - complete(&worker->started); -} - /* * Worker will start processing some work. Move it to the busy list, if * it's currently on the freelist @@ -489,8 +480,13 @@ static int io_wqe_worker(void *data) struct io_worker *worker = data; struct io_wqe *wqe = worker->wqe; struct io_wq *wq = wqe->wq; + char buf[TASK_COMM_LEN]; - io_worker_start(worker); + worker->flags |= (IO_WORKER_F_UP | IO_WORKER_F_RUNNING); + io_wqe_inc_running(worker); + + sprintf(buf, "iou-wrk-%d", wq->task_pid); + set_task_comm(current, buf); while (!test_bit(IO_WQ_BIT_EXIT, &wq->state)) { set_current_state(TASK_INTERRUPTIBLE); @@ -565,67 +561,11 @@ void io_wq_worker_sleeping(struct task_struct *tsk) raw_spin_unlock_irq(&worker->wqe->lock); } -static int task_thread(void *data, int index) -{ - struct io_worker *worker = data; - struct io_wqe *wqe = worker->wqe; - struct io_wqe_acct *acct = &wqe->acct[index]; - struct io_wq *wq = wqe->wq; - char buf[TASK_COMM_LEN]; - - sprintf(buf, "iou-wrk-%d", wq->task_pid); - set_task_comm(current, buf); - - current->pf_io_worker = worker; - worker->task = current; - - set_cpus_allowed_ptr(current, cpumask_of_node(wqe->node)); - current->flags |= PF_NO_SETAFFINITY; - - raw_spin_lock_irq(&wqe->lock); - hlist_nulls_add_head_rcu(&worker->nulls_node, &wqe->free_list); - list_add_tail_rcu(&worker->all_list, &wqe->all_list); - worker->flags |= IO_WORKER_F_FREE; - if (index == IO_WQ_ACCT_BOUND) - worker->flags |= IO_WORKER_F_BOUND; - if (!acct->nr_workers && (worker->flags & IO_WORKER_F_BOUND)) - worker->flags |= IO_WORKER_F_FIXED; - acct->nr_workers++; - raw_spin_unlock_irq(&wqe->lock); - - io_wqe_worker(data); - do_exit(0); -} - -static int task_thread_bound(void *data) -{ - return task_thread(data, IO_WQ_ACCT_BOUND); -} - -static int task_thread_unbound(void *data) -{ - return task_thread(data, IO_WQ_ACCT_UNBOUND); -} - -pid_t io_wq_fork_thread(int (*fn)(void *), void *arg) -{ - unsigned long flags = CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD| - CLONE_IO|SIGCHLD; - struct kernel_clone_args args = { - .flags = ((lower_32_bits(flags) | CLONE_VM | - CLONE_UNTRACED) & ~CSIGNAL), - .exit_signal = (lower_32_bits(flags) & CSIGNAL), - .stack = (unsigned long)fn, - .stack_size = (unsigned long)arg, - }; - - return kernel_clone(&args); -} - static bool create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index) { + struct io_wqe_acct *acct = &wqe->acct[index]; struct io_worker *worker; - pid_t pid; + struct task_struct *tsk; __set_current_state(TASK_RUNNING); @@ -638,21 +578,33 @@ static bool create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index) worker->wqe = wqe; spin_lock_init(&worker->lock); init_completion(&worker->ref_done); - init_completion(&worker->started); atomic_inc(&wq->worker_refs); - if (index == IO_WQ_ACCT_BOUND) - pid = io_wq_fork_thread(task_thread_bound, worker); - else - pid = io_wq_fork_thread(task_thread_unbound, worker); - if (pid < 0) { + tsk = create_io_thread(io_wqe_worker, worker, wqe->node); + if (IS_ERR(tsk)) { if (atomic_dec_and_test(&wq->worker_refs)) complete(&wq->worker_done); kfree(worker); return false; } - wait_for_completion(&worker->started); + + tsk->pf_io_worker = worker; + worker->task = tsk; + set_cpus_allowed_ptr(tsk, cpumask_of_node(wqe->node)); + tsk->flags |= PF_NOFREEZE | PF_NO_SETAFFINITY; + + raw_spin_lock_irq(&wqe->lock); + hlist_nulls_add_head_rcu(&worker->nulls_node, &wqe->free_list); + list_add_tail_rcu(&worker->all_list, &wqe->all_list); + worker->flags |= IO_WORKER_F_FREE; + if (index == IO_WQ_ACCT_BOUND) + worker->flags |= IO_WORKER_F_BOUND; + if (!acct->nr_workers && (worker->flags & IO_WORKER_F_BOUND)) + worker->flags |= IO_WORKER_F_FIXED; + acct->nr_workers++; + raw_spin_unlock_irq(&wqe->lock); + wake_up_new_task(tsk); return true; } @@ -696,6 +648,7 @@ static bool io_wq_for_each_worker(struct io_wqe *wqe, static bool io_wq_worker_wake(struct io_worker *worker, void *data) { + set_notify_signal(worker->task); wake_up_process(worker->task); return false; } @@ -752,10 +705,6 @@ static int io_wq_manager(void *data) sprintf(buf, "iou-mgr-%d", wq->task_pid); set_task_comm(current, buf); - current->flags |= PF_IO_WORKER; - wq->manager = get_task_struct(current); - - complete(&wq->started); do { set_current_state(TASK_INTERRUPTIBLE); @@ -815,21 +764,20 @@ append: static int io_wq_fork_manager(struct io_wq *wq) { - int ret; + struct task_struct *tsk; if (wq->manager) return 0; reinit_completion(&wq->worker_done); - current->flags |= PF_IO_WORKER; - ret = io_wq_fork_thread(io_wq_manager, wq); - current->flags &= ~PF_IO_WORKER; - if (ret >= 0) { - wait_for_completion(&wq->started); + tsk = create_io_thread(io_wq_manager, wq, NUMA_NO_NODE); + if (!IS_ERR(tsk)) { + wq->manager = get_task_struct(tsk); + wake_up_new_task(tsk); return 0; } - return ret; + return PTR_ERR(tsk); } static void io_wqe_enqueue(struct io_wqe *wqe, struct io_wq_work *work) @@ -1062,7 +1010,6 @@ struct io_wq *io_wq_create(unsigned bounded, struct io_wq_data *data) } wq->task_pid = current->pid; - init_completion(&wq->started); init_completion(&wq->exited); refcount_set(&wq->refs, 1); diff --git a/fs/io-wq.h b/fs/io-wq.h index 42f0be64a84d..5fbf7997149e 100644 --- a/fs/io-wq.h +++ b/fs/io-wq.h @@ -119,8 +119,6 @@ void io_wq_put_and_exit(struct io_wq *wq); void io_wq_enqueue(struct io_wq *wq, struct io_wq_work *work); void io_wq_hash_work(struct io_wq_work *work, void *val); -pid_t io_wq_fork_thread(int (*fn)(void *), void *arg); - static inline bool io_wq_is_hashed(struct io_wq_work *work) { return work->flags & IO_WQ_WORK_HASHED; diff --git a/fs/io_uring.c b/fs/io_uring.c index e55369555e5c..76e243c056b5 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -6668,7 +6668,6 @@ static int io_sq_thread(void *data) sprintf(buf, "iou-sqp-%d", sqd->task_pid); set_task_comm(current, buf); - sqd->thread = current; current->pf_io_worker = NULL; if (sqd->sq_cpu != -1) @@ -6677,8 +6676,6 @@ static int io_sq_thread(void *data) set_cpus_allowed_ptr(current, cpu_online_mask); current->flags |= PF_NO_SETAFFINITY; - complete(&sqd->completion); - wait_for_completion(&sqd->startup); while (!io_sq_thread_should_stop(sqd)) { @@ -7818,21 +7815,22 @@ void __io_uring_free(struct task_struct *tsk) static int io_sq_thread_fork(struct io_sq_data *sqd, struct io_ring_ctx *ctx) { + struct task_struct *tsk; int ret; clear_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state); reinit_completion(&sqd->completion); ctx->sqo_exec = 0; sqd->task_pid = current->pid; - current->flags |= PF_IO_WORKER; - ret = io_wq_fork_thread(io_sq_thread, sqd); - current->flags &= ~PF_IO_WORKER; - if (ret < 0) { - sqd->thread = NULL; - return ret; - } - wait_for_completion(&sqd->completion); - return io_uring_alloc_task_context(sqd->thread, ctx); + tsk = create_io_thread(io_sq_thread, sqd, NUMA_NO_NODE); + if (IS_ERR(tsk)) + return PTR_ERR(tsk); + ret = io_uring_alloc_task_context(tsk, ctx); + if (ret) + set_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state); + sqd->thread = tsk; + wake_up_new_task(tsk); + return ret; } static int io_sq_offload_create(struct io_ring_ctx *ctx, @@ -7855,6 +7853,7 @@ static int io_sq_offload_create(struct io_ring_ctx *ctx, fdput(f); } if (ctx->flags & IORING_SETUP_SQPOLL) { + struct task_struct *tsk; struct io_sq_data *sqd; ret = -EPERM; @@ -7896,15 +7895,16 @@ static int io_sq_offload_create(struct io_ring_ctx *ctx, } sqd->task_pid = current->pid; - current->flags |= PF_IO_WORKER; - ret = io_wq_fork_thread(io_sq_thread, sqd); - current->flags &= ~PF_IO_WORKER; - if (ret < 0) { - sqd->thread = NULL; + tsk = create_io_thread(io_sq_thread, sqd, NUMA_NO_NODE); + if (IS_ERR(tsk)) { + ret = PTR_ERR(tsk); goto err; } - wait_for_completion(&sqd->completion); - ret = io_uring_alloc_task_context(sqd->thread, ctx); + ret = io_uring_alloc_task_context(tsk, ctx); + if (ret) + set_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state); + sqd->thread = tsk; + wake_up_new_task(tsk); if (ret) goto err; } else if (p->flags & IORING_SETUP_SQ_AFF) { -- cgit From ca0a26511c679a797f86589894a4523db36d833e Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Thu, 4 Mar 2021 17:15:48 -0700 Subject: io_uring: don't keep looping for more events if we can't flush overflow It doesn't make sense to wait for more events to come in, if we can't even flush the overflow we already have to the ring. Return -EBUSY for that condition, just like we do for attempts to submit with overflow pending. Cc: stable@vger.kernel.org # 5.11 Signed-off-by: Jens Axboe --- fs/io_uring.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index 76e243c056b5..044170165402 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -1451,18 +1451,22 @@ static bool __io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force, return all_flushed; } -static void io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force, +static bool io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force, struct task_struct *tsk, struct files_struct *files) { + bool ret = true; + if (test_bit(0, &ctx->cq_check_overflow)) { /* iopoll syncs against uring_lock, not completion_lock */ if (ctx->flags & IORING_SETUP_IOPOLL) mutex_lock(&ctx->uring_lock); - __io_cqring_overflow_flush(ctx, force, tsk, files); + ret = __io_cqring_overflow_flush(ctx, force, tsk, files); if (ctx->flags & IORING_SETUP_IOPOLL) mutex_unlock(&ctx->uring_lock); } + + return ret; } static void __io_cqring_fill_event(struct io_kiocb *req, long res, long cflags) @@ -6883,11 +6887,16 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events, iowq.nr_timeouts = atomic_read(&ctx->cq_timeouts); trace_io_uring_cqring_wait(ctx, min_events); do { - io_cqring_overflow_flush(ctx, false, NULL, NULL); + /* if we can't even flush overflow, don't wait for more */ + if (!io_cqring_overflow_flush(ctx, false, NULL, NULL)) { + ret = -EBUSY; + break; + } prepare_to_wait_exclusive(&ctx->wait, &iowq.wq, TASK_INTERRUPTIBLE); ret = io_cqring_wait_schedule(ctx, &iowq, &timeout); finish_wait(&ctx->wait, &iowq.wq); + cond_resched(); } while (ret > 0); restore_saved_sigmask_unless(ret == -EINTR); -- cgit From b5b0ecb736f1ce1e68eb50613c0cfecff10198eb Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Thu, 4 Mar 2021 21:02:58 -0700 Subject: io_uring: clear IOCB_WAITQ for non -EIOCBQUEUED return The callback can only be armed, if we get -EIOCBQUEUED returned. It's important that we clear the WAITQ bit for other cases, otherwise we can queue for async retry and filemap will assume that we're armed and return -EAGAIN instead of just blocking for the IO. Cc: stable@vger.kernel.org # 5.9+ Signed-off-by: Jens Axboe --- fs/io_uring.c | 1 + 1 file changed, 1 insertion(+) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index 044170165402..5762750c666c 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -3286,6 +3286,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags) if (ret == -EIOCBQUEUED) return 0; /* we got some bytes, but not all. retry. */ + kiocb->ki_flags &= ~IOCB_WAITQ; } while (ret > 0 && ret < io_size); done: kiocb_done(kiocb, ret, issue_flags); -- cgit From 09ca6c40c2024211657fdb2c50522a355610c3b7 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Fri, 5 Mar 2021 08:14:08 -0700 Subject: io-wq: kill hashed waitqueue before manager exits If we race with shutting down the io-wq context and someone queueing a hashed entry, then we can exit the manager with it armed. If it then triggers after the manager has exited, we can have a use-after-free where io_wqe_hash_wake() attempts to wake a now gone manager process. Move the killing of the hashed write queue into the manager itself, so that we know we've killed it before the task exits. Fixes: e941894eae31 ("io-wq: make buffered file write hashed work map per-ctx") Signed-off-by: Jens Axboe --- fs/io-wq.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'fs') diff --git a/fs/io-wq.c b/fs/io-wq.c index d7cfe8fd282a..28868eb4cd09 100644 --- a/fs/io-wq.c +++ b/fs/io-wq.c @@ -726,6 +726,11 @@ static int io_wq_manager(void *data) if (atomic_read(&wq->worker_refs)) wait_for_completion(&wq->worker_done); + spin_lock_irq(&wq->hash->wait.lock); + for_each_node(node) + list_del_init(&wq->wqes[node]->wait.entry); + spin_unlock_irq(&wq->hash->wait.lock); + io_wq_cancel_pending(wq); complete(&wq->exited); do_exit(0); @@ -1051,15 +1056,11 @@ static void io_wq_destroy(struct io_wq *wq) set_bit(IO_WQ_BIT_EXIT, &wq->state); io_wq_destroy_manager(wq); - spin_lock_irq(&wq->hash->wait.lock); for_each_node(node) { struct io_wqe *wqe = wq->wqes[node]; - - list_del_init(&wqe->wait.entry); WARN_ON_ONCE(!wq_list_empty(&wqe->work_list)); kfree(wqe); } - spin_unlock_irq(&wq->hash->wait.lock); io_wq_put_hash(wq->hash); kfree(wq->wqes); kfree(wq); -- cgit From 86e0d6766cf909813474857bd22fdc04c97c0b36 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Fri, 5 Mar 2021 08:44:39 -0700 Subject: io_uring: make SQPOLL thread parking saner We have this weird true/false return from parking, and then some of the callers decide to look at that. It can lead to unbalanced parks and sqd locking. Have the callers check the thread status once it's parked. We know we have the lock at that point, so it's either valid or it's NULL. Fix race with parking on thread exit. We need to be careful here with ordering of the sdq->lock and the IO_SQ_THREAD_SHOULD_PARK bit. Rename sqd->completion to sqd->parked to reflect that this is the only thing this completion event doesn. Signed-off-by: Jens Axboe --- fs/io_uring.c | 65 +++++++++++++++++++++++++++-------------------------------- 1 file changed, 30 insertions(+), 35 deletions(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index 5762750c666c..d30cbf0f7b1c 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -274,7 +274,7 @@ struct io_sq_data { unsigned long state; struct completion startup; - struct completion completion; + struct completion parked; struct completion exited; }; @@ -6656,7 +6656,7 @@ static void io_sq_thread_parkme(struct io_sq_data *sqd) * wait_task_inactive(). */ preempt_disable(); - complete(&sqd->completion); + complete(&sqd->parked); schedule_preempt_disabled(); preempt_enable(); } @@ -6751,14 +6751,18 @@ static int io_sq_thread(void *data) io_run_task_work(); - if (io_sq_thread_should_park(sqd)) - io_sq_thread_parkme(sqd); - /* - * Clear thread under lock so that concurrent parks work correctly + * Ensure that we park properly if racing with someone trying to park + * while we're exiting. If we fail to grab the lock, check park and + * park if necessary. The ordering with the park bit and the lock + * ensures that we catch this reliably. */ - complete(&sqd->completion); - mutex_lock(&sqd->lock); + if (!mutex_trylock(&sqd->lock)) { + if (io_sq_thread_should_park(sqd)) + io_sq_thread_parkme(sqd); + mutex_lock(&sqd->lock); + } + sqd->thread = NULL; list_for_each_entry(ctx, &sqd->ctx_list, sqd_list) { ctx->sqo_exec = 1; @@ -7067,29 +7071,25 @@ static int io_sqe_files_unregister(struct io_ring_ctx *ctx) static void io_sq_thread_unpark(struct io_sq_data *sqd) __releases(&sqd->lock) { - if (!sqd->thread) - return; if (sqd->thread == current) return; clear_bit(IO_SQ_THREAD_SHOULD_PARK, &sqd->state); - wake_up_state(sqd->thread, TASK_PARKED); + if (sqd->thread) + wake_up_state(sqd->thread, TASK_PARKED); mutex_unlock(&sqd->lock); } -static bool io_sq_thread_park(struct io_sq_data *sqd) +static void io_sq_thread_park(struct io_sq_data *sqd) __acquires(&sqd->lock) { if (sqd->thread == current) - return true; + return; + set_bit(IO_SQ_THREAD_SHOULD_PARK, &sqd->state); mutex_lock(&sqd->lock); - if (!sqd->thread) { - mutex_unlock(&sqd->lock); - return false; + if (sqd->thread) { + wake_up_process(sqd->thread); + wait_for_completion(&sqd->parked); } - set_bit(IO_SQ_THREAD_SHOULD_PARK, &sqd->state); - wake_up_process(sqd->thread); - wait_for_completion(&sqd->completion); - return true; } static void io_sq_thread_stop(struct io_sq_data *sqd) @@ -7185,7 +7185,7 @@ static struct io_sq_data *io_get_sq_data(struct io_uring_params *p) mutex_init(&sqd->lock); init_waitqueue_head(&sqd->wait); init_completion(&sqd->startup); - init_completion(&sqd->completion); + init_completion(&sqd->parked); init_completion(&sqd->exited); return sqd; } @@ -7829,7 +7829,7 @@ static int io_sq_thread_fork(struct io_sq_data *sqd, struct io_ring_ctx *ctx) int ret; clear_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state); - reinit_completion(&sqd->completion); + reinit_completion(&sqd->parked); ctx->sqo_exec = 0; sqd->task_pid = current->pid; tsk = create_io_thread(io_sq_thread, sqd, NUMA_NO_NODE); @@ -8712,7 +8712,6 @@ static void io_uring_cancel_task_requests(struct io_ring_ctx *ctx, struct files_struct *files) { struct task_struct *task = current; - bool did_park = false; if ((ctx->flags & IORING_SETUP_SQPOLL) && ctx->sq_data) { /* never started, nothing to cancel */ @@ -8720,11 +8719,10 @@ static void io_uring_cancel_task_requests(struct io_ring_ctx *ctx, io_sq_offload_start(ctx); return; } - did_park = io_sq_thread_park(ctx->sq_data); - if (did_park) { - task = ctx->sq_data->thread; + io_sq_thread_park(ctx->sq_data); + task = ctx->sq_data->thread; + if (task) atomic_inc(&task->io_uring->in_idle); - } } io_cancel_defer_files(ctx, task, files); @@ -8733,10 +8731,10 @@ static void io_uring_cancel_task_requests(struct io_ring_ctx *ctx, if (!files) io_uring_try_cancel_requests(ctx, task, NULL); - if (did_park) { + if (task) atomic_dec(&task->io_uring->in_idle); + if (ctx->sq_data) io_sq_thread_unpark(ctx->sq_data); - } } /* @@ -8836,15 +8834,12 @@ static void io_uring_cancel_sqpoll(struct io_ring_ctx *ctx) if (!sqd) return; - if (!io_sq_thread_park(sqd)) - return; - tctx = ctx->sq_data->thread->io_uring; - /* can happen on fork/alloc failure, just ignore that state */ - if (!tctx) { + io_sq_thread_park(sqd); + if (!sqd->thread || !sqd->thread->io_uring) { io_sq_thread_unpark(sqd); return; } - + tctx = ctx->sq_data->thread->io_uring; atomic_inc(&tctx->in_idle); do { /* read completions before cancelations */ -- cgit From e45cff58858883290c98f65d409839a7295c95f3 Mon Sep 17 00:00:00 2001 From: Pavel Begunkov Date: Sun, 28 Feb 2021 22:35:14 +0000 Subject: io_uring: don't restrict issue_flags for io_openat 45d189c606292 ("io_uring: replace force_nonblock with flags") did something strange for io_openat() slicing all issue_flags but IO_URING_F_NONBLOCK. Not a bug for now, but better to just forward the flags. Signed-off-by: Pavel Begunkov Signed-off-by: Jens Axboe --- fs/io_uring.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs') diff --git a/fs/io_uring.c b/fs/io_uring.c index d30cbf0f7b1c..92c25b5f1349 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -3828,7 +3828,7 @@ err: static int io_openat(struct io_kiocb *req, unsigned int issue_flags) { - return io_openat2(req, issue_flags & IO_URING_F_NONBLOCK); + return io_openat2(req, issue_flags); } static int io_remove_buffers_prep(struct io_kiocb *req, -- cgit