[PATCH 81/98] nvmet: fix pc_ref use-after-free in preempt and abort

[Jens Axboe <axboe@kernel.dk>]

From: Chris Mason <mason@kernel.org>

With reservations enabled on a namespace, the target keeps a pc_ref per
controller, a percpu_ref counting that controller's I/O on the
namespace. "Preempt and abort" kills the pc_refs of the preempted
controllers and waits for the kill to be confirmed in
nvmet_pr_set_ctrl_to_abort(), then waits for the count to drop to zero
and resurrects them in nvmet_pr_do_abort(). Neither holds a reference
to the pc_ref, only pr_sem. nvmet_ctrl_destroy_pr() frees the pc_refs
under subsys->lock when the controller goes away, so a host that
registers a key, disconnects, and sends preempt and abort with that key
over a second connection frees a pc_ref that the abort is sleeping on:

    preempt and abort on C1            last put of C2
    nvmet_pr_set_ctrl_to_abort()
      percpu_ref_kill_and_confirm()
      wait_for_completion()
                                       nvmet_ctrl_destroy_pr()
                                         percpu_ref_exit()
                                         kfree(pc_ref)
    percpu_ref_switch_to_atomic_rcu()  /* use after free */

Put the pc_refs that an abort has killed on a per-namespace list.
nvmet_ctrl_destroy_pr() only flags a pc_ref on that list, and
nvmet_pr_do_abort() frees flagged ones once it's done with them. The
list and the flag are written under the xarray lock.

Fixes: 5a47c2080a73 ("nvmet: support reservation feature")
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Chaitanya Kulkarni <kch@nvidia.com>
Signed-off-by: Chris Mason <mason@kernel.org>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/nvme/target/nvmet.h |  5 ++
 drivers/nvme/target/pr.c    | 92 ++++++++++++++++++++++++++-----------
 2 files changed, 69 insertions(+), 28 deletions(-)

diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 5c2021b89ec4..563b966b8f75 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -88,6 +88,8 @@ struct nvmet_pr {
 	 */
 	struct semaphore	pr_sem;
 	struct list_head	registrant_list;
+	/* The pc_refs that a running "preempt and abort" has killed. */
+	struct list_head	abort_list;
 };
 
 struct nvmet_pr_per_ctrl_ref {
@@ -95,6 +97,9 @@ struct nvmet_pr_per_ctrl_ref {
 	struct completion	free_done;
 	struct completion	confirm_done;
 	uuid_t			hostid;
+	/* Both are written under the lock of ns->pr_per_ctrl_refs. */
+	struct list_head	abort_entry;
+	bool			ctrl_gone;
 };
 
 struct nvmet_ns {
diff --git a/drivers/nvme/target/pr.c b/drivers/nvme/target/pr.c
index 14eaa99f98fa..1c0d6cac1a6b 100644
--- a/drivers/nvme/target/pr.c
+++ b/drivers/nvme/target/pr.c
@@ -468,18 +468,54 @@ static void nvmet_pr_confirm_ns_pc_ref(struct percpu_ref *ref)
 	complete(&pc_ref->confirm_done);
 }
 
+static void nvmet_pr_free_pc_ref(struct nvmet_pr_per_ctrl_ref *pc_ref)
+{
+	percpu_ref_exit(&pc_ref->ref);
+	kfree(pc_ref);
+}
+
+/*
+ * The controller goes away. "Preempt and abort" sleeps on the pc_refs on its
+ * list, so leave the free of such a pc_ref to nvmet_pr_do_abort().
+ */
+static void nvmet_pr_erase_pc_ref(struct nvmet_ns *ns, unsigned long cntlid)
+{
+	struct nvmet_pr_per_ctrl_ref *pc_ref;
+
+	xa_lock(&ns->pr_per_ctrl_refs);
+	pc_ref = __xa_erase(&ns->pr_per_ctrl_refs, cntlid);
+	if (pc_ref && !list_empty(&pc_ref->abort_entry)) {
+		pc_ref->ctrl_gone = true;
+		pc_ref = NULL;
+	}
+	xa_unlock(&ns->pr_per_ctrl_refs);
+
+	if (pc_ref)
+		nvmet_pr_free_pc_ref(pc_ref);
+}
+
 static void nvmet_pr_set_ctrl_to_abort(struct nvmet_req *req, uuid_t *hostid)
 {
 	struct nvmet_pr_per_ctrl_ref *pc_ref;
 	struct nvmet_ns *ns = req->ns;
 	unsigned long idx;
 
+	xa_lock(&ns->pr_per_ctrl_refs);
 	xa_for_each(&ns->pr_per_ctrl_refs, idx, pc_ref) {
-		if (uuid_equal(&pc_ref->hostid, hostid)) {
-			percpu_ref_kill_and_confirm(&pc_ref->ref,
-						nvmet_pr_confirm_ns_pc_ref);
-			wait_for_completion(&pc_ref->confirm_done);
-		}
+		if (uuid_equal(&pc_ref->hostid, hostid) &&
+		    list_empty(&pc_ref->abort_entry))
+			list_add_tail(&pc_ref->abort_entry,
+				      &ns->pr.abort_list);
+	}
+	xa_unlock(&ns->pr_per_ctrl_refs);
+
+	list_for_each_entry(pc_ref, &ns->pr.abort_list, abort_entry) {
+		/* Killed for another host that this command preempts. */
+		if (percpu_ref_is_dying(&pc_ref->ref))
+			continue;
+		percpu_ref_kill_and_confirm(&pc_ref->ref,
+					    nvmet_pr_confirm_ns_pc_ref);
+		wait_for_completion(&pc_ref->confirm_done);
 	}
 }
 
@@ -618,20 +654,27 @@ static u16 nvmet_pr_preempt(struct nvmet_req *req,
 static void nvmet_pr_do_abort(struct work_struct *w)
 {
 	struct nvmet_req *req = container_of(w, struct nvmet_req, r.abort_work);
-	struct nvmet_pr_per_ctrl_ref *pc_ref;
+	struct nvmet_pr_per_ctrl_ref *pc_ref, *tmp;
 	struct nvmet_ns *ns = req->ns;
-	unsigned long idx;
+	bool ctrl_gone;
 
 	/*
 	 * The target does not support abort, just wait per-controller ref to 0.
 	 */
-	xa_for_each(&ns->pr_per_ctrl_refs, idx, pc_ref) {
-		if (percpu_ref_is_dying(&pc_ref->ref)) {
-			wait_for_completion(&pc_ref->free_done);
-			reinit_completion(&pc_ref->confirm_done);
-			reinit_completion(&pc_ref->free_done);
-			percpu_ref_resurrect(&pc_ref->ref);
-		}
+	list_for_each_entry_safe(pc_ref, tmp, &ns->pr.abort_list, abort_entry) {
+		wait_for_completion(&pc_ref->free_done);
+		reinit_completion(&pc_ref->confirm_done);
+		reinit_completion(&pc_ref->free_done);
+		percpu_ref_resurrect(&pc_ref->ref);
+
+		/* Off the list, the pc_ref belongs to its controller again. */
+		xa_lock(&ns->pr_per_ctrl_refs);
+		list_del_init(&pc_ref->abort_entry);
+		ctrl_gone = pc_ref->ctrl_gone;
+		xa_unlock(&ns->pr_per_ctrl_refs);
+
+		if (ctrl_gone)
+			nvmet_pr_free_pc_ref(pc_ref);
 	}
 
 	up(&ns->pr.pr_sem);
@@ -1052,6 +1095,8 @@ static int nvmet_pr_alloc_and_insert_pc_ref(struct nvmet_ns *ns,
 	init_completion(&pc_ref->free_done);
 	init_completion(&pc_ref->confirm_done);
 	uuid_copy(&pc_ref->hostid, hostid);
+	INIT_LIST_HEAD(&pc_ref->abort_entry);
+	pc_ref->ctrl_gone = false;
 
 	ret = xa_insert(&ns->pr_per_ctrl_refs, idx, pc_ref, GFP_KERNEL);
 	if (ret)
@@ -1067,7 +1112,6 @@ static int nvmet_pr_alloc_and_insert_pc_ref(struct nvmet_ns *ns,
 int nvmet_ctrl_init_pr(struct nvmet_ctrl *ctrl)
 {
 	struct nvmet_subsys *subsys = ctrl->subsys;
-	struct nvmet_pr_per_ctrl_ref *pc_ref;
 	struct nvmet_ns *ns = NULL;
 	unsigned long idx;
 	int ret;
@@ -1095,19 +1139,14 @@ int nvmet_ctrl_init_pr(struct nvmet_ctrl *ctrl)
 
 free_per_ctrl_refs:
 	nvmet_for_each_enabled_ns(&subsys->namespaces, idx, ns) {
-		if (ns->pr.enable) {
-			pc_ref = xa_erase(&ns->pr_per_ctrl_refs, ctrl->cntlid);
-			if (pc_ref)
-				percpu_ref_exit(&pc_ref->ref);
-			kfree(pc_ref);
-		}
+		if (ns->pr.enable)
+			nvmet_pr_erase_pc_ref(ns, ctrl->cntlid);
 	}
 	return ret;
 }
 
 void nvmet_ctrl_destroy_pr(struct nvmet_ctrl *ctrl)
 {
-	struct nvmet_pr_per_ctrl_ref *pc_ref;
 	struct nvmet_ns *ns;
 	unsigned long idx;
 
@@ -1115,12 +1154,8 @@ void nvmet_ctrl_destroy_pr(struct nvmet_ctrl *ctrl)
 	mutex_destroy(&ctrl->pr_log_mgr.lock);
 
 	nvmet_for_each_enabled_ns(&ctrl->subsys->namespaces, idx, ns) {
-		if (ns->pr.enable) {
-			pc_ref = xa_erase(&ns->pr_per_ctrl_refs, ctrl->cntlid);
-			if (pc_ref)
-				percpu_ref_exit(&pc_ref->ref);
-			kfree(pc_ref);
-		}
+		if (ns->pr.enable)
+			nvmet_pr_erase_pc_ref(ns, ctrl->cntlid);
 	}
 }
 
@@ -1136,6 +1171,7 @@ int nvmet_pr_init_ns(struct nvmet_ns *ns)
 	atomic_set(&ns->pr.generation, 0);
 	sema_init(&ns->pr.pr_sem, 1);
 	INIT_LIST_HEAD(&ns->pr.registrant_list);
+	INIT_LIST_HEAD(&ns->pr.abort_list);
 	ns->pr.notify_mask = 0;
 
 	xa_init(&ns->pr_per_ctrl_refs);
-- 
2.56.0