nvme_ctrl_reset() freed every SQ/CQ right after nvme_ns_drain(), which only waits out requests on a per-namespace BlockBackend. That is safe as long as the guest first tore down I/O queues gracefully (Delete I/O SQ/CQ), since nvme_del_sq() already cancels and waits for anything left on a queue before freeing it.
A reset that happens without that graceful sequence first (e.g. an abrupt/asynchronous controller reset) can still have commands inflight on blk_aio_*. Freeing sq/cq before those complete leaves their completion callbacks (nvme_rw_cb() and friends) to run against already-freed NvmeRequest/NvmeSQueue/NvmeCQueue memory via nvme_enqueue_req_completion(), causing a use-after-free/segfault. Factor the cancel-and-wait loop already used by nvme_del_sq() into nvme_sq_cancel_inflight(), and run it over every queue in nvme_ctrl_reset() before the free loops. A pending Async Event Request has no aiocb (nvme_aer() parks it without issuing any block I/O), so drop it directly instead of asserting. Signed-off-by: Minwoo Im <[email protected]> --- hw/nvme/ctrl.c | 47 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index a67e1598891c..1482da57e4d9 100644 --- a/hw/nvme/ctrl.c +++ b/hw/nvme/ctrl.c @@ -4822,6 +4822,32 @@ static int nvme_init_sq_ioeventfd(NvmeSQueue *sq) return 0; } +/* + * Cancel every command still inflight on sq and block until each one's + * completion callback has run. Safe to call regardless of how the queue is + * going away (explicit Delete SQ, or a controller reset that never went + * through the guest's graceful queue teardown) since it guarantees no + * in-flight blk_aio_* callback can fire after sq/req memory is freed. + * + * A pending Async Event Request has no aiocb (nvme_aer() parks it without + * issuing any block I/O), so there is nothing to cancel; just drop it. + */ +static void nvme_sq_cancel_inflight(NvmeSQueue *sq, uint16_t status) +{ + NvmeRequest *r; + + while (!QTAILQ_EMPTY(&sq->out_req_list)) { + r = QTAILQ_FIRST(&sq->out_req_list); + r->status = status; + + if (r->aiocb) { + blk_aio_cancel(r->aiocb); + } else { + QTAILQ_REMOVE(&sq->out_req_list, r, entry); + } + } +} + static void nvme_free_sq(NvmeSQueue *sq, NvmeCtrl *n) { uint16_t offset = sq->sqid << 3; @@ -4856,14 +4882,7 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest *req) trace_pci_nvme_del_sq(qid); sq = n->sq[qid]; - while (!QTAILQ_EMPTY(&sq->out_req_list)) { - r = QTAILQ_FIRST(&sq->out_req_list); - assert(r->aiocb); - r->status = NVME_CMD_ABORT_SQ_DEL; - blk_aio_cancel(r->aiocb); - } - - assert(QTAILQ_EMPTY(&sq->out_req_list)); + nvme_sq_cancel_inflight(sq, NVME_CMD_ABORT_SQ_DEL); if (!nvme_check_cqid(n, sq->cqid)) { cq = n->cq[sq->cqid]; @@ -8022,6 +8041,18 @@ static void nvme_ctrl_reset(NvmeCtrl *n, NvmeResetType rst) nvme_ns_drain(ns); } + /* + * Cancel and wait out every inflight command on every queue first. A + * reset is not required to be preceded by the guest's graceful + * Delete I/O SQ/CQ sequence, so sq/cq must not be freed below while a + * blk_aio_* completion for them could still be in flight. + */ + for (i = 0; i < n->num_queues; i++) { + if (n->sq[i] != NULL) { + nvme_sq_cancel_inflight(n->sq[i], NVME_CMD_ABORT_SQ_DEL); + } + } + for (i = 0; i < n->num_queues; i++) { if (n->sq[i] != NULL) { nvme_free_sq(n->sq[i], n); -- 2.34.1
