[PATCH 96/98] nvmet: take sq->ref in nvmet_sq_fatal_error()

[Jens Axboe <axboe@kernel.dk>]

From: Chris Mason <mason@kernel.org>

nvmet_sq_destroy() kills sq->ref, waits for the commands to drop it,
then puts the controller and clears sq->ctrl. So sq->ctrl may only be
used while holding sq->ref. nvmet_sq_fatal_error() reads it without,
and its transport callers hold nothing either: in nvmet-rdma the
completion handlers run until the QP is drained, which
nvmet_rdma_release_queue_work() does only after nvmet_sq_destroy(), and
in nvmet-tcp io_work can run between the two cancels of the release
work. A short capsule or a failed completion that races the disconnect
then dereferences a NULL sq->ctrl, or locks ctrl->lock and queues
fatal_err_work in a freed controller. The host needs nothing but a
Connect, reproduced over rxe with a sleep added to widen the window.

Take sq->ref with percpu_ref_tryget_live() around the body. If the ref
is already dead the queue is going away and the error is dropped, which
is the mitigation: nothing can tell a call that would have finished in
time from one that wouldn't. tryget_live after percpu_ref_exit() is
fine, it sees __PERCPU_REF_DEAD and touches nothing freed, and
nvmet_req_init() already relies on that. nvmet_auth_fatal_error() runs
in a command that holds the ref, so it calls the body directly.

Fixes: 14c862dbb0a0 ("nvmet-rdma: drain the queue-pair just before freeing it")
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Chaitanya Kulkarni <kch@nvidia.com>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Chris Mason <mason@kernel.org>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/nvme/target/core.c             | 21 ++++++++++++++++++++-
 drivers/nvme/target/fabrics-cmd-auth.c |  3 ++-
 drivers/nvme/target/nvmet.h            |  1 +
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index b78a4d4eb838..19f96f1b7e74 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1833,8 +1833,11 @@ EXPORT_SYMBOL_GPL(nvmet_ctrl_fatal_error);
  * A fatal error on one queue.  The Connect command of an I/O queue needs no
  * secret, so until the queue has authenticated it may be anybody's: do not
  * let it take down the controller of the host whose NQN it has named.
+ *
+ * The caller holds sq->ref, as a command that is being executed does: until it
+ * is put, nvmet_sq_destroy() waits, and does not clear sq->ctrl.
  */
-void nvmet_sq_fatal_error(struct nvmet_sq *sq)
+void __nvmet_sq_fatal_error(struct nvmet_sq *sq)
 {
 	struct nvmet_ctrl *ctrl = sq->ctrl;
 
@@ -1843,6 +1846,22 @@ void nvmet_sq_fatal_error(struct nvmet_sq *sq)
 	else
 		nvmet_ctrl_fatal_error(ctrl);
 }
+
+/*
+ * A transport calls this with no reference of its own, concurrently with
+ * nvmet_sq_destroy(), which puts the controller and clears sq->ctrl.  As in
+ * nvmet_req_init(), the call can come after percpu_ref_exit():
+ * percpu_ref_tryget_live() then sees that the ref is dead and touches nothing
+ * that is freed.
+ */
+void nvmet_sq_fatal_error(struct nvmet_sq *sq)
+{
+	if (!percpu_ref_tryget_live(&sq->ref))
+		return;
+
+	__nvmet_sq_fatal_error(sq);
+	percpu_ref_put(&sq->ref);
+}
 EXPORT_SYMBOL_GPL(nvmet_sq_fatal_error);
 
 ssize_t nvmet_ctrl_host_traddr(struct nvmet_ctrl *ctrl,
diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
index cb6be285b3c4..020a989fe263 100644
--- a/drivers/nvme/target/fabrics-cmd-auth.c
+++ b/drivers/nvme/target/fabrics-cmd-auth.c
@@ -33,12 +33,13 @@ static void nvmet_auth_expired_work(struct work_struct *work)
 	mutex_unlock(&sq->auth_lock);
 }
 
+/* Called from a command that is being executed, which holds sq->ref. */
 static void nvmet_auth_fatal_error(struct nvmet_sq *sq, bool passed)
 {
 	if (passed)
 		nvmet_ctrl_fatal_error(sq->ctrl);
 	else
-		nvmet_sq_fatal_error(sq);
+		__nvmet_sq_fatal_error(sq);
 }
 
 void nvmet_auth_sq_init(struct nvmet_sq *sq)
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 29105e2a7022..3519537705f8 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -631,6 +631,7 @@ void nvmet_sq_destroy(struct nvmet_sq *sq);
 int nvmet_sq_init(struct nvmet_sq *sq, struct nvmet_cq *cq);
 
 void nvmet_ctrl_fatal_error(struct nvmet_ctrl *ctrl);
+void __nvmet_sq_fatal_error(struct nvmet_sq *sq);
 void nvmet_sq_fatal_error(struct nvmet_sq *sq);
 
 void nvmet_update_cc(struct nvmet_ctrl *ctrl, u32 new);
-- 
2.56.0