Re: [PATCH 09/19] nvmet: fix pc_ref use-after-free in preempt and abort

[Christoph Hellwig <hch@lst.de>]

Adding Guixin for the PR code.

On Thu, Oct 29, 2026 at 08:52:42AM -0700, Keith Busch wrote:
> 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>
> Signed-off-by: Keith Busch <kbusch@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---