[PATCH 09/19] nvmet: fix pc_ref use-after-free in preempt and abort
Christoph Hellwig
hch at lst.de
Mon Nov 2 22:04:14 PST 2026
Adding Guixin for the PR code.
On Thu, Oct 29, 2026 at 08:52:42AM -0700, Keith Busch wrote:
> 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
---end quoted text---
More information about the Linux-nvme
mailing list