With UBLK_F_AUTO_BUF_REG, invalid sqe->addr can fail after ublk_fill_io_cmd() has set UBLK_IO_FLAG_ACTIVE. The uring_cmd is completed while the tag stays active, which can hang teardown. Split validation from buffer apply so the check has no side effects, then take the uring_cmd and store the already-validated buffer. Apply the same order in FETCH so io->buf is not written before __ublk_fetch() state checks. Fixes: 52460dda3a77 ("ublk: move auto buffer register handling into one dedicated helper") Suggested-by: Caleb Sander Mateos Signed-off-by: Yang Xiuwei --- drivers/block/ublk_drv.c | 74 +++++++++++++++++++++------------------- 1 file changed, 38 insertions(+), 36 deletions(-) diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index 4ca6ec738c93..041bd3ea023f 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -3075,18 +3075,19 @@ static inline int ublk_check_cmd_op(u32 cmd_op) return 0; } -static inline int ublk_set_auto_buf_reg(struct ublk_io *io, struct io_uring_cmd *cmd) +/* Must run before ublk_fill_io_cmd() / __ublk_fetch(). */ +static inline int ublk_validate_io_buf(const struct ublk_device *ub, + struct io_uring_cmd *cmd, + struct ublk_auto_buf_reg *buf) { - struct ublk_auto_buf_reg buf; - - buf = ublk_sqe_addr_to_auto_buf_reg(READ_ONCE(cmd->sqe->addr)); + if (!ublk_dev_support_auto_buf_reg(ub)) + return 0; - if (buf.reserved0 || buf.reserved1) + *buf = ublk_sqe_addr_to_auto_buf_reg(READ_ONCE(cmd->sqe->addr)); + if (buf->reserved0 || buf->reserved1) return -EINVAL; - - if (buf.flags & ~UBLK_AUTO_BUF_REG_F_MASK) + if (buf->flags & ~UBLK_AUTO_BUF_REG_F_MASK) return -EINVAL; - io->buf.auto_reg = buf; return 0; } @@ -3107,17 +3108,25 @@ static void ublk_clear_auto_buf_reg(struct ublk_io *io, * responsibility for unregistering the buffer, otherwise * this ublk request gets stuck. */ - if (io->buf_ctx_handle == io_uring_cmd_ctx_handle(cmd)) + if (buf_idx && + io->buf_ctx_handle == io_uring_cmd_ctx_handle(cmd)) *buf_idx = io->buf.auto_reg.index; } } -static int ublk_handle_auto_buf_reg(struct ublk_io *io, - struct io_uring_cmd *cmd, - u16 *buf_idx) +static inline void ublk_apply_io_buf(const struct ublk_device *ub, + struct ublk_io *io, + struct io_uring_cmd *cmd, + unsigned long buf_addr, + const struct ublk_auto_buf_reg *auto_buf, + u16 *buf_idx) { - ublk_clear_auto_buf_reg(io, cmd, buf_idx); - return ublk_set_auto_buf_reg(io, cmd); + if (ublk_dev_support_auto_buf_reg(ub)) { + ublk_clear_auto_buf_reg(io, cmd, buf_idx); + io->buf.auto_reg = *auto_buf; + } else { + io->buf.addr = buf_addr; + } } /* Once we return, `io->req` can't be used any more */ @@ -3134,18 +3143,6 @@ ublk_fill_io_cmd(struct ublk_io *io, struct io_uring_cmd *cmd) return req; } -static inline int -ublk_config_io_buf(const struct ublk_device *ub, struct ublk_io *io, - struct io_uring_cmd *cmd, unsigned long buf_addr, - u16 *buf_idx) -{ - if (ublk_dev_support_auto_buf_reg(ub)) - return ublk_handle_auto_buf_reg(io, cmd, buf_idx); - - io->buf.addr = buf_addr; - return 0; -} - static inline void ublk_prep_cancel(struct io_uring_cmd *cmd, unsigned int issue_flags, struct ublk_queue *ubq, unsigned int tag) @@ -3286,6 +3283,7 @@ static int __ublk_fetch(struct io_uring_cmd *cmd, struct ublk_device *ub, static int ublk_fetch(struct io_uring_cmd *cmd, struct ublk_device *ub, struct ublk_io *io, __u64 buf_addr, u16 q_id) { + struct ublk_auto_buf_reg auto_buf; int ret; /* @@ -3294,11 +3292,13 @@ static int ublk_fetch(struct io_uring_cmd *cmd, struct ublk_device *ub, * FETCH, so it is fine even for IO_URING_F_NONBLOCK. */ mutex_lock(&ub->mutex); - ret = __ublk_fetch(cmd, ub, io, q_id); - if (!ret) - ret = ublk_config_io_buf(ub, io, cmd, buf_addr, NULL); + ret = ublk_validate_io_buf(ub, cmd, &auto_buf); if (!ret) + ret = __ublk_fetch(cmd, ub, io, q_id); + if (!ret) { + ublk_apply_io_buf(ub, io, cmd, buf_addr, &auto_buf, NULL); ublk_mark_io_ready(ub, q_id, io); + } mutex_unlock(&ub->mutex); return ret; } @@ -3441,13 +3441,18 @@ static int ublk_ch_uring_cmd_local(struct io_uring_cmd *cmd, case UBLK_IO_REGISTER_IO_BUF: return ublk_daemon_register_io_buf(cmd, ub, q_id, tag, io, addr, issue_flags); - case UBLK_IO_COMMIT_AND_FETCH_REQ: + case UBLK_IO_COMMIT_AND_FETCH_REQ: { + struct ublk_auto_buf_reg auto_buf; + ret = ublk_check_commit_and_fetch(ub, io, addr); + if (ret) + goto out; + ret = ublk_validate_io_buf(ub, cmd, &auto_buf); if (ret) goto out; io->res = result; req = ublk_fill_io_cmd(io, cmd); - ret = ublk_config_io_buf(ub, io, cmd, addr, &buf_idx); + ublk_apply_io_buf(ub, io, cmd, addr, &auto_buf, &buf_idx); if (buf_idx != UBLK_INVALID_BUF_IDX) io_buffer_unregister_bvec(cmd, buf_idx, issue_flags); compl = ublk_need_complete_req(ub, io); @@ -3456,10 +3461,8 @@ static int ublk_ch_uring_cmd_local(struct io_uring_cmd *cmd, req->__sector = addr; if (compl) __ublk_complete_rq(req, io, ublk_dev_need_map_io(ub), NULL); - - if (ret) - goto out; break; + } case UBLK_IO_NEED_GET_DATA: /* * ublk_get_data() may fail and fallback to requeue, so keep @@ -3467,8 +3470,7 @@ static int ublk_ch_uring_cmd_local(struct io_uring_cmd *cmd, * request */ req = ublk_fill_io_cmd(io, cmd); - ret = ublk_config_io_buf(ub, io, cmd, addr, NULL); - WARN_ON_ONCE(ret); + io->buf.addr = addr; if (likely(ublk_get_data(ubq, io, req))) { __ublk_prep_compl_io_cmd(io, req); return UBLK_IO_RES_OK; -- 2.25.1