[PATCH 09/19] nvmet: fix pc_ref use-after-free in preempt and abort
Keith Busch
kbusch at meta.com
Thu Oct 29 08:52:42 PDT 2026
From: Chris Mason <mason at 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 at lst.de>
Cc: Sagi Grimberg <sagi at grimberg.me>
Cc: Chaitanya Kulkarni <kch at nvidia.com>
Signed-off-by: Chris Mason <mason at kernel.org>
Signed-off-by: Jens Axboe <axboe at kernel.dk>
Signed-off-by: Keith Busch <kbusch at kernel.org>
---
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 d7804bf91aaa8..a89b384c4bbc9 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 14eaa99f98fa3..1c0d6cac1a6b3 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.52.0
More information about the Linux-nvme
mailing list