STOP_DEV cancels the fetched commands after dropping ub->mutex, so START_DEV and FETCH can run in between: STOP_DEV START_DEV / FETCH lock ub->mutex ublk_stop_dev_unlocked() (not started: does nothing) unlock ub->mutex START_DEV goes live, schedules scan FETCH publishes a new command cancel_work_sync(scan) cancels the new disk's scan ublk_cancel_dev() takes the new command: NULL io->cmd on a queue which isn't canceling A START_DEV after STOP_DEV also goes live over the canceled commands. Hold a reference on the server's /dev/ublkcN while canceling: STOP_DEV lock ub->mutex ublk_stop_dev_unlocked() cancel_work_sync(scan) server attached: take a reference on its file, set STOPPING unlock ub->mutex ublk_cancel_dev() drop the reference - STOPPING makes FETCH and PREP get UBLK_IO_RES_ABORT and START_DEV -EBUSY; a START_DEV or END_USER_RECOVERY waiting for the device to get ready wakes up too; the reset in the server's release clears it - the release can't run before the reference is dropped, so no new server can attach, and the cancel only meets this server's commands - the cancel still runs after the unlock: io_uring_cmd_done() may take uring_lock, under which FETCH takes ub->mutex ublk_ch_open() saves the file in ->ch_file and ublk_ch_release() clears it, under cancel_mutex. The release runs after the last reference is gone, so take ours with file_ref_get(). Drop it with __fput_sync(): if it is the last one, fput() would defer the release to this task, which DEL_DEV then makes wait for the device to be freed. A server which opened the device but hasn't fetched is stopped too. The cancel callback's io->cmd check allows NULL: the cancel may take the command meanwhile. Fixes: 85248d670b71 ("ublk: move ublk_cancel_dev() out of ub->mutex") Cc: stable@vger.kernel.org Reported-by: Josef Bacik Closes: https://lore.kernel.org/linux-block/20260928-b4-ublk-cancel-stop-v1-0-4a4360232a46@toxicpanda.com/ Signed-off-by: Ming Lei --- drivers/block/ublk_drv.c | 85 +++++++++++++++++++++++++++++++++++----- 1 file changed, 76 insertions(+), 9 deletions(-) diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index 162a4d1a2e2a..f57d544c1da2 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -321,6 +321,8 @@ struct ublk_device { #define UB_STATE_OPEN 0 #define UB_STATE_USED 1 #define UB_STATE_DELETED 2 +/* STOP_DEV canceled the server's commands, until its release */ +#define UB_STATE_STOPPING 3 unsigned long state; int ub_number; @@ -334,6 +336,8 @@ struct ublk_device { u16 nr_queue_ready; bool unprivileged_daemons; struct mutex cancel_mutex; + /* the open /dev/ublkcN, protected by cancel_mutex */ + struct file *ch_file; /* * A cancel started in this FETCH round. Set by ublk_set_canceling(), * cleared only by ublk_reset_ch_dev() when a new round starts. While @@ -2406,6 +2410,9 @@ static int ublk_ch_open(struct inode *inode, struct file *filp) return -EBUSY; filp->private_data = ub; ub->ublksrv_tgid = current->tgid; + mutex_lock(&ub->cancel_mutex); + ub->ch_file = filp; + mutex_unlock(&ub->cancel_mutex); return 0; } @@ -2432,6 +2439,7 @@ static void ublk_reset_ch_dev(struct ublk_device *ub) ub->nr_queue_ready = 0; ub->unprivileged_daemons = false; ub->ublksrv_tgid = -1; + clear_bit(UB_STATE_STOPPING, &ub->state); } static struct gendisk *ublk_get_disk(struct ublk_device *ub) @@ -2635,6 +2643,9 @@ static int ublk_ch_release(struct inode *inode, struct file *filp) { struct ublk_device *ub = filp->private_data; + mutex_lock(&ub->cancel_mutex); + ub->ch_file = NULL; + mutex_unlock(&ub->cancel_mutex); /* * Grab ublk device reference, so it won't be gone until we are * really released from work function. @@ -2896,6 +2907,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd, { struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd); struct ublk_queue *ubq = pdu->ubq; + struct io_uring_cmd *cur; struct task_struct *task; struct ublk_io *io; @@ -2912,7 +2924,9 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd, ublk_start_cancel(ubq->dev); - WARN_ON_ONCE(io->cmd != cmd); + /* NULL if STOP_DEV's cancel took it meanwhile */ + cur = READ_ONCE(io->cmd); + WARN_ON_ONCE(cur && cur != cmd); ublk_cancel_cmd(ubq, pdu->tag, issue_flags); } @@ -3023,13 +3037,54 @@ static void ublk_stop_dev_unlocked(struct ublk_device *ub) put_disk(disk); } +static struct file *ublk_get_ch_file(struct ublk_device *ub) +{ + struct file *file; + + mutex_lock(&ub->cancel_mutex); + file = ub->ch_file; + if (file && !file_ref_get(&file->f_ref)) + file = NULL; + mutex_unlock(&ub->cancel_mutex); + return file; +} + static void ublk_stop_dev(struct ublk_device *ub) { + struct file *file; + + /* + * FETCH, PREP and START_DEV take ub->mutex. If a server has + * /dev/ublkcN open, set STOPPING, which turns it away, and hold a + * reference on the file: the server's release, whose reset clears + * STOPPING and lets a new server attach, can't run before we drop it. + * So the cancel can run after the unlock and only meets this server's + * commands; it has to: io_uring_cmd_done() may take uring_lock, under + * which FETCH takes ub->mutex. + */ mutex_lock(&ub->mutex); ublk_stop_dev_unlocked(ub); - mutex_unlock(&ub->mutex); cancel_work_sync(&ub->partition_scan_work); + file = ublk_get_ch_file(ub); + if (file) { + /* + * The server has to close /dev/ublkcN before this device can + * be started again: the reset in its release clears STOPPING. + */ + set_bit(UB_STATE_STOPPING, &ub->state); + /* for wake_up_var() below, see wake_up_bit() */ + smp_mb__after_atomic(); + } + mutex_unlock(&ub->mutex); + + /* no server: nothing to cancel */ + if (!file) + return; + + /* wake a START_DEV waiting for the device to get ready */ + wake_up_var(&ub->nr_queue_ready); ublk_cancel_dev(ub); + __fput_sync(file); } static void ublk_reset_io_flags(struct ublk_queue *ubq, struct ublk_io *io) @@ -3295,6 +3350,9 @@ static int ublk_check_fetch_buf(const struct ublk_device *ub, __u64 buf_addr) static int __ublk_fetch(struct io_uring_cmd *cmd, struct ublk_device *ub, struct ublk_io *io, u16 q_id) { + if (test_bit(UB_STATE_STOPPING, &ub->state)) + return UBLK_IO_RES_ABORT; + /* UBLK_IO_FETCH_REQ is only allowed before dev is setup */ if (ublk_dev_ready(ub)) return -EBUSY; @@ -4473,22 +4531,27 @@ static bool ublk_validate_user_pid(struct ublk_device *ub, pid_t ublksrv_pid) return ub->ublksrv_tgid == ublksrv_pid; } +static bool ublk_dev_ready_or_stopping(const struct ublk_device *ub) +{ + return ublk_dev_ready(ub) || test_bit(UB_STATE_STOPPING, &ub->state); +} + /* - * Wait until all queues have fetched their I/O commands, and return with - * ub->mutex held and readiness guaranteed. The queues stay canceling if - * this round saw a cancel, see ublk_queue_reset_io_flags(). Ready may - * regress between wakeup and mutex_lock() (F_BATCH UNPREP, daemon death), - * so re-check it under the mutex and wait again. + * Wait until all queues have fetched their I/O commands, or STOP_DEV set + * UB_STATE_STOPPING, and return with ub->mutex held. The queues stay + * canceling if this round saw a cancel, see ublk_queue_reset_io_flags(). + * Ready may regress between wakeup and mutex_lock() (F_BATCH UNPREP, + * daemon death), so re-check it under the mutex and wait again. */ static int ublk_wait_dev_ready_and_lock(struct ublk_device *ub) { while (true) { if (wait_var_event_interruptible(&ub->nr_queue_ready, - ublk_dev_ready(ub))) + ublk_dev_ready_or_stopping(ub))) return -EINTR; mutex_lock(&ub->mutex); - if (ublk_dev_ready(ub)) + if (ublk_dev_ready_or_stopping(ub)) return 0; mutex_unlock(&ub->mutex); } @@ -4588,6 +4651,10 @@ static int ublk_ctrl_start_dev(struct ublk_device *ub, ret = -EEXIST; goto out_unlock; } + if (test_bit(UB_STATE_STOPPING, &ub->state)) { + ret = -EBUSY; + goto out_unlock; + } disk = blk_mq_alloc_disk(&ub->tag_set, &lim, NULL); if (IS_ERR(disk)) { -- 2.55.0