[PATCH 0/2] nvmet: passthru cleanup and fixup I/O hotpath

[PATCH 0/2] nvmet: passthru cleanup and fixup I/O hotpath

[Nilay Shroff]

Hi,

This series addresses a race in the passthru I/O hotpath where
concurrently disabling the passthru controller while I/Os are in flight
can result in a use-after-free bug.

We were able to reproduce this bug with NVMe/TCP configured and by
injecting an additional delay into the target-side I/O processing code.
If the passthru controller is disabled while an I/O is still in flight,
it results in the following kernel crash:

BUG: Kernel NULL pointer dereference on read at 0x00000030
[...]
CPU: 9 UID: 0 PID: 4143 Comm: kworker/9:5H Kdump: loaded Not tainted 7.3.0-rc3+ #14 PREEMPT 
Hardware name: IBM,9080-HEX Power11 (architected) 0x820200 0xf000007 of:IBM,FW1110.00 (NH1110_031) hv:phyp pSeries
Workqueue: nvmet_tcp_wq nvmet_tcp_io_work [nvmet_tcp]
[...]
NIP [c0080000156d8fe0] nvmet_passthru_execute_cmd+0x58/0x45c [nvmet]
LR [c0080000156d8fc4] nvmet_passthru_execute_cmd+0x3c/0x45c [nvmet]
Call Trace:
 nvmet_passthru_execute_cmd+0x3c/0x45c [nvmet] (unreliable)
 nvmet_tcp_done_recv_pdu+0x2d0/0x718 [nvmet_tcp]
 nvmet_tcp_try_recv_pdu+0x29c/0x348 [nvmet_tcp]
 nvmet_tcp_io_work+0xe8/0x838 [nvmet_tcp]
 process_one_work+0x224/0x5ec
 worker_thread+0x1f8/0x3e8
 kthread+0x178/0x1ac
 start_kernel_thread+0x14/0x18

There are two patches in this series. The first patch groups all
passthru-related fields into a separate struct nvmet_passthru, which
makes the code easier to maintain and reason about. The second patch
fixes the kernel bug described above.

As usual, code review comments and feedback are most welcome!

Thanks!

Nilay Shroff (2):
  nvmet: introduce struct nvmet_passthru
  nvmet: fix use-after-free error in passthru I/O hotpath

 drivers/nvme/target/configfs.c | 45 +++++++++--------
 drivers/nvme/target/core.c     |  6 ++-
 drivers/nvme/target/nvmet.h    | 41 +++++++++++++---
 drivers/nvme/target/passthru.c | 88 +++++++++++++++++++++++++---------
 4 files changed, 130 insertions(+), 50 deletions(-)

-- 
2.53.0

[PATCH 1/2] nvmet: introduce struct nvmet_passthru

[Nilay Shroff]

Currently, passthru parameters are grouped under struct nvmet_subsys.
Since passthru can be configured through configfs and all passthru
parameters are exposed under a separate configfs subdirectory, group
these parameters under a new struct nvmet_passthru.

Grouping passthru-specific parameters in a separate structure makes
the code easier to maintain and reason about.

There are no functional changes intended.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/configfs.c | 45 ++++++++++++++++++++--------------
 drivers/nvme/target/core.c     |  2 +-
 drivers/nvme/target/nvmet.h    | 18 ++++++++------
 drivers/nvme/target/passthru.c | 44 ++++++++++++++++++---------------
 4 files changed, 62 insertions(+), 47 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index b03b5d1c2dc3..6f060ee7e15b 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -904,22 +904,23 @@ static const struct config_item_type nvmet_namespaces_type = {
 static ssize_t nvmet_passthru_device_path_show(struct config_item *item,
 		char *page)
 {
-	struct nvmet_subsys *subsys = to_subsys(item->ci_parent);
+	struct nvmet_passthru *passthru = &to_subsys(item->ci_parent)->passthru;
 
-	return snprintf(page, PAGE_SIZE, "%s\n", subsys->passthru_ctrl_path);
+	return snprintf(page, PAGE_SIZE, "%s\n", passthru->ctrl_path);
 }
 
 static ssize_t nvmet_passthru_device_path_store(struct config_item *item,
 		const char *page, size_t count)
 {
 	struct nvmet_subsys *subsys = to_subsys(item->ci_parent);
+	struct nvmet_passthru *passthru = &subsys->passthru;
 	size_t len;
 	int ret;
 
 	mutex_lock(&subsys->lock);
 
 	ret = -EBUSY;
-	if (subsys->passthru_ctrl)
+	if (passthru->ctrl)
 		goto out_unlock;
 
 	ret = -EINVAL;
@@ -927,10 +928,10 @@ static ssize_t nvmet_passthru_device_path_store(struct config_item *item,
 	if (!len)
 		goto out_unlock;
 
-	kfree(subsys->passthru_ctrl_path);
+	kfree(passthru->ctrl_path);
 	ret = -ENOMEM;
-	subsys->passthru_ctrl_path = kstrndup(page, len, GFP_KERNEL);
-	if (!subsys->passthru_ctrl_path)
+	passthru->ctrl_path = kstrndup(page, len, GFP_KERNEL);
+	if (!passthru->ctrl_path)
 		goto out_unlock;
 
 	mutex_unlock(&subsys->lock);
@@ -945,9 +946,9 @@ CONFIGFS_ATTR(nvmet_passthru_, device_path);
 static ssize_t nvmet_passthru_enable_show(struct config_item *item,
 		char *page)
 {
-	struct nvmet_subsys *subsys = to_subsys(item->ci_parent);
+	struct nvmet_passthru *passthru = &to_subsys(item->ci_parent)->passthru;
 
-	return sprintf(page, "%d\n", subsys->passthru_ctrl ? 1 : 0);
+	return sprintf(page, "%d\n", passthru->ctrl ? 1 : 0);
 }
 
 static ssize_t nvmet_passthru_enable_store(struct config_item *item,
@@ -972,18 +973,20 @@ CONFIGFS_ATTR(nvmet_passthru_, enable);
 static ssize_t nvmet_passthru_admin_timeout_show(struct config_item *item,
 		char *page)
 {
-	return sprintf(page, "%u\n", to_subsys(item->ci_parent)->admin_timeout);
+	struct nvmet_passthru *passthru = &to_subsys(item->ci_parent)->passthru;
+
+	return sprintf(page, "%u\n", passthru->admin_timeout);
 }
 
 static ssize_t nvmet_passthru_admin_timeout_store(struct config_item *item,
 		const char *page, size_t count)
 {
-	struct nvmet_subsys *subsys = to_subsys(item->ci_parent);
+	struct nvmet_passthru *passthru = &to_subsys(item->ci_parent)->passthru;
 	unsigned int timeout;
 
 	if (kstrtouint(page, 0, &timeout))
 		return -EINVAL;
-	subsys->admin_timeout = timeout;
+	passthru->admin_timeout = timeout;
 	return count;
 }
 CONFIGFS_ATTR(nvmet_passthru_, admin_timeout);
@@ -991,18 +994,20 @@ CONFIGFS_ATTR(nvmet_passthru_, admin_timeout);
 static ssize_t nvmet_passthru_io_timeout_show(struct config_item *item,
 		char *page)
 {
-	return sprintf(page, "%u\n", to_subsys(item->ci_parent)->io_timeout);
+	struct nvmet_passthru *passthru = &to_subsys(item->ci_parent)->passthru;
+
+	return sprintf(page, "%u\n", passthru->io_timeout);
 }
 
 static ssize_t nvmet_passthru_io_timeout_store(struct config_item *item,
 		const char *page, size_t count)
 {
-	struct nvmet_subsys *subsys = to_subsys(item->ci_parent);
+	struct nvmet_passthru *passthru = &to_subsys(item->ci_parent)->passthru;
 	unsigned int timeout;
 
 	if (kstrtouint(page, 0, &timeout))
 		return -EINVAL;
-	subsys->io_timeout = timeout;
+	passthru->io_timeout = timeout;
 	return count;
 }
 CONFIGFS_ATTR(nvmet_passthru_, io_timeout);
@@ -1010,18 +1015,20 @@ CONFIGFS_ATTR(nvmet_passthru_, io_timeout);
 static ssize_t nvmet_passthru_clear_ids_show(struct config_item *item,
 		char *page)
 {
-	return sprintf(page, "%u\n", to_subsys(item->ci_parent)->clear_ids);
+	struct nvmet_passthru *passthru = &to_subsys(item->ci_parent)->passthru;
+
+	return sprintf(page, "%u\n", passthru->clear_ids);
 }
 
 static ssize_t nvmet_passthru_clear_ids_store(struct config_item *item,
 		const char *page, size_t count)
 {
-	struct nvmet_subsys *subsys = to_subsys(item->ci_parent);
+	struct nvmet_passthru *passthru = &to_subsys(item->ci_parent)->passthru;
 	unsigned int clear_ids;
 
 	if (kstrtouint(page, 0, &clear_ids))
 		return -EINVAL;
-	subsys->clear_ids = clear_ids;
+	passthru->clear_ids = clear_ids;
 	return count;
 }
 CONFIGFS_ATTR(nvmet_passthru_, clear_ids);
@@ -1042,9 +1049,9 @@ static const struct config_item_type nvmet_passthru_type = {
 
 static void nvmet_add_passthru_group(struct nvmet_subsys *subsys)
 {
-	config_group_init_type_name(&subsys->passthru_group,
+	config_group_init_type_name(&subsys->passthru.group,
 				    "passthru", &nvmet_passthru_type);
-	configfs_add_default_group(&subsys->passthru_group,
+	configfs_add_default_group(&subsys->passthru.group,
 				   &subsys->group);
 }
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 8eea0a504308..9ab07dbe8cbe 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1646,7 +1646,7 @@ struct nvmet_ctrl *nvmet_alloc_ctrl(struct nvmet_alloc_ctrl_args *args)
 #ifdef CONFIG_NVME_TARGET_PASSTHRU
 	/* By default, set loop targets to clear IDS by default */
 	if (ctrl->port->disc_addr.trtype == NVMF_TRTYPE_LOOP)
-		subsys->clear_ids = 1;
+		subsys->passthru.clear_ids = 1;
 #endif
 
 	INIT_WORK(&ctrl->async_event_work, nvmet_async_event_work);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 162e2fdd848e..8f5dccee7d26 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -319,6 +319,15 @@ struct nvmet_ctrl {
 	struct nvmet_pr_log_mgr pr_log_mgr;
 };
 
+struct nvmet_passthru {
+	struct nvme_ctrl	*ctrl;
+	char			*ctrl_path;
+	struct config_group	group;
+	unsigned int		admin_timeout;
+	unsigned int		io_timeout;
+	unsigned int		clear_ids;
+};
+
 struct nvmet_subsys {
 	enum nvme_subsys_type	type;
 
@@ -358,12 +367,7 @@ struct nvmet_subsys {
 	char			*firmware_rev;
 
 #ifdef CONFIG_NVME_TARGET_PASSTHRU
-	struct nvme_ctrl	*passthru_ctrl;
-	char			*passthru_ctrl_path;
-	struct config_group	passthru_group;
-	unsigned int		admin_timeout;
-	unsigned int		io_timeout;
-	unsigned int		clear_ids;
+	struct nvmet_passthru	passthru;
 #endif /* CONFIG_NVME_TARGET_PASSTHRU */
 
 #ifdef CONFIG_BLK_DEV_ZONED
@@ -793,7 +797,7 @@ u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req);
 u16 nvmet_parse_passthru_io_cmd(struct nvmet_req *req);
 static inline bool nvmet_is_passthru_subsys(struct nvmet_subsys *subsys)
 {
-	return subsys->passthru_ctrl;
+	return subsys->passthru.ctrl;
 }
 #else /* CONFIG_NVME_TARGET_PASSTHRU */
 static inline void nvmet_passthru_subsys_free(struct nvmet_subsys *subsys)
diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index fa6527c537e2..81ac220da8ba 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -26,7 +26,7 @@ void nvmet_passthrough_override_cap(struct nvmet_ctrl *ctrl)
 	 * Multiple command set support can only be declared if the underlying
 	 * controller actually supports it.
 	 */
-	if (!nvme_multi_css(ctrl->subsys->passthru_ctrl))
+	if (!nvme_multi_css(ctrl->subsys->passthru.ctrl))
 		ctrl->cap &= ~(1ULL << 43);
 }
 
@@ -39,7 +39,7 @@ static u16 nvmet_passthru_override_id_descs(struct nvmet_req *req)
 	void *data;
 	u8 csi;
 
-	if (!ctrl->subsys->clear_ids)
+	if (!ctrl->subsys->passthru.clear_ids)
 		return status;
 
 	data = kzalloc(NVME_IDENTIFY_DATA_SIZE, GFP_KERNEL);
@@ -89,7 +89,7 @@ static u16 nvmet_passthru_override_id_descs(struct nvmet_req *req)
 static u16 nvmet_passthru_override_id_ctrl(struct nvmet_req *req)
 {
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
-	struct nvme_ctrl *pctrl = ctrl->subsys->passthru_ctrl;
+	struct nvme_ctrl *pctrl = ctrl->subsys->passthru.ctrl;
 	u16 status = NVME_SC_SUCCESS;
 	struct nvme_id_ctrl *id;
 	unsigned int max_hw_sectors;
@@ -208,7 +208,7 @@ static u16 nvmet_passthru_override_id_ns(struct nvmet_req *req)
 	 */
 	id->mc = 0;
 
-	if (req->sq->ctrl->subsys->clear_ids) {
+	if (req->sq->ctrl->subsys->passthru.clear_ids) {
 		memset(id->nguid, 0, NVME_NIDT_NGUID_LEN);
 		memset(id->eui64, 0, NVME_NIDT_EUI64_LEN);
 	}
@@ -305,7 +305,8 @@ static int nvmet_passthru_map_sg(struct nvmet_req *req, struct request *rq)
 
 static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 {
-	struct nvme_ctrl *ctrl = nvmet_req_subsys(req)->passthru_ctrl;
+	struct nvmet_passthru *passthru = &nvmet_req_subsys(req)->passthru;
+	struct nvme_ctrl *ctrl = passthru->ctrl;
 	struct request_queue *q = ctrl->admin_q;
 	struct nvme_ns *ns = NULL;
 	struct request *rq = NULL;
@@ -325,9 +326,9 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 		}
 
 		q = ns->queue;
-		timeout = nvmet_req_subsys(req)->io_timeout;
+		timeout = passthru->io_timeout;
 	} else {
-		timeout = nvmet_req_subsys(req)->admin_timeout;
+		timeout = passthru->admin_timeout;
 	}
 
 	rq = blk_mq_alloc_request(q, nvme_req_op(req->cmd), 0);
@@ -386,7 +387,7 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
  */
 static void nvmet_passthru_set_host_behaviour(struct nvmet_req *req)
 {
-	struct nvme_ctrl *ctrl = nvmet_req_subsys(req)->passthru_ctrl;
+	struct nvme_ctrl *ctrl = nvmet_req_subsys(req)->passthru.ctrl;
 	struct nvme_feat_host_behavior *host;
 	u16 status = NVME_SC_INTERNAL;
 	int ret;
@@ -586,15 +587,16 @@ u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
 
 int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys)
 {
+	struct nvmet_passthru *passthru = &subsys->passthru;
 	struct nvme_ctrl *ctrl;
 	struct file *file;
 	int ret = -EINVAL;
 	void *old;
 
 	mutex_lock(&subsys->lock);
-	if (!subsys->passthru_ctrl_path)
+	if (!passthru->ctrl_path)
 		goto out_unlock;
-	if (subsys->passthru_ctrl)
+	if (passthru->ctrl)
 		goto out_unlock;
 
 	if (subsys->nr_namespaces) {
@@ -602,7 +604,7 @@ int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys)
 		goto out_unlock;
 	}
 
-	file = filp_open(subsys->passthru_ctrl_path, O_RDWR, 0);
+	file = filp_open(passthru->ctrl_path, O_RDWR, 0);
 	if (IS_ERR(file)) {
 		ret = PTR_ERR(file);
 		goto out_unlock;
@@ -611,7 +613,7 @@ int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys)
 	ctrl = nvme_ctrl_from_file(file);
 	if (!ctrl) {
 		pr_err("failed to open nvme controller %s\n",
-		       subsys->passthru_ctrl_path);
+		       passthru->ctrl_path);
 
 		goto out_put_file;
 	}
@@ -626,7 +628,7 @@ int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys)
 	if (old)
 		goto out_put_file;
 
-	subsys->passthru_ctrl = ctrl;
+	passthru->ctrl = ctrl;
 	subsys->ver = ctrl->vs;
 
 	if (subsys->ver < NVME_VS(1, 2, 1)) {
@@ -636,7 +638,7 @@ int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys)
 		subsys->ver = NVME_VS(1, 2, 1);
 	}
 	nvme_get_ctrl(ctrl);
-	__module_get(subsys->passthru_ctrl->ops->module);
+	__module_get(passthru->ctrl->ops->module);
 	ret = 0;
 
 out_put_file:
@@ -648,12 +650,14 @@ int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys)
 
 static void __nvmet_passthru_ctrl_disable(struct nvmet_subsys *subsys)
 {
-	if (subsys->passthru_ctrl) {
-		xa_erase(&passthru_subsystems, subsys->passthru_ctrl->instance);
-		module_put(subsys->passthru_ctrl->ops->module);
-		nvme_put_ctrl(subsys->passthru_ctrl);
+	struct nvmet_passthru *passthru = &subsys->passthru;
+
+	if (passthru->ctrl) {
+		xa_erase(&passthru_subsystems, passthru->ctrl->instance);
+		module_put(passthru->ctrl->ops->module);
+		nvme_put_ctrl(passthru->ctrl);
 	}
-	subsys->passthru_ctrl = NULL;
+	passthru->ctrl = NULL;
 	subsys->ver = NVMET_DEFAULT_VS;
 }
 
@@ -669,5 +673,5 @@ void nvmet_passthru_subsys_free(struct nvmet_subsys *subsys)
 	mutex_lock(&subsys->lock);
 	__nvmet_passthru_ctrl_disable(subsys);
 	mutex_unlock(&subsys->lock);
-	kfree(subsys->passthru_ctrl_path);
+	kfree(subsys->passthru.ctrl_path);
 }
-- 
2.53.0

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

[Nilay Shroff]

Concurrently disabling a passthru controller while passthru I/Os are
in flight can potentially result in a use-after-free. Introduce a
percpu refcount, an atomic flag, and an nvmet request flag to track
reference held by passthru I/Os and protect the passthru controller
lifetime.

When enabling the passthru controller, initialize the percpu refcount
and set the enabled flag. Each passthru I/O acquires a reference before
entering the passthru hot path and sets the nvmet request flag to record
that the reference is held. The reference is released when the I/O
completes and the request flag is set.

When disabling the passthru controller, clear the enabled flag, kill the
percpu refcount, and wait for all in-flight I/Os to release their
references before releasing the passthru controller. Concurrent disable
attempts are serialized by the atomic enabled flag: only the first
disable attempt observes the flag set and proceeds, while subsequent
attempts bail out.

Once disabling starts, new I/Os fail to acquire a live reference and
therefore cannot enter the passthru hot path.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/core.c     |  4 +++
 drivers/nvme/target/nvmet.h    | 23 ++++++++++++++
 drivers/nvme/target/passthru.c | 58 ++++++++++++++++++++++++++++------
 3 files changed, 75 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 9ab07dbe8cbe..0870977eccf6 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -819,6 +819,9 @@ static void __nvmet_req_complete(struct nvmet_req *req, u16 status)
 		nvmet_pr_put_ns_pc_ref(pc_ref);
 	if (ns)
 		nvmet_put_namespace(ns);
+
+	if (req->p.ref_held)
+		nvmet_put_passthru_ref(req);
 }
 
 void nvmet_req_complete(struct nvmet_req *req, u16 status)
@@ -1195,6 +1198,7 @@ bool nvmet_req_init(struct nvmet_req *req, struct nvmet_sq *sq,
 	req->error_loc = NVMET_NO_ERROR_LOC;
 	req->error_slba = 0;
 	req->pc_ref = NULL;
+	req->p.ref_held = false;
 
 	/* no support for fused commands yet */
 	if (unlikely(flags & (NVME_CMD_FUSE_FIRST | NVME_CMD_FUSE_SECOND))) {
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 8f5dccee7d26..2953102a90bd 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -320,6 +320,11 @@ struct nvmet_ctrl {
 };
 
 struct nvmet_passthru {
+	struct percpu_ref	ref;
+	struct completion	disable_done;
+#define NVMET_PASSTHRU_ENABLED	0
+	unsigned long		flags;
+
 	struct nvme_ctrl	*ctrl;
 	char			*ctrl_path;
 	struct config_group	group;
@@ -478,6 +483,7 @@ struct nvmet_req {
 			struct request		*rq;
 			struct work_struct      work;
 			bool			use_workqueue;
+			bool			ref_held;
 		} p;
 #ifdef CONFIG_BLK_DEV_ZONED
 		struct {
@@ -799,6 +805,16 @@ static inline bool nvmet_is_passthru_subsys(struct nvmet_subsys *subsys)
 {
 	return subsys->passthru.ctrl;
 }
+
+static inline bool nvmet_get_passthru_ref(struct nvmet_req *req)
+{
+	return percpu_ref_tryget_live(&nvmet_req_subsys(req)->passthru.ref);
+}
+
+static inline void nvmet_put_passthru_ref(struct nvmet_req *req)
+{
+	percpu_ref_put(&nvmet_req_subsys(req)->passthru.ref);
+}
 #else /* CONFIG_NVME_TARGET_PASSTHRU */
 static inline void nvmet_passthru_subsys_free(struct nvmet_subsys *subsys)
 {
@@ -818,6 +834,13 @@ static inline bool nvmet_is_passthru_subsys(struct nvmet_subsys *subsys)
 {
 	return NULL;
 }
+static inline bool nvmet_get_passthru_ref(struct nvmet_req *req)
+{
+	return NULL;
+}
+static inline void nvmet_put_passthru_ref(struct nvmet_req *req)
+{
+}
 #endif /* CONFIG_NVME_TARGET_PASSTHRU */
 
 static inline bool nvmet_is_passthru_req(struct nvmet_req *req)
diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index 81ac220da8ba..c18b1dda2a18 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -305,9 +305,9 @@ static int nvmet_passthru_map_sg(struct nvmet_req *req, struct request *rq)
 
 static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 {
-	struct nvmet_passthru *passthru = &nvmet_req_subsys(req)->passthru;
-	struct nvme_ctrl *ctrl = passthru->ctrl;
-	struct request_queue *q = ctrl->admin_q;
+	struct nvmet_passthru *passthru;
+	struct nvme_ctrl *ctrl;
+	struct request_queue *q;
 	struct nvme_ns *ns = NULL;
 	struct request *rq = NULL;
 	unsigned int timeout;
@@ -315,6 +315,16 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 	u16 status;
 	int ret;
 
+	req->p.ref_held = nvmet_get_passthru_ref(req);
+	if (!req->p.ref_held) {
+		status = NVME_SC_INTERNAL | NVME_STATUS_DNR;
+		goto out;
+	}
+
+	passthru = &nvmet_req_subsys(req)->passthru;
+	ctrl = passthru->ctrl;
+	q = ctrl->admin_q;
+
 	if (likely(req->sq->qid != 0)) {
 		u32 nsid = le32_to_cpu(req->cmd->common.nsid);
 
@@ -387,11 +397,18 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
  */
 static void nvmet_passthru_set_host_behaviour(struct nvmet_req *req)
 {
-	struct nvme_ctrl *ctrl = nvmet_req_subsys(req)->passthru.ctrl;
+	struct nvme_ctrl *ctrl;
 	struct nvme_feat_host_behavior *host;
 	u16 status = NVME_SC_INTERNAL;
 	int ret;
 
+	req->p.ref_held = nvmet_get_passthru_ref(req);
+	if (!req->p.ref_held) {
+		status |= NVME_STATUS_DNR;
+		goto out_complete_req;
+	}
+	ctrl = nvmet_req_subsys(req)->passthru.ctrl;
+
 	host = kzalloc(sizeof(*host) * 2, GFP_KERNEL);
 	if (!host)
 		goto out_complete_req;
@@ -585,6 +602,14 @@ u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
 	}
 }
 
+static void nvmet_release_passthru_ctrl(struct percpu_ref *ref)
+{
+	struct nvmet_passthru *passthru = container_of(ref,
+				struct nvmet_passthru, ref);
+
+	complete(&passthru->disable_done);
+}
+
 int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys)
 {
 	struct nvmet_passthru *passthru = &subsys->passthru;
@@ -628,9 +653,15 @@ int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys)
 	if (old)
 		goto out_put_file;
 
+	ret = percpu_ref_init(&passthru->ref, nvmet_release_passthru_ctrl,
+			0, GFP_KERNEL);
+	if (ret) {
+		xa_erase(&passthru_subsystems, ctrl->instance);
+		goto out_put_file;
+	}
+	init_completion(&passthru->disable_done);
 	passthru->ctrl = ctrl;
 	subsys->ver = ctrl->vs;
-
 	if (subsys->ver < NVME_VS(1, 2, 1)) {
 		pr_warn("nvme controller version is too old: %llu.%llu.%llu, advertising 1.2.1\n",
 			NVME_MAJOR(subsys->ver), NVME_MINOR(subsys->ver),
@@ -639,6 +670,7 @@ int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys)
 	}
 	nvme_get_ctrl(ctrl);
 	__module_get(passthru->ctrl->ops->module);
+	set_bit(NVMET_PASSTHRU_ENABLED, &passthru->flags);
 	ret = 0;
 
 out_put_file:
@@ -652,11 +684,17 @@ static void __nvmet_passthru_ctrl_disable(struct nvmet_subsys *subsys)
 {
 	struct nvmet_passthru *passthru = &subsys->passthru;
 
-	if (passthru->ctrl) {
-		xa_erase(&passthru_subsystems, passthru->ctrl->instance);
-		module_put(passthru->ctrl->ops->module);
-		nvme_put_ctrl(passthru->ctrl);
-	}
+	if (!test_and_clear_bit(NVMET_PASSTHRU_ENABLED, &passthru->flags))
+		return;
+
+	percpu_ref_kill(&passthru->ref);
+	wait_for_completion(&passthru->disable_done);
+	percpu_ref_exit(&passthru->ref);
+
+	xa_erase(&passthru_subsystems, passthru->ctrl->instance);
+	module_put(passthru->ctrl->ops->module);
+	nvme_put_ctrl(passthru->ctrl);
+
 	passthru->ctrl = NULL;
 	subsys->ver = NVMET_DEFAULT_VS;
 }
-- 
2.53.0

Re: [PATCH 1/2] nvmet: introduce struct nvmet_passthru

[Christoph Hellwig]

On Fri, Sep 25, 2026 at 04:51:09PM +0530, Nilay Shroff wrote:
> Currently, passthru parameters are grouped under struct nvmet_subsys.
> Since passthru can be configured through configfs and all passthru
> parameters are exposed under a separate configfs subdirectory, group
> these parameters under a new struct nvmet_passthru.
> 
> Grouping passthru-specific parameters in a separate structure makes
> the code easier to maintain and reason about.
> 
> There are no functional changes intended.

If the fields are move out anyway, should they be a separate, dynamically
allocated object so that non-passthrough users don't pay for the memory
allocation?

Re: [PATCH 1/2] nvmet: introduce struct nvmet_passthru

[Nilay Shroff]

On 9/28/26 10:40 AM, Christoph Hellwig wrote:
> On Fri, Sep 25, 2026 at 04:51:09PM +0530, Nilay Shroff wrote:
>> Currently, passthru parameters are grouped under struct nvmet_subsys.
>> Since passthru can be configured through configfs and all passthru
>> parameters are exposed under a separate configfs subdirectory, group
>> these parameters under a new struct nvmet_passthru.
>>
>> Grouping passthru-specific parameters in a separate structure makes
>> the code easier to maintain and reason about.
>>
>> There are no functional changes intended.
> 
> If the fields are move out anyway, should they be a separate, dynamically
> allocated object so that non-passthrough users don't pay for the memory
> allocation?
> 
Yes makes sense. I'll change it to the dynamically allocated object in next
revision.

Thanks,
--Nilay