Fix ublk stop races - #2
Open
bsbernd wants to merge 6 commits into
Open
Conversation
This reverts commit 0ad6835.
This reverts commit cd624cf.
del_gendisk() blocks in blk_mq_freeze_queue_wait() until every in-flight request has ended. ublk_stop_dev() cancelled the server's parked commands and aborted its own in-flight requests only after del_gendisk() had already returned, so neither side could ever make progress: the ublk server sits in io_uring_enter() waiting for CQEs that only ublk_cancel_dev() posts, while the control task sits inside del_gendisk() waiting for requests only ublk_abort_queue() can end. Move cancel-then-abort into ublk_stop_dev_unlocked(), before del_gendisk(), matching the sequence ublk_ch_release_work_fn() already uses. cancel_mutex is released before the abort loop runs (not held across it) so it doesn't needlessly block the server's own exit path for the loop's duration. Fixes: 71f28f3 ("ublk_drv: add io_uring based userspace block driver") Cc: stable@vger.kernel.org Signed-off-by: Bernd Schubert <bernd@bsbernd.com>
ublk_abort_queue() reads and disposes of a tag's OWNED_BY_SRV state without any lock, racing a live server on the same tag three ways: the io->cmd/io->req union is published in two separate, unlocked writes, so a concurrent reader can observe the flag set and the old io->cmd value still in the union; a request already handed back to the server via UBLK_IO_COMMIT_AND_FETCH_REQ or NEED_GET_DATA can be disposed of a second time by abort; and abort never clears the flag, so a retried abort pass reprocesses an already-ended request. Give each tag one lock-protected claim, the same io->lock the batch-IO commit path already uses for this: publish (auto buffer registration and the classic completion path) sets OWNED_BY_SRV and stores io->req as one locked step, and every place that hands a tag back to driver ownership (COMMIT_AND_FETCH_REQ, NEED_GET_DATA, ublk_abort_queue()) rechecks the flag and clears it under the same lock before touching the request. Whichever side loses the race backs off cleanly instead of running twice. Fixes: 64d1dc5 ("ublk: store request pointer in ublk_io") Cc: stable@vger.kernel.org Signed-off-by: Bernd Schubert <bernd@bsbernd.com>
ublk_queue_rq() hands a started request to the server via a task-work hop: ublk_queue_cmd() schedules the dispatch, ublk_dispatch_req() runs it and publishes OWNED_BY_SRV later. A single ublk_abort_queue() pass during that window sees a request that is started but not yet owned by the server, leaves it alone, and del_gendisk() then waits for a request nobody will ever end. ublk_abort_queue() now reports how many tags it had to leave alone using the same started-but-not-owned predicate ublk_cancel_cmd() already relies on. ublk_stop_dev_unlocked() quiesces the queue around the whole cancel/set_canceling/retry sequence, the same bracket ublk_force_abort_dev() already uses, and repeats the abort pass until nothing is pending before calling del_gendisk(). Quiescing blocks new submissions only, not the task-work completions the retry loop is waiting on, and keeps a fresh request from appearing after the loop's last zero-pending scan. A stuck loop warns once and doubles its warn interval on each further repeat, so it stays diagnosable instead of turning into a silent hang inside STOP_DEV. Fixes: 71f28f3 ("ublk_drv: add io_uring based userspace block driver") Cc: stable@vger.kernel.org Signed-off-by: Bernd Schubert <bernd@bsbernd.com>
__ublk_prep_compl_io_cmd(), ublk_auto_buf_io_setup() and ublk_batch_commit_io_check() all require the caller to already hold io->lock before touching UBLK_IO_FLAG_OWNED_BY_SRV/io->req, but only one of the three documented that in a comment and none enforced it. Add lockdep_assert_held() to each, matching how __ublk_acquire_fcmd() already asserts ubq->evts_lock, so a future caller that forgets to lock is caught at runtime instead of racing silently. Signed-off-by: Bernd Schubert <bernd@bsbernd.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.