[PATCH v2 2/2] nvmet: fix use-after-free in passthru I/O hotpath

[Nilay Shroff]

Hi Shin'ichiro,

On 11/2/26 4:19 PM, Shin'ichiro Kawasaki wrote:
> Hi, Nilay
> 
> I ran blktests with the kernel v7.4-rc1, and found the test case nvme/037 hangs
> when it runs for rdma transport and siw driver [1]. I bisected and found this
> patch as the commit 5e12d81874a1 is the trigger. If this han is a new issue, fix
> actions will be appreciated.

Thanks for report!

I tried recreating this issue on my system running nvme/037 multiple times but
I couldn't recreate it.

Looking through the code carefully, it seems that we release the passthru request
reference in __nvmet_req_complete() after ops->queue_repsonse() returns. However once
ops->queue_response() is called, the transport may complete and recycle the request
on another CPU. If the request is reused, its req->p.ref_held field may be reset
to false before __nvmet_req_complete() checks it in nvmet_put_passthru_ref().
Consequently, the passthru reference may not be released, leaving
wait_for_completion(&passthru->disable_done) in __nvmet_passthru_ctrl_disable()
blocked indefinitely.

Based on the above, I prepared a patch[1] to fix this issue. May I ask you to
validate this patch on your system? If this works for you then I'll send out a
formal patch.

[1]
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index a8171b88035c..714ac8add4c8 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -804,6 +804,7 @@ static void __nvmet_req_complete(struct nvmet_req *req, u16 status)
  {
  	struct nvmet_ns *ns = req->ns;
  	struct nvmet_pr_per_ctrl_ref *pc_ref = req->pc_ref;
+	struct nvmet_passthru *passthru = nvmet_passthru_ref_held(req);

  	if (!req->sq->sqhd_disabled)
  		nvmet_update_sq_head(req);
@@ -822,7 +823,8 @@ static void __nvmet_req_complete(struct nvmet_req *req, u16 status)
  	if (ns)
  		nvmet_put_namespace(ns);

-	nvmet_put_passthru_ref(req);
+	if (passthru)
+		nvmet_put_passthru_ref(passthru);
  }

  void nvmet_req_complete(struct nvmet_req *req, u16 status)
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index d7804bf91aaa..f8205fbfee6e 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -824,6 +824,15 @@ static inline struct nvmet_passthru *nvmet_subsys_passthru(
  	return subsys->passthru;
  }

+static inline struct nvmet_passthru *nvmet_passthru_ref_held(
+		struct nvmet_req *req)
+{
+	if (req->p.ref_held)
+		return nvmet_req_subsys(req)->passthru;
+
+	return NULL;
+}
+
  static inline bool nvmet_get_passthru_ref(struct nvmet_req *req)
  {
  	if (!percpu_ref_tryget_live(&nvmet_req_subsys(req)->passthru->ref))
@@ -833,13 +842,9 @@ static inline bool nvmet_get_passthru_ref(struct nvmet_req *req)
  	return true;
  }

-static inline void nvmet_put_passthru_ref(struct nvmet_req *req)
+static inline void nvmet_put_passthru_ref(struct nvmet_passthru *passthru)
  {
-	if (!req->p.ref_held)
-		return;
-
-	req->p.ref_held = false;
-	percpu_ref_put(&nvmet_req_subsys(req)->passthru->ref);
+	percpu_ref_put(&passthru->ref);
  }
  #else /* CONFIG_NVME_TARGET_PASSTHRU */
  static inline void nvmet_passthru_subsys_free(struct nvmet_subsys *subsys)
@@ -865,11 +870,16 @@ static inline struct nvmet_passthru *nvmet_subsys_passthru(
  {
  	return NULL;
  }
+static inline struct nvmet_passthru *nvmet_passthru_ref_held(
+		struct nvmet_req *req)
+{
+	return NULL;
+}
  static inline bool nvmet_get_passthru_ref(struct nvmet_req *req)
  {
  	return false;
  }
-static inline void nvmet_put_passthru_ref(struct nvmet_req *req)
+static inline void nvmet_put_passthru_ref(struct nvmet_passthru *passthru)
  {
  }
  #endif /* CONFIG_NVME_TARGET_PASSTHRU */