[PATCH 00/19] nvme patch pile

[PATCH 00/19] nvme patch pile

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Jens sent a large patch series of various proposed fixes in parts of the
block stack. The original series can be found here:

  https://lore.kernel.org/linux-block/20261028164415.1262013-1-axboe@kernel.dk/

I want the nvme patches considered separately and applied through the
nvme subsystem tree, so this is following up on that: this series pulls
out many of the nvme patches from that series, and I'm sending this to
the linux-nvme mailing list for further visibility.

This does not include every nvme patch from the original: some proposals
have been applied already, some had duplicate or alternate fixes from
other contributors, some no longer apply, and others had feedback that
indicate rejecting the proposal for something else. But this is the
remaining patches to be considered that still cleanly apply, and
includes received reviews.

This is based on nvme-7.4 merged with block-7.4.

Chris Mason (11):
  nvmet: fix pc_ref use-after-free in preempt and abort
  nvmet: move preempt and abort work off nvmet-wq
  nvmet-rdma: treat a failed connect as a disconnect
  nvmet-rdma: disconnect only established queues
  nvmet-rdma: take a device off the list last
  nvmet-rdma: queue the release work under the mutex
  nvmet-rdma: disconnect a queue on device removal
  nvmet-rdma: reject queues larger than max_cqe
  nvmet-rdma: don't run the wait list recursively
  nvmet-rdma: fix a lost run of the wait list
  nvmet: wait only for executing I/O in preempt and abort

Josef Bacik (8):
  nvme-loop: delete the controller if a reset fails
  nvmet-tcp: ask for the TLS record type on every receive
  nvme-fabrics: reconnect if the association is gone
  nvmet-auth: keep the old keys if the new ones fail
  nvmet: accept I/O queues on the controller's port only
  nvmet: check the host NQN against the TLS PSK identity
  nvmet-tcp: let the accept work come to an end
  nvme-tcp: start error recovery when a send fails

 drivers/nvme/host/fabrics.c       |  15 +++-
 drivers/nvme/host/tcp.c           |   2 +
 drivers/nvme/target/auth.c        |  69 ++++++---------
 drivers/nvme/target/core.c        |  18 +++-
 drivers/nvme/target/fabrics-cmd.c |  33 +++++++
 drivers/nvme/target/loop.c        |   5 +-
 drivers/nvme/target/nvmet.h       |  10 ++-
 drivers/nvme/target/pr.c          | 140 +++++++++++++++++++++++-------
 drivers/nvme/target/rdma.c        | 126 ++++++++++++++++++++-------
 drivers/nvme/target/tcp.c         |  13 ++-
 10 files changed, 318 insertions(+), 113 deletions(-)

-- 
2.52.0

[PATCH 01/19] nvme-loop: delete the controller if a reset fails

[Keith Busch]

From: Josef Bacik <josef@toxicpanda.com>

If the reconnect of a reset fails, nvme_loop_reset_ctrl_work() tears the
controller down by hand with nvme_uninit_ctrl(). Since that was split
from nvme_stop_ctrl() it no longer removes the namespaces, which hold
references to the controller. A reset while the target no longer accepts
the host (allow_any_host 0) is enough to get there: a sysfs reset hangs
in kernfs_drain(), an ioctl reset leaves a controller stuck in
CONNECTING with its block devices, the cntlid can't be reused, and
module unload deletes the controller a second time and hits a
use-after-free.

Do what the other fabrics drivers do and call nvme_delete_ctrl(). The
delete path quiesces the admin queue and only unquiesces it if the queue
is live, which it isn't after a failed reset, so unquiesce it
unconditionally before destroying it so a stuck request can't block the
freeze.

Fixes: d09f2b45f346 ("nvme: split nvme_uninit_ctrl into stop and uninit")
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Chaitanya Kulkarni <kch@nvidia.com>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/loop.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index e579a307798b9..80b67608eb155 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -420,6 +420,8 @@ static void nvme_loop_shutdown_ctrl(struct nvme_loop_ctrl *ctrl)
 static void nvme_loop_delete_ctrl_host(struct nvme_ctrl *ctrl)
 {
 	nvme_loop_shutdown_ctrl(to_loop_ctrl(ctrl));
+	/* Left quiesced if a failed reset has left no admin queue behind */
+	nvme_unquiesce_admin_queue(ctrl);
 	nvme_remove_admin_tag_set(ctrl);
 }
 
@@ -483,9 +485,8 @@ static void nvme_loop_reset_ctrl_work(struct work_struct *work)
 	nvme_cancel_admin_tagset(&ctrl->ctrl);
 	nvme_loop_destroy_admin_queue(ctrl);
 out_disable:
-	nvme_remove_admin_tag_set(&ctrl->ctrl);
 	dev_warn(ctrl->ctrl.device, "Removing after reset failure\n");
-	nvme_uninit_ctrl(&ctrl->ctrl);
+	nvme_delete_ctrl(&ctrl->ctrl);
 }
 
 static const struct nvme_ctrl_ops nvme_loop_ctrl_ops = {
-- 
2.52.0

[PATCH 02/19] nvmet-tcp: ask for the TLS record type on every receive

[Keith Busch]

From: Josef Bacik <josef@toxicpanda.com>

nvmet_tcp_alloc_cmd() points recv_msg.msg_control at recv_cbuf once.
The first receive that stores a record type there advances msg_control
and leaves msg_controllen at 0 for the lifetime of the command. From
then on nvmet_tcp_try_recv_data() sees the record type of that first
receive: if it was an alert, all later data is dropped as alerts and the
queue stalls, if it was data, a later alert can't be reported and
tls_sw_recvmsg() fails with -EIO.

Set up the control buffer before each receive, like the other receive
paths do.

Fixes: a1c5dd8355b1 ("nvmet-tcp: control messages for recvmsg()")
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Hannes Reinecke <hare@kernel.org>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/tcp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 8092ce4914b62..158e422851a4b 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -1302,6 +1302,11 @@ static int nvmet_tcp_try_recv_data(struct nvmet_tcp_queue *queue)
 	int len, ret;
 
 	while (msg_data_left(&cmd->recv_msg)) {
+		if (queue->tls_pskid) {
+			/* The last receive has used it up */
+			cmd->recv_msg.msg_control = cmd->recv_cbuf;
+			cmd->recv_msg.msg_controllen = sizeof(cmd->recv_cbuf);
+		}
 		len = sock_recvmsg(cmd->queue->sock, &cmd->recv_msg,
 			cmd->recv_msg.msg_flags);
 		if (len <= 0)
-- 
2.52.0

[PATCH 03/19] nvme-fabrics: reconnect if the association is gone

[Keith Busch]

From: Josef Bacik <josef@toxicpanda.com>

A reconnect connects the admin queue, gets a controller ID, and connects
the I/O queues with it. If the controller goes away in between, the
Connect of an I/O queue fails with Connect Invalid Parameters pointing
at the cntlid, with DNR set:

  nvme nvme0: Connect Invalid Data Parameter, cntlid: 1
  nvme nvme0: Failed reconnect attempt 1/600
  nvme nvme0: Removing controller (16770)...

nvmf_should_reconnect() honours DNR and the host deletes the controller
with 599 attempts to go. DNR is right in that resending that command is
pointless, but the next attempt would create a new association with a
new cntlid. This is not a verdict on the host like a failed admin
Connect is.

Treat it as a lost association and return -ENOTCONN, so the retries are
bounded by ctrl_loss_tmo like after any other loss.

Fixes: adfde7ed0b30 ("nvme-fabrics: short-circuit reconnect retries")
Cc: Keith Busch <kbusch@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/fabrics.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 3d7ad86c1955f..aeff98e16d6d5 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -542,8 +542,19 @@ int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid)
 			NVME_SUBMIT_RESERVED |
 			NVME_SUBMIT_NOWAIT);
 	if (ret) {
-		nvmf_log_connect_error(ctrl, ret, le32_to_cpu(res.u32),
-				       &cmd, data);
+		result = le32_to_cpu(res.u32);
+		nvmf_log_connect_error(ctrl, ret, result, &cmd, data);
+		/*
+		 * The controller ID we got from the admin queue's Connect is
+		 * not known any more: the association has gone since. DNR is
+		 * right about this command and says nothing about the next
+		 * association. Don't let nvmf_should_reconnect() see it.
+		 */
+		if (ret > 0 &&
+		    (ret & NVME_SCT_SC_MASK) == NVME_SC_CONNECT_INVALID_PARAM &&
+		    result >> 16 && (result & 0xffff) ==
+				offsetof(struct nvmf_connect_data, cntlid))
+			ret = -ENOTCONN;
 		goto out_free_data;
 	}
 	result = le32_to_cpu(res.u32);
-- 
2.52.0

[PATCH 04/19] nvmet-auth: keep the old keys if the new ones fail

[Keith Busch]

From: Josef Bacik <josef@toxicpanda.com>

AUTH_Negotiate on the admin queue has nvmet_setup_auth() reload the
host's keys, and it frees the old host key first. If decoding the new
secret fails, or an allocation does, the controller is left without a
host key, which means no authentication at all: nvmet_check_auth_status()
passes everything and I/O queue Connects no longer ask for it. configfs
accepts any string starting with "DHHC-1:" and a digit, so one typo
while changing a secret opens the namespaces to any connected admin
queue.

Decode the new keys first and only then replace the old ones.

Fixes: db1312dd9548 ("nvmet: implement basic In-Band Authentication")
Cc: Hannes Reinecke <hare@suse.de>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Hannes Reinecke <hare@kernel.org>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/auth.c | 69 ++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 41 deletions(-)

diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
index 574025730834c..ccba84b1344e0 100644
--- a/drivers/nvme/target/auth.c
+++ b/drivers/nvme/target/auth.c
@@ -135,6 +135,7 @@ u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq, bool reset)
 	int ret = 0;
 	struct nvmet_host_link *p;
 	struct nvmet_host *host = NULL;
+	struct nvme_dhchap_key *host_key, *ctrl_key = NULL;
 
 	down_read(&nvmet_config_sem);
 	if (nvmet_is_disc_subsys(ctrl->subsys))
@@ -172,55 +173,41 @@ u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq, bool reset)
 		goto out_unlock;
 	}
 
-	if (host->dhchap_hash_id == ctrl->shash_id) {
-		pr_debug("Re-use existing hash ID %d\n",
-			 ctrl->shash_id);
-	} else {
-		ctrl->shash_id = host->dhchap_hash_id;
-	}
-
-	/* Skip the 'DHHC-1:XX:' prefix */
-	nvme_auth_free_key(ctrl->host_key);
-	ctrl->host_key = nvme_auth_extract_key(host->dhchap_secret + 10,
-					       host->dhchap_key_hash);
-	if (IS_ERR(ctrl->host_key)) {
+	/*
+	 * Skip the 'DHHC-1:XX:' prefix. Keep the old keys until the new ones
+	 * are there: a controller without a host key needs no authentication.
+	 */
+	host_key = nvme_auth_extract_key(host->dhchap_secret + 10,
+					 host->dhchap_key_hash);
+	if (IS_ERR(host_key)) {
 		ret = NVME_AUTH_DHCHAP_FAILURE_NOT_USABLE;
-		ctrl->host_key = NULL;
-		goto out_free_hash;
+		goto out_unlock;
 	}
+	if (host->dhchap_ctrl_secret) {
+		ctrl_key = nvme_auth_extract_key(host->dhchap_ctrl_secret + 10,
+						 host->dhchap_ctrl_key_hash);
+		if (IS_ERR(ctrl_key)) {
+			nvme_auth_free_key(host_key);
+			ret = NVME_AUTH_DHCHAP_FAILURE_NOT_USABLE;
+			goto out_unlock;
+		}
+	}
+	nvme_auth_free_key(ctrl->host_key);
+	ctrl->host_key = host_key;
+	nvme_auth_free_key(ctrl->ctrl_key);
+	ctrl->ctrl_key = ctrl_key;
+	ctrl->shash_id = host->dhchap_hash_id;
 #ifdef CONFIG_NVME_TARGET_AUTH_DEBUG
 	pr_debug("%s: using hash %s key %*ph\n", __func__,
 		 ctrl->host_key->hash > 0 ?
 		 nvme_auth_hmac_name(ctrl->host_key->hash) : "none",
 		 (int)ctrl->host_key->len, ctrl->host_key->key);
+	if (ctrl->ctrl_key)
+		pr_debug("%s: using ctrl hash %s key %*ph\n", __func__,
+			 ctrl->ctrl_key->hash > 0 ?
+			 nvme_auth_hmac_name(ctrl->ctrl_key->hash) : "none",
+			 (int)ctrl->ctrl_key->len, ctrl->ctrl_key->key);
 #endif
-	nvme_auth_free_key(ctrl->ctrl_key);
-	if (!host->dhchap_ctrl_secret) {
-		ctrl->ctrl_key = NULL;
-		goto out_unlock;
-	}
-
-	ctrl->ctrl_key = nvme_auth_extract_key(host->dhchap_ctrl_secret + 10,
-					       host->dhchap_ctrl_key_hash);
-	if (IS_ERR(ctrl->ctrl_key)) {
-		ret = NVME_AUTH_DHCHAP_FAILURE_NOT_USABLE;
-		ctrl->ctrl_key = NULL;
-		goto out_free_hash;
-	}
-#ifdef CONFIG_NVME_TARGET_AUTH_DEBUG
-	pr_debug("%s: using ctrl hash %s key %*ph\n", __func__,
-		 ctrl->ctrl_key->hash > 0 ?
-		 nvme_auth_hmac_name(ctrl->ctrl_key->hash) : "none",
-		 (int)ctrl->ctrl_key->len, ctrl->ctrl_key->key);
-#endif
-out_free_hash:
-	if (ret) {
-		if (ctrl->host_key) {
-			nvme_auth_free_key(ctrl->host_key);
-			ctrl->host_key = NULL;
-		}
-		ctrl->shash_id = 0;
-	}
 out_unlock:
 	up_read(&nvmet_config_sem);
 
-- 
2.52.0

[PATCH 05/19] nvmet: accept I/O queues on the controller's port only

[Keith Busch]

From: Josef Bacik <josef@toxicpanda.com>

nvmet_ctrl_find_get() looks the subsystem up through the port of the
request, but then takes the controller from the subsystem list whatever
port it was created on. A controller belongs to one port and one
transport: ctrl->ops is that of the admin queue, and both
->install_queue() and ->delete_ctrl() assume the queues are their own.
So an I/O queue arriving on another port of the same subsystem, possibly
on another transport, is installed into a controller it has no part in,
nvmet_tcp_install_queue() writes to it as a TCP queue, and an error on
it deletes that controller. The Connect comes before any authentication.

Skip controllers of other ports, and compare the transport too since a
port's transport can be changed while it's disabled.

Fixes: a07b4970f464 ("nvmet: add a generic NVMe target")
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Hannes Reinecke <hare@kernel.org>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index a8171b88035c5..27b27c83ff831 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1512,6 +1512,11 @@ struct nvmet_ctrl *nvmet_ctrl_find_get(const char *subsysnqn,
 				pr_warn("hostnqn mismatch.\n");
 				continue;
 			}
+			/* All queues of a controller come in by its port. */
+			if (ctrl->port != req->port || ctrl->ops != req->ops) {
+				pr_warn("port mismatch.\n");
+				continue;
+			}
 			if (!kref_get_unless_zero(&ctrl->ref))
 				continue;
 
-- 
2.52.0

[PATCH 06/19] nvmet: check the host NQN against the TLS PSK identity

[Keith Busch]

From: Josef Bacik <josef@toxicpanda.com>

The identity of a TLS PSK names the host and subsystem the key was
generated for. The target remembers which key a handshake ended with,
but never compares it with the host NQN in the Connect that follows. So
a host holding any PSK in the port's keyring can connect as another
host, sees what that host may see, and isn't asked for DH-HMAC-CHAP
since TLS queues skip it. It can add an I/O queue to that host's TLS
controller the same way.

Refuse a Connect whose host NQN isn't the one in the PSK identity of the
queue. The subsystem NQN isn't compared, a discovery subsystem goes by
two names.

Fixes: 675b453e0241 ("nvmet-tcp: enable TLS handshake upcall")
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Chaitanya Kulkarni <kch@nvidia.com>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/fabrics-cmd.c | 33 +++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index fc626c2fd2c92..b950000696c2a 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -267,6 +267,30 @@ static u32 nvmet_connect_result(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq)
 		(needs_auth ? NVME_CONNECT_AUTHREQ_ATR : 0);
 }
 
+/*
+ * The identity of a TLS PSK names the host that the key was made for:
+ * "NVMe<version><type><hash> <host NQN> <subsystem NQN> ...".
+ * Whoever has proven to hold the key is that host, and no other one.
+ */
+static u16 nvmet_check_tls_host(struct nvmet_sq *sq, const char *hostnqn)
+{
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+	size_t len = strlen(hostnqn);
+	const char *id;
+
+	if (!sq->tls_key)
+		return 0;
+
+	id = strchr(sq->tls_key->description, ' ');
+	if (!id || strncmp(id + 1, hostnqn, len) || id[len + 1] != ' ') {
+		pr_warn("TLS PSK %08x does not belong to host %s\n",
+			key_serial(sq->tls_key), hostnqn);
+		return NVME_SC_CONNECT_INVALID_HOST | NVME_STATUS_DNR;
+	}
+#endif
+	return 0;
+}
+
 static void nvmet_execute_admin_connect(struct nvmet_req *req)
 {
 	struct nvmf_connect_command *c = &req->cmd->connect;
@@ -312,6 +336,10 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
 	d->subsysnqn[NVMF_NQN_FIELD_LEN - 1] = '\0';
 	d->hostnqn[NVMF_NQN_FIELD_LEN - 1] = '\0';
 
+	args.status = nvmet_check_tls_host(req->sq, d->hostnqn);
+	if (args.status)
+		goto out;
+
 	args.subsysnqn = d->subsysnqn;
 	args.hostnqn = d->hostnqn;
 	args.hostid = &d->hostid;
@@ -366,6 +394,11 @@ static void nvmet_execute_io_connect(struct nvmet_req *req)
 
 	d->subsysnqn[NVMF_NQN_FIELD_LEN - 1] = '\0';
 	d->hostnqn[NVMF_NQN_FIELD_LEN - 1] = '\0';
+
+	status = nvmet_check_tls_host(req->sq, d->hostnqn);
+	if (status)
+		goto out;
+
 	ctrl = nvmet_ctrl_find_get(d->subsysnqn, d->hostnqn,
 				   le16_to_cpu(d->cntlid), req);
 	if (!ctrl) {
-- 
2.52.0

[PATCH 07/19] nvmet-tcp: let the accept work come to an end

[Keith Busch]

From: Josef Bacik <josef@toxicpanda.com>

nvmet_tcp_accept_work() loops until there's nothing left to accept, and
anyone opening and closing connections fast enough keeps it running
indefinitely, since tearing down a dead connection takes long enough for
more to arrive. Whoever flushes nvmet_wq waits for it: removing any
other port of the target does in nvmet_port_release(), and so does
unloading the rdma and fc modules.

Accept a bounded number of connections per run and requeue the work. A
flush doesn't wait for work queued behind it.

Fixes: 872d26a391da ("nvmet-tcp: add NVMe over TCP target driver")
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Hannes Reinecke <hare@kernel.org>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/tcp.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 158e422851a4b..5105317cc61e3 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -102,6 +102,7 @@ MODULE_PARM_DESC(tls_handshake_timeout,
 #define NVMET_TCP_RECV_BUDGET		8
 #define NVMET_TCP_SEND_BUDGET		8
 #define NVMET_TCP_IO_WORK_BUDGET	64
+#define NVMET_TCP_ACCEPT_BUDGET		16
 
 enum nvmet_tcp_send_state {
 	NVMET_TCP_SEND_DATA_PDU,
@@ -2073,9 +2074,9 @@ static void nvmet_tcp_accept_work(struct work_struct *w)
 	struct nvmet_tcp_port *port =
 		container_of(w, struct nvmet_tcp_port, accept_work);
 	struct socket *newsock;
-	int ret;
+	int ret, i;
 
-	while (true) {
+	for (i = 0; i < NVMET_TCP_ACCEPT_BUDGET; i++) {
 		ret = kernel_accept(port->sock, &newsock, O_NONBLOCK);
 		if (ret < 0) {
 			if (ret != -EAGAIN)
@@ -2084,6 +2085,9 @@ static void nvmet_tcp_accept_work(struct work_struct *w)
 		}
 		nvmet_tcp_alloc_queue(port, newsock);
 	}
+
+	/* There may be more. Come to an end for those who flush nvmet_wq. */
+	queue_work(nvmet_wq, &port->accept_work);
 }
 
 static void nvmet_tcp_listen_data_ready(struct sock *sk)
-- 
2.52.0

[PATCH 08/19] nvme-tcp: start error recovery when a send fails

[Keith Busch]

From: Josef Bacik <josef@toxicpanda.com>

When the target closes a connection, nvme_tcp_state_change() calls
nvme_tcp_error_recovery(), which does nothing unless the controller is
live. While connecting that's fine for the queue being set up, its
Connect fails and the attempt with it. But if an already connected queue
is closed at the end of the attempt, nothing fails, the notification is
lost and the controller goes live on a dead socket. Every request on
that queue then fails with "failed to send request -32" and is retried,
forever with native multipathing, and if it's the admin queue the first
keep-alive fails too. Nothing but a manual reset recovers it. The Linux
target closes its connections at exactly that moment after a fatal
error.

Start error recovery when a request can't be sent, as nvme-rdma does.

Fixes: 3f2304f8c6d6 ("nvme-tcp: add NVMe over TCP host driver")
Cc: Keith Busch <kbusch@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Hannes Reinecke <hare@kernel.org>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/tcp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index bc46e5f3eaa79..9e15311e7d9cc 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1437,6 +1437,8 @@ static int nvme_tcp_try_send(struct nvme_tcp_queue *queue)
 			"failed to send request %d\n", ret);
 		nvme_tcp_fail_request(queue->request);
 		nvme_tcp_done_send_req(queue);
+		/* The close may have come when nobody could act on it */
+		nvme_tcp_error_recovery(&queue->ctrl->ctrl);
 	}
 out:
 	memalloc_noreclaim_restore(noreclaim_flag);
-- 
2.52.0

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

[Keith Busch]

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

[PATCH 10/19] nvmet: move preempt and abort work off nvmet-wq

[Keith Busch]

From: Chris Mason <mason@kernel.org>

nvmet_pr_do_abort() runs on nvmet-wq, holds pr_sem while it waits for
the preempted controllers' I/O, and releases it at the end. nvmet-wq
also runs nvmet_tcp_release_queue_work(), which cancel_work_sync()s the
io_work of a closed connection, and that io_work may be sleeping on
pr_sem in a reservation command. So a host that keeps a write in flight
on a preempted controller, sends a second preempt and abort, and then
closes enough connections with a reservation command in flight fills
nvmet-wq with release works that wait for pr_sem. When the first abort
finishes, the second one is queued on a full workqueue behind them and
never starts, pr_sem is never released, and the accept work doesn't run
either, so nobody can connect.

Queue the abort work on its own workqueue so it starts regardless. The
release works can still fill nvmet-wq while pr_sem is held, and a
file-backed namespace's flush, DSM and write zeroes need nvmet-wq
themselves, so this is a mitigation.

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/core.c  | 11 ++++++++++-
 drivers/nvme/target/nvmet.h |  1 +
 drivers/nvme/target/pr.c    |  6 +++++-
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 27b27c83ff831..1ed250cd4b56e 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -29,6 +29,7 @@ struct workqueue_struct *nvmet_wq;
 EXPORT_SYMBOL_GPL(nvmet_wq);
 struct workqueue_struct *nvmet_aen_wq;
 EXPORT_SYMBOL_GPL(nvmet_aen_wq);
+struct workqueue_struct *nvmet_pr_wq;
 
 /*
  * This read/write semaphore is used to synchronize access to configuration
@@ -2018,9 +2019,14 @@ static int __init nvmet_init(void)
 	if (!nvmet_aen_wq)
 		goto out_free_nvmet_work_queue;
 
+	nvmet_pr_wq = alloc_workqueue("nvmet-pr-wq",
+				      WQ_MEM_RECLAIM | WQ_UNBOUND, 0);
+	if (!nvmet_pr_wq)
+		goto out_free_nvmet_aen_work_queue;
+
 	error = nvmet_init_debugfs();
 	if (error)
-		goto out_free_nvmet_aen_work_queue;
+		goto out_free_nvmet_pr_work_queue;
 
 	error = nvmet_init_discovery();
 	if (error)
@@ -2036,6 +2042,8 @@ static int __init nvmet_init(void)
 	nvmet_exit_discovery();
 out_exit_debugfs:
 	nvmet_exit_debugfs();
+out_free_nvmet_pr_work_queue:
+	destroy_workqueue(nvmet_pr_wq);
 out_free_nvmet_aen_work_queue:
 	destroy_workqueue(nvmet_aen_wq);
 out_free_nvmet_work_queue:
@@ -2055,6 +2063,7 @@ static void __exit nvmet_exit(void)
 	nvmet_exit_discovery();
 	nvmet_exit_debugfs();
 	ida_destroy(&cntlid_ida);
+	destroy_workqueue(nvmet_pr_wq);
 	destroy_workqueue(nvmet_aen_wq);
 	destroy_workqueue(nvmet_wq);
 	destroy_workqueue(buffered_io_wq);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index a89b384c4bbc9..7b429cc16d491 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -528,6 +528,7 @@ extern struct workqueue_struct *buffered_io_wq;
 extern struct workqueue_struct *zbd_wq;
 extern struct workqueue_struct *nvmet_wq;
 extern struct workqueue_struct *nvmet_aen_wq;
+extern struct workqueue_struct *nvmet_pr_wq;
 
 static inline void nvmet_set_result(struct nvmet_req *req, u32 result)
 {
diff --git a/drivers/nvme/target/pr.c b/drivers/nvme/target/pr.c
index 1c0d6cac1a6b3..1a03f74c2e14f 100644
--- a/drivers/nvme/target/pr.c
+++ b/drivers/nvme/target/pr.c
@@ -756,7 +756,11 @@ static void nvmet_execute_pr_acquire(struct nvmet_req *req)
 	if (!status && acquire_act == NVME_PR_ACQUIRE_ACT_PREEMPT_AND_ABORT) {
 		kfree(d);
 		INIT_WORK(&req->r.abort_work, nvmet_pr_do_abort);
-		queue_work(nvmet_wq, &req->r.abort_work);
+		/*
+		 * Not nvmet_wq: works on it can wait for pr_sem, which this
+		 * work releases.
+		 */
+		queue_work(nvmet_pr_wq, &req->r.abort_work);
 		return;
 	}
 
-- 
2.52.0

[PATCH 11/19] nvmet-rdma: treat a failed connect as a disconnect

[Keith Busch]

From: Chris Mason <mason@kernel.org>

Once a connect request has been accepted, a queue is only freed by its
release_work, so that must be queued once. The disconnect paths take
the queue off nvmet_rdma_queue_list first and queue the work only if it
was still on it. nvmet_rdma_queue_connect_fail(), for REJECTED,
UNREACHABLE and CONNECT_ERROR, queues it unconditionally and only warns
if the queue isn't connecting. But the IB CM delivers a REJ from the
peer while the connection is established and even after we've sent a
DREQ, so a peer that causes a disconnect with a short capsule and then
sends a REJ while the release work is running queues it a second time
and the work runs on a freed queue. No credentials needed, just RoCE or
IB reachability, reproduced over rxe with a UDP socket.

Route connect failures through nvmet_rdma_queue_disconnect(), so only
the path that takes the queue off the list queues the work. A REJ on an
established connection still frees the queue, the RDMA CM reports no
disconnect after one.

Fixes: 8f000cac6e7a ("nvmet-rdma: add a NVMe over Fabrics RDMA target driver")
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/rdma.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index b9e39d1657d29..fb20ba1b84e7f 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -1745,15 +1745,14 @@ static void nvmet_rdma_queue_disconnect(struct nvmet_rdma_queue *queue)
 static void nvmet_rdma_queue_connect_fail(struct rdma_cm_id *cm_id,
 		struct nvmet_rdma_queue *queue)
 {
-	WARN_ON_ONCE(queue->state != NVMET_RDMA_Q_CONNECTING);
-
-	mutex_lock(&nvmet_rdma_queue_mutex);
-	if (!list_empty(&queue->queue_list))
-		list_del_init(&queue->queue_list);
-	mutex_unlock(&nvmet_rdma_queue_mutex);
-
 	pr_err("failed to connect queue %d\n", queue->idx);
-	queue_work(nvmet_wq, &queue->release_work);
+
+	/*
+	 * The peer decides when these events arrive: a REJ is also delivered
+	 * for a connection that is established, or that we are disconnecting.
+	 * Whoever takes the queue off the list queues release_work, once.
+	 */
+	nvmet_rdma_queue_disconnect(queue);
 }
 
 /**
-- 
2.52.0

[PATCH 12/19] nvmet-rdma: disconnect only established queues

[Keith Busch]

From: Chris Mason <mason@kernel.org>

Over iWARP, rdma_disconnect() sleeps in iw_cm_disconnect() until the
ESTABLISHED event of the connection has been handled, and iw_cm handles
all events of all connections one at a time on iw_cm_wq.
nvmet_rdma_destroy_port_queues(), nvmet_rdma_remove_one() and
nvmet_rdma_delete_ctrl() walk the queue list with
nvmet_rdma_queue_mutex held and call rdma_disconnect() for each, while
nvmet_rdma_cm_handler() takes the same mutex. If the walk sleeps on a
queue whose ESTABLISHED is queued behind another queue's event, both
are stuck and iWARP connection handling is dead until reboot. A peer
sets that up by connecting in a loop, or over siw by asking for MPA
peer-to-peer mode and then staying silent, which delays ESTABLISHED
indefinitely.

Only call rdma_disconnect() for a queue in NVMET_RDMA_Q_LIVE, which the
handler sets when it gets ESTABLISHED. A queue that never got there is
left to the release work: ib_drain_qp() moves the QP to the error
state, which ends the connection, and rdma_destroy_id() tells the peer.
Over IB and RoCE the skipped call did nothing more than that.

Fixes: f1d4ef7d8883 ("nvmet-rdma: register ib_client to not deadlock in device removal")
Fixes: fcf73a804c7d ("nvmet-rdma: fix use-after-free when a port is removed")
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/rdma.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index fb20ba1b84e7f..fc0c85076cee4 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -1694,7 +1694,7 @@ static void nvmet_rdma_queue_established(struct nvmet_rdma_queue *queue)
 
 static void __nvmet_rdma_queue_disconnect(struct nvmet_rdma_queue *queue)
 {
-	bool disconnect = false;
+	bool disconnect = false, live = false;
 	unsigned long flags;
 
 	pr_debug("cm_id= %p queue->state= %d\n", queue->cm_id, queue->state);
@@ -1713,6 +1713,7 @@ static void __nvmet_rdma_queue_disconnect(struct nvmet_rdma_queue *queue)
 		}
 		fallthrough;
 	case NVMET_RDMA_Q_LIVE:
+		live = queue->state == NVMET_RDMA_Q_LIVE;
 		queue->state = NVMET_RDMA_Q_DISCONNECTING;
 		disconnect = true;
 		break;
@@ -1722,7 +1723,17 @@ static void __nvmet_rdma_queue_disconnect(struct nvmet_rdma_queue *queue)
 	spin_unlock_irqrestore(&queue->state_lock, flags);
 
 	if (disconnect) {
-		rdma_disconnect(queue->cm_id);
+		/*
+		 * Over iWARP rdma_disconnect() sleeps until the CM has handled
+		 * the ESTABLISHED event of this connection. The CM handles the
+		 * events of all connections one at a time, and our handler
+		 * takes nvmet_rdma_queue_mutex, which callers of this function
+		 * hold. So a queue that has not seen the event is left to the
+		 * release work: ib_drain_qp() moves the queue pair to the error
+		 * state, which ends the connection.
+		 */
+		if (live)
+			rdma_disconnect(queue->cm_id);
 		queue_work(nvmet_wq, &queue->release_work);
 	}
 }
-- 
2.52.0

[PATCH 13/19] nvmet-rdma: take a device off the list last

[Keith Busch]

From: Chris Mason <mason@kernel.org>

nvmet_rdma_remove_one() disconnects the queues of a device and flushes
nvmet-wq, but since commit a3dd7d0022c3 it returns right away if the
device isn't on device_list. nvmet_rdma_free_dev(), run by the release
work of the last queue, takes the device off that list first and frees
the SRQs and the PD afterwards. A device removal in between finds
nothing on the list, returns, and the RDMA core frees the device under
ib_dealloc_pd().

Free the SRQs and the PD first and unlist the device last, so the
remove callback finds it and its flush waits for the release work.

Fixes: a3dd7d0022c3 ("nvmet-rdma: Don't flush system_wq by default during remove_one")
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/rdma.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index fc0c85076cee4..fe91df653e5cf 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -1197,13 +1197,17 @@ static void nvmet_rdma_free_dev(struct kref *ref)
 	struct nvmet_rdma_device *ndev =
 		container_of(ref, struct nvmet_rdma_device, ref);
 
+	nvmet_rdma_destroy_srqs(ndev);
+	ib_dealloc_pd(ndev->pd);
+
+	/*
+	 * Not earlier: nvmet_rdma_remove_one() waits for the release work that
+	 * got here only if it finds the device on the list.
+	 */
 	mutex_lock(&device_list_mutex);
 	list_del(&ndev->entry);
 	mutex_unlock(&device_list_mutex);
 
-	nvmet_rdma_destroy_srqs(ndev);
-	ib_dealloc_pd(ndev->pd);
-
 	kfree(ndev);
 }
 
-- 
2.52.0

[PATCH 14/19] nvmet-rdma: queue the release work under the mutex

[Keith Busch]

From: Chris Mason <mason@kernel.org>

nvmet_rdma_remove_one() takes the queue mutex, unlists each queue of
the device and queues its release work, then flushes nvmet-wq. So a
queue that still holds objects on the device must either be on the
list or have its release work queued. nvmet_rdma_queue_disconnect()
breaks that: it unlists the queue under the mutex, drops it, and only
then calls __nvmet_rdma_queue_disconnect() to queue the work. A device
removal in that window finds nothing to wait for, and the release work
then destroys the QP and CQ share on a freed device.

Queue the work before dropping the mutex, as nvmet_rdma_remove_one()
itself does. That's safe now that rdma_disconnect() no longer sleeps on
an iWARP event that needs the same mutex.

Fixes: f1d4ef7d8883 ("nvmet-rdma: register ib_client to not deadlock in device removal")
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/rdma.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index fe91df653e5cf..35fd3f3c35dc1 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -1744,17 +1744,17 @@ static void __nvmet_rdma_queue_disconnect(struct nvmet_rdma_queue *queue)
 
 static void nvmet_rdma_queue_disconnect(struct nvmet_rdma_queue *queue)
 {
-	bool disconnect = false;
-
+	/*
+	 * The release work is queued with the mutex held, so that
+	 * nvmet_rdma_remove_one() finds the queue on the list or its work on
+	 * the workqueue.
+	 */
 	mutex_lock(&nvmet_rdma_queue_mutex);
 	if (!list_empty(&queue->queue_list)) {
 		list_del_init(&queue->queue_list);
-		disconnect = true;
+		__nvmet_rdma_queue_disconnect(queue);
 	}
 	mutex_unlock(&nvmet_rdma_queue_mutex);
-
-	if (disconnect)
-		__nvmet_rdma_queue_disconnect(queue);
 }
 
 static void nvmet_rdma_queue_connect_fail(struct rdma_cm_id *cm_id,
-- 
2.52.0

[PATCH 15/19] nvmet-rdma: disconnect a queue on device removal

[Keith Busch]

From: Chris Mason <mason@kernel.org>

cma_remove_one() sends DEVICE_REMOVAL to the owner of every cm_id on
the device and then waits, without timeout, until they're all
destroyed. nvmet_rdma_device_removal() does nothing for a queue cm_id
and leaves the queues to nvmet_rdma_remove_one(), which only works if
that runs first. As a module it does, nvmet-rdma registers after
rdma_cm. Built in, both register at the same initcall level and
drivers/Makefile lists nvme/ ahead of infiniband/, so cma_remove_one()
runs first and waits for cm_ids that only the later callback would
destroy. Since the RDMA CM drops all later events for a cm_id once it
has sent DEVICE_REMOVAL, a peer closing its connection doesn't help
either. One connection from anyone who can reach the port hangs "rdma
link delete" until something else ends the queue.

Call nvmet_rdma_queue_disconnect() for the queue. It unlists the queue
and queues the release work, which destroys the cm_id. Don't wait for
it, rdma_destroy_id() waits for the handler to return, which was the
deadlock before commit f1d4ef7d8883.

Fixes: f1d4ef7d8883 ("nvmet-rdma: register ib_client to not deadlock in device removal")
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/rdma.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 35fd3f3c35dc1..412d48fe7e26a 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -1780,10 +1780,13 @@ static void nvmet_rdma_queue_connect_fail(struct rdma_cm_id *cm_id,
  * queue cm_id and/or a device bound listener cm_id (where in this
  * case queue will be null).
  *
- * We registered an ib_client to handle device removal for queues,
- * so we only need to handle the listening port cm_ids. In this case
- * we nullify the priv to prevent double cm_id destruction and destroying
- * the cm_id implicitly by returning a non-zero rc to the callout.
+ * The RDMA CM waits until each cm_id of the device is destroyed, so a queue
+ * has to go now. Our ib_client also removes the queues, but it cannot be
+ * relied on here: built in, it is called after the RDMA CM's client.
+ *
+ * For a listening port cm_id we nullify the priv to prevent double cm_id
+ * destruction and destroying the cm_id implicitly by returning a non-zero
+ * rc to the callout.
  */
 static int nvmet_rdma_device_removal(struct rdma_cm_id *cm_id,
 		struct nvmet_rdma_queue *queue)
@@ -1791,11 +1794,7 @@ static int nvmet_rdma_device_removal(struct rdma_cm_id *cm_id,
 	struct nvmet_rdma_port *port;
 
 	if (queue) {
-		/*
-		 * This is a queue cm_id. we have registered
-		 * an ib_client to handle queues removal
-		 * so don't interfere and just return.
-		 */
+		nvmet_rdma_queue_disconnect(queue);
 		return 0;
 	}
 
-- 
2.52.0

[PATCH 16/19] nvmet-rdma: reject queues larger than max_cqe

[Keith Busch]

From: Chris Mason <mason@kernel.org>

The host picks hrqsize and hsqsize in its RDMA CM connect request, and
nvmet_rdma_create_queue_ib() asks ib_cq_pool_get() for
recv + 2 * send + 1 CQ entries, up to 196607 for an I/O queue. The
pool limits each CQ it allocates to the device's max_cqe, so if the
request exceeds that no CQ ever fits and ib_alloc_cqs() keeps adding
CQs until an allocation fails. Those CQs stay until the device is
removed. rxe has a max_cqe of 32767, and one connect request from an
unprivileged process OOMs a 1 GiB target in under a second, with
nothing to kill that frees the memory.

Refuse a queue that needs more than max_cqe entries before touching the
pool. The connect is rejected with NVME_RDMA_CM_NO_RSC like any other
queue creation failure.

Fixes: ca0f1a8055be ("nvmet-rdma: use new shared CQ mechanism")
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/rdma.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 412d48fe7e26a..fced200678853 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -1293,8 +1293,17 @@ static int nvmet_rdma_create_queue_ib(struct nvmet_rdma_queue *queue)
 	 */
 	nr_cqe = queue->recv_queue_size + 2 * queue->send_queue_size;
 
-	queue->cq = ib_cq_pool_get(ndev->device, nr_cqe + 1,
-				   queue->comp_vector, IB_POLL_WORKQUEUE);
+	/*
+	 * The host picks both sizes. Never ask ib_cq_pool_get() for more
+	 * entries than one CQ of the device can have: no CQ would ever fit,
+	 * and it would allocate new CQs until an allocation fails.
+	 */
+	if (nr_cqe + 1 > ndev->device->attrs.max_cqe)
+		queue->cq = ERR_PTR(-EINVAL);
+	else
+		queue->cq = ib_cq_pool_get(ndev->device, nr_cqe + 1,
+					   queue->comp_vector,
+					   IB_POLL_WORKQUEUE);
 	if (IS_ERR(queue->cq)) {
 		ret = PTR_ERR(queue->cq);
 		pr_err("failed to create CQ cqe= %d ret= %d\n",
-- 
2.52.0

[PATCH 17/19] nvmet-rdma: don't run the wait list recursively

[Keith Busch]

From: Chris Mason <mason@kernel.org>

nvmet_rdma_release_rsp() returns the send queue slots of a response and
then runs the wait list, passing each waiting command to
nvmet_rdma_execute_command(). Many commands complete inside their
execute callback, and if the response can't be posted,
nvmet_rdma_queue_response() calls nvmet_rdma_release_rsp() right there,
which starts another run of the wait list inside the first:

    nvmet_rdma_process_wr_wait_list()
      nvmet_rdma_execute_command()
        nvmet_execute_admin_connect()
          nvmet_req_complete()
            nvmet_rdma_queue_response()
              ib_post_send() fails
              nvmet_rdma_release_rsp()
                nvmet_rdma_process_wr_wait_list()

Each waiting command adds a level while posts keep failing, and the host
decides how many commands wait: it sizes the receive queue, can fill
the send queue with commands that nvmet_req_init() rejects without
taking a slot, and over siw can make posts fail by holding back an
RDMA READ. 400 waiting Connects overflow the stack of the CQ worker.

Before the loop executes a command, point the response's new
wait_released at a flag on the loop's stack. A release that finds it
set means the loop hasn't returned from that command yet: it returns
the slots and releases the response, but sets the flag and leaves the
list to the loop instead of running it. The flag tells the loop not to
touch the response again.

Fixes: 8f000cac6e7a ("nvmet-rdma: add a NVMe over Fabrics RDMA target driver")
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/rdma.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index fced200678853..1e434cc451c28 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -77,6 +77,8 @@ struct nvmet_rdma_rsp {
 	u32			invalidate_rkey;
 
 	struct list_head	wait_list;
+	/* set under rsp_wr_wait_lock while the wait list's loop executes us */
+	bool			*wait_released;
 	int			tag;
 };
 
@@ -511,23 +513,42 @@ static int nvmet_rdma_post_recv(struct nvmet_rdma_device *ndev,
 	return ret;
 }
 
-static void nvmet_rdma_process_wr_wait_list(struct nvmet_rdma_queue *queue)
+static void nvmet_rdma_process_wr_wait_list(struct nvmet_rdma_queue *queue,
+					    struct nvmet_rdma_rsp *done)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&queue->rsp_wr_wait_lock, flags);
+	if (done->wait_released) {
+		/*
+		 * The loop below has not come back from this command yet. It is
+		 * further up this stack if the command completed in place and
+		 * its response could not be posted. Otherwise it runs in
+		 * another context. It goes on with the list, so leave the list
+		 * to it.
+		 */
+		*done->wait_released = true;
+		WRITE_ONCE(done->wait_released, NULL);
+		spin_unlock_irqrestore(&queue->rsp_wr_wait_lock, flags);
+		return;
+	}
 	while (!list_empty(&queue->rsp_wr_wait_list)) {
 		struct nvmet_rdma_rsp *rsp;
+		bool released = false;
 		bool ret;
 
 		rsp = list_entry(queue->rsp_wr_wait_list.next,
 				struct nvmet_rdma_rsp, wait_list);
 		list_del(&rsp->wait_list);
+		rsp->wait_released = &released;
 
 		spin_unlock_irqrestore(&queue->rsp_wr_wait_lock, flags);
 		ret = nvmet_rdma_execute_command(rsp);
 		spin_lock_irqsave(&queue->rsp_wr_wait_lock, flags);
 
+		/* A released response can be freed, or in use again. */
+		if (!released)
+			WRITE_ONCE(rsp->wait_released, NULL);
 		if (!ret) {
 			list_add(&rsp->wait_list, &queue->rsp_wr_wait_list);
 			break;
@@ -680,8 +701,9 @@ static void nvmet_rdma_release_rsp(struct nvmet_rdma_rsp *rsp)
 
 	nvmet_rdma_free_rsp_resources(rsp);
 
-	if (unlikely(!list_empty_careful(&queue->rsp_wr_wait_list)))
-		nvmet_rdma_process_wr_wait_list(queue);
+	if (unlikely(READ_ONCE(rsp->wait_released) ||
+		     !list_empty_careful(&queue->rsp_wr_wait_list)))
+		nvmet_rdma_process_wr_wait_list(queue, rsp);
 
 	nvmet_rdma_put_rsp(rsp);
 }
-- 
2.52.0

[PATCH 18/19] nvmet-rdma: fix a lost run of the wait list

[Keith Busch]

From: Chris Mason <mason@kernel.org>

A command is checked for free send queue slots before it's on the wait
list, in nvmet_rdma_handle_command() and when the processing loop takes
it off to retry, and nvmet_rdma_release_rsp() looks at the list without
a lock after giving slots back. The CQ worker and the CM thread (via
nvmet_rdma_queue_established()) can handle the commands of one queue
concurrently, so a release can see an empty list while the other side
is between the failed slot check and the list add:

    handle a command                  release a response
    too few free slots
                                      give the slots back
                                      wait list empty: return
    add the command to the list       /* nobody runs the list */

If that was the last response in flight the command stays on the list
forever. It holds sq->ref, so nvmet_sq_destroy() hangs at disconnect
and the queue's memory is never freed. A host hits it by sending more
Connects at once than the send queue has slots.

Look at the slots again after a command is on the list: the loop retries
after a put-back with a full barrier in between, nvmet_rdma_release_rsp()
has a matching barrier between returning the slots and looking at the
list, and nvmet_rdma_handle_command() runs the list once after adding
its command.

Fixes: 8f000cac6e7a ("nvmet-rdma: add a NVMe over Fabrics RDMA target driver")
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/rdma.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 1e434cc451c28..7f22a2a1886dc 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -519,7 +519,7 @@ static void nvmet_rdma_process_wr_wait_list(struct nvmet_rdma_queue *queue,
 	unsigned long flags;
 
 	spin_lock_irqsave(&queue->rsp_wr_wait_lock, flags);
-	if (done->wait_released) {
+	if (done && done->wait_released) {
 		/*
 		 * The loop below has not come back from this command yet. It is
 		 * further up this stack if the command completed in place and
@@ -551,6 +551,15 @@ static void nvmet_rdma_process_wr_wait_list(struct nvmet_rdma_queue *queue,
 			WRITE_ONCE(rsp->wait_released, NULL);
 		if (!ret) {
 			list_add(&rsp->wait_list, &queue->rsp_wr_wait_list);
+			/*
+			 * A release that came while the command was off the
+			 * list can have seen an empty list, so look at the
+			 * room again.
+			 * Pairs with the barrier in nvmet_rdma_release_rsp().
+			 */
+			smp_mb();
+			if (atomic_read(&queue->sq_wr_avail) > rsp->n_rdma)
+				continue;
 			break;
 		}
 	}
@@ -698,6 +707,11 @@ static void nvmet_rdma_release_rsp(struct nvmet_rdma_rsp *rsp)
 	struct nvmet_rdma_queue *queue = rsp->queue;
 
 	atomic_add(1 + rsp->n_rdma, &queue->sq_wr_avail);
+	/*
+	 * Give the room back ahead of the look at the wait list below.
+	 * Pairs with the smp_mb() in nvmet_rdma_process_wr_wait_list().
+	 */
+	smp_mb__after_atomic();
 
 	nvmet_rdma_free_rsp_resources(rsp);
 
@@ -1025,6 +1039,8 @@ static void nvmet_rdma_handle_command(struct nvmet_rdma_queue *queue,
 		spin_lock_irqsave(&queue->rsp_wr_wait_lock, flags);
 		list_add_tail(&cmd->wait_list, &queue->rsp_wr_wait_list);
 		spin_unlock_irqrestore(&queue->rsp_wr_wait_lock, flags);
+		/* The last response may have been released in between. */
+		nvmet_rdma_process_wr_wait_list(queue, NULL);
 	}
 
 	return;
-- 
2.52.0

[PATCH 19/19] nvmet: wait only for executing I/O in preempt and abort

[Keith Busch]

From: Chris Mason <mason@kernel.org>

nvmet_parse_io_cmd() takes the pc_ref for an I/O command as soon as it's
parsed, and "preempt and abort" waits for all pc_refs of the preempted
controllers before it releases pr_sem. But a write that still needs its
data can't complete without the host sending it and without the
transport context receiving it, and that same context is where Register,
Acquire and Release sleep on pr_sem. So a host that sends a write and
holds the data back, then preempt and abort with its own key, then
Register on the same queue, deadlocks io_work, the abort work and the
write with each other. Two well-behaved hosts get there by accident
when one fences the other while it re-registers under I/O. The
connection can't be torn down, the namespace can't be disabled, and
with nvmet_config_sem held across that, nobody can connect.

Take the pc_ref when the transport calls ->execute(), once all the data
is there: nvmet_parse_io_cmd() stashes the real execute method and
installs nvmet_pr_execute_io(), which gets the reference, rechecks the
reservation access since the host may have lost it since parse time,
and calls through. Preempt and abort then only waits for commands that
have started executing, which need neither the host nor the transport
to finish. A write whose data arrives after the abort now fails with
Reservation Conflict where it was previously executed, that's the
input the hang needs.

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/core.c  |  2 +-
 drivers/nvme/target/nvmet.h |  4 +++-
 drivers/nvme/target/pr.c    | 42 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 1ed250cd4b56e..4f6fb7c4181ab 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1176,7 +1176,7 @@ static u16 nvmet_parse_io_cmd(struct nvmet_req *req)
 		if (ret)
 			return ret;
 
-		ret = nvmet_pr_get_ns_pc_ref(req);
+		nvmet_pr_set_execute(req);
 	}
 	return ret;
 }
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 7b429cc16d491..99b3bf5cbd3b6 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -513,6 +513,8 @@ struct nvmet_req {
 	struct nvmet_port	*port;
 
 	void (*execute)(struct nvmet_req *req);
+	/* what nvmet_pr_execute_io() calls once it has the pc_ref */
+	void (*pr_execute)(struct nvmet_req *req);
 	const struct nvmet_fabrics_ops *ops;
 
 	struct pci_dev		*p2p_dev;
@@ -1041,7 +1043,7 @@ void nvmet_pr_exit_ns(struct nvmet_ns *ns);
 void nvmet_execute_get_log_page_resv(struct nvmet_req *req);
 u16 nvmet_set_feat_resv_notif_mask(struct nvmet_req *req, u32 mask);
 u16 nvmet_get_feat_resv_notif_mask(struct nvmet_req *req);
-u16 nvmet_pr_get_ns_pc_ref(struct nvmet_req *req);
+void nvmet_pr_set_execute(struct nvmet_req *req);
 static inline void nvmet_pr_put_ns_pc_ref(struct nvmet_pr_per_ctrl_ref *pc_ref)
 {
 	percpu_ref_put(&pc_ref->ref);
diff --git a/drivers/nvme/target/pr.c b/drivers/nvme/target/pr.c
index 1a03f74c2e14f..798f5ce9011f8 100644
--- a/drivers/nvme/target/pr.c
+++ b/drivers/nvme/target/pr.c
@@ -1060,7 +1060,7 @@ u16 nvmet_pr_check_cmd_access(struct nvmet_req *req)
 	return status;
 }
 
-u16 nvmet_pr_get_ns_pc_ref(struct nvmet_req *req)
+static u16 nvmet_pr_get_ns_pc_ref(struct nvmet_req *req)
 {
 	struct nvmet_pr_per_ctrl_ref *pc_ref;
 
@@ -1072,6 +1072,46 @@ u16 nvmet_pr_get_ns_pc_ref(struct nvmet_req *req)
 	return NVME_SC_SUCCESS;
 }
 
+static void nvmet_pr_execute_io(struct nvmet_req *req)
+{
+	u16 status;
+
+	status = nvmet_pr_get_ns_pc_ref(req);
+	if (!status) {
+		/*
+		 * The host can have lost its registration since the command was
+		 * parsed. A get that succeeds after "preempt and abort" has
+		 * resurrected the pc_ref has to see that. In percpu mode the
+		 * get orders no later load, so this pairs with the
+		 * smp_store_release() in __percpu_ref_switch_to_percpu(). While
+		 * the resurrect is under way the get is a fully ordered
+		 * atomic_long_inc_not_zero(). queue_work() in
+		 * nvmet_execute_pr_acquire() orders the unregistration before
+		 * the resurrect.
+		 */
+		smp_rmb();
+		status = nvmet_pr_check_cmd_access(req);
+	}
+	if (status) {
+		nvmet_req_complete(req, status);
+		return;
+	}
+
+	req->pr_execute(req);
+}
+
+/*
+ * "Preempt and abort" waits for the commands that hold a pc_ref. Until the
+ * transport calls ->execute(), the end of a command can depend on the host,
+ * which may still have to send the data, and on the context and the resources
+ * of the transport. So a command gets its pc_ref only then.
+ */
+void nvmet_pr_set_execute(struct nvmet_req *req)
+{
+	req->pr_execute = req->execute;
+	req->execute = nvmet_pr_execute_io;
+}
+
 static void nvmet_pr_ctrl_ns_all_cmds_done(struct percpu_ref *ref)
 {
 	struct nvmet_pr_per_ctrl_ref *pc_ref =
-- 
2.52.0

Re: [PATCH 08/19] nvme-tcp: start error recovery when a send fails

[Maurizio Lombardi]

On Thu Oct 29, 2026 at 4:52 PM CET, Keith Busch wrote:
> From: Josef Bacik <josef@toxicpanda.com>
>
> When the target closes a connection, nvme_tcp_state_change() calls
> nvme_tcp_error_recovery(), which does nothing unless the controller is
> live. While connecting that's fine for the queue being set up, its
> Connect fails and the attempt with it. But if an already connected queue
> is closed at the end of the attempt, nothing fails, the notification is
> lost and the controller goes live on a dead socket. Every request on
> that queue then fails with "failed to send request -32" and is retried,
> forever with native multipathing, and if it's the admin queue the first
> keep-alive fails too. Nothing but a manual reset recovers it. The Linux
> target closes its connections at exactly that moment after a fatal
> error.
>
> Start error recovery when a request can't be sent, as nvme-rdma does.
>
> Fixes: 3f2304f8c6d6 ("nvme-tcp: add NVMe over TCP host driver")
> Cc: Keith Busch <kbusch@kernel.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> Reviewed-by: Hannes Reinecke <hare@kernel.org>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/nvme/host/tcp.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index bc46e5f3eaa79..9e15311e7d9cc 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1437,6 +1437,8 @@ static int nvme_tcp_try_send(struct nvme_tcp_queue *queue)
>  			"failed to send request %d\n", ret);
>  		nvme_tcp_fail_request(queue->request);
>  		nvme_tcp_done_send_req(queue);
> +		/* The close may have come when nobody could act on it */
> +		nvme_tcp_error_recovery(&queue->ctrl->ctrl);
>  	}
>  out:
>  	memalloc_noreclaim_restore(noreclaim_flag);


I remember I posted an identical patch several months ago and I
self-nacked it because it made blktests md/001 fail

https://lore.kernel.org/linux-nvme/DDNRV5MAVQCF.2SH49C0P5MKVF@bsdbackstore.eu/

You might want to check if it's still the case before merging it.

Maurizio

Re: [PATCH 01/19] nvme-loop: delete the controller if a reset fails

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 03/19] nvme-fabrics: reconnect if the association is gone

[Christoph Hellwig]

On Thu, Oct 29, 2026 at 08:52:36AM -0700, Keith Busch wrote:
>  	if (ret) {
> -		nvmf_log_connect_error(ctrl, ret, le32_to_cpu(res.u32),
> -				       &cmd, data);
> +		result = le32_to_cpu(res.u32);

Nit on the result assignment:  This can just be moved before the if
and shared between the success and failure cases.

> +		nvmf_log_connect_error(ctrl, ret, result, &cmd, data);
> +		/*
> +		 * The controller ID we got from the admin queue's Connect is
> +		 * not known any more: the association has gone since. DNR is
> +		 * right about this command and says nothing about the next
> +		 * association. Don't let nvmf_should_reconnect() see it.
> +		 */
> +		if (ret > 0 &&
> +		    (ret & NVME_SCT_SC_MASK) == NVME_SC_CONNECT_INVALID_PARAM &&
> +		    result >> 16 && (result & 0xffff) ==
> +				offsetof(struct nvmf_connect_data, cntlid))
> +			ret = -ENOTCONN;

I really don't like this.  What real problem is this fixing?  Do we
really see setups where the the controller goes away before I/O command
connect, and the caller expects retries?  In that case we'll need to
describe this scenario.

And the condition above should move into a helper clearly documenting
it, and the decoding of the result should use common helpers with
nvmf_log_connect_error.

Re: [PATCH 04/19] nvmet-auth: keep the old keys if the new ones fail

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 05/19] nvmet: accept I/O queues on the controller's port only

[Christoph Hellwig]

On Thu, Oct 29, 2026 at 08:52:38AM -0700, Keith Busch wrote:
> From: Josef Bacik <josef@toxicpanda.com>
> 
> nvmet_ctrl_find_get() looks the subsystem up through the port of the
> request, but then takes the controller from the subsystem list whatever
> port it was created on. A controller belongs to one port and one
> transport: ctrl->ops is that of the admin queue, and both
> ->install_queue() and ->delete_ctrl() assume the queues are their own.
> So an I/O queue arriving on another port of the same subsystem, possibly
> on another transport, is installed into a controller it has no part in,
> nvmet_tcp_install_queue() writes to it as a TCP queue, and an error on
> it deletes that controller. The Connect comes before any authentication.
> 
> Skip controllers of other ports, and compare the transport too since a
> port's transport can be changed while it's disabled.
> 
> Fixes: a07b4970f464 ("nvmet: add a generic NVMe target")
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> Cc: Chaitanya Kulkarni <kch@nvidia.com>
> Reviewed-by: Hannes Reinecke <hare@kernel.org>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/nvme/target/core.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index a8171b88035c5..27b27c83ff831 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -1512,6 +1512,11 @@ struct nvmet_ctrl *nvmet_ctrl_find_get(const char *subsysnqn,
>  				pr_warn("hostnqn mismatch.\n");
>  				continue;
>  			}
> +			/* All queues of a controller come in by its port. */

This comment is vert confusing.  I'd drop it.

> +			if (ctrl->port != req->port || ctrl->ops != req->ops) {
> +				pr_warn("port mismatch.\n");
> +				continue;
> +			}

And I don't think this warning is very useful (same for the hostnqn one
above that looks like an accidental debug printk).

Re: [PATCH 06/19] nvmet: check the host NQN against the TLS PSK identity

[Christoph Hellwig]

On Thu, Oct 29, 2026 at 08:52:39AM -0700, Keith Busch wrote:
> From: Josef Bacik <josef@toxicpanda.com>
> 
> The identity of a TLS PSK names the host and subsystem the key was
> generated for. The target remembers which key a handshake ended with,
> but never compares it with the host NQN in the Connect that follows. So
> a host holding any PSK in the port's keyring can connect as another
> host, sees what that host may see, and isn't asked for DH-HMAC-CHAP
> since TLS queues skip it. It can add an I/O queue to that host's TLS
> controller the same way.
> 
> Refuse a Connect whose host NQN isn't the one in the PSK identity of the
> queue. The subsystem NQN isn't compared, a discovery subsystem goes by
> two names.
> 
> Fixes: 675b453e0241 ("nvmet-tcp: enable TLS handshake upcall")
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> Cc: Chaitanya Kulkarni <kch@nvidia.com>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/nvme/target/fabrics-cmd.c | 33 +++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
> index fc626c2fd2c92..b950000696c2a 100644
> --- a/drivers/nvme/target/fabrics-cmd.c
> +++ b/drivers/nvme/target/fabrics-cmd.c
> @@ -267,6 +267,30 @@ static u32 nvmet_connect_result(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq)
>  		(needs_auth ? NVME_CONNECT_AUTHREQ_ATR : 0);
>  }
>  
> +/*
> + * The identity of a TLS PSK names the host that the key was made for:
> + * "NVMe<version><type><hash> <host NQN> <subsystem NQN> ...".
> + * Whoever has proven to hold the key is that host, and no other one.
> + */
> +static u16 nvmet_check_tls_host(struct nvmet_sq *sq, const char *hostnqn)
> +{
> +#ifdef CONFIG_NVME_TARGET_TCP_TLS
> +	size_t len = strlen(hostnqn);
> +	const char *id;
> +
> +	if (!sq->tls_key)
> +		return 0;
> +
> +	id = strchr(sq->tls_key->description, ' ');
> +	if (!id || strncmp(id + 1, hostnqn, len) || id[len + 1] != ' ') {
> +		pr_warn("TLS PSK %08x does not belong to host %s\n",
> +			key_serial(sq->tls_key), hostnqn);
> +		return NVME_SC_CONNECT_INVALID_HOST | NVME_STATUS_DNR;
> +	}
> +#endif
> +	return 0;
> +}

#ifdef for an entire function body is an anti-pattern.  Either stub
out the entire function, or use IS_ENABLED depending on which one
works better here.

The rest looks good.

Re: [PATCH 07/19] nvmet-tcp: let the accept work come to an end

[Christoph Hellwig]

On Thu, Oct 29, 2026 at 08:52:40AM -0700, Keith Busch wrote:
> From: Josef Bacik <josef@toxicpanda.com>
> 
> nvmet_tcp_accept_work() loops until there's nothing left to accept, and
> anyone opening and closing connections fast enough keeps it running
> indefinitely, since tearing down a dead connection takes long enough for
> more to arrive. Whoever flushes nvmet_wq waits for it: removing any
> other port of the target does in nvmet_port_release(), and so does
> unloading the rdma and fc modules.
> 
> Accept a bounded number of connections per run and requeue the work. A
> flush doesn't wait for work queued behind it.

The subject is very odd, and should probably use something more like
a shortened version of this sentence.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Christoph Hellwig]

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---

Re: [PATCH 11/19] nvmet-rdma: treat a failed connect as a disconnect

[Christoph Hellwig]

On Thu, Oct 29, 2026 at 08:52:44AM -0700, Keith Busch wrote:
> +	/*
> +	 * The peer decides when these events arrive: a REJ is also delivered
> +	 * for a connection that is established, or that we are disconnecting.
> +	 * Whoever takes the queue off the list queues release_work, once.
> +	 */
> +	nvmet_rdma_queue_disconnect(queue);

Very odd wording, I think this needs a surperivising human in the room..

Re: [PATCH 12/19] nvmet-rdma: disconnect only established queues

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 13/19] nvmet-rdma: take a device off the list last

[Christoph Hellwig]

> @@ -1197,13 +1197,17 @@ static void nvmet_rdma_free_dev(struct kref *ref)
>  	struct nvmet_rdma_device *ndev =
>  		container_of(ref, struct nvmet_rdma_device, ref);
>  
> +	nvmet_rdma_destroy_srqs(ndev);
> +	ib_dealloc_pd(ndev->pd);
> +
> +	/*
> +	 * Not earlier: nvmet_rdma_remove_one() waits for the release work that
> +	 * got here only if it finds the device on the list.
> +	 */

Very odd wording.  This should be something like:

	/*
	 * Only delete from the device list once all RDMA resources are torn
	 * down because nvmet_rdma_remove_one waits for the release work only
	 * if the device still is on the list.
	 */

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 14/19] nvmet-rdma: queue the release work under the mutex

[Christoph Hellwig]

On Thu, Oct 29, 2026 at 08:52:47AM -0700, Keith Busch wrote:
> From: Chris Mason <mason@kernel.org>
> 
> nvmet_rdma_remove_one() takes the queue mutex, unlists each queue of
> the device and queues its release work, then flushes nvmet-wq. So a
> queue that still holds objects on the device must either be on the
> list or have its release work queued. nvmet_rdma_queue_disconnect()
> breaks that: it unlists the queue under the mutex, drops it, and only
> then calls __nvmet_rdma_queue_disconnect() to queue the work. A device
> removal in that window finds nothing to wait for, and the release work
> then destroys the QP and CQ share on a freed device.
> 
> Queue the work before dropping the mutex, as nvmet_rdma_remove_one()
> itself does. That's safe now that rdma_disconnect() no longer sleeps on
> an iWARP event that needs the same mutex.
> 
> Fixes: f1d4ef7d8883 ("nvmet-rdma: register ib_client to not deadlock in device removal")
> 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/rdma.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
> index fe91df653e5cf..35fd3f3c35dc1 100644
> --- a/drivers/nvme/target/rdma.c
> +++ b/drivers/nvme/target/rdma.c
> @@ -1744,17 +1744,17 @@ static void __nvmet_rdma_queue_disconnect(struct nvmet_rdma_queue *queue)
>  
>  static void nvmet_rdma_queue_disconnect(struct nvmet_rdma_queue *queue)
>  {
> -	bool disconnect = false;
> -
> +	/*
> +	 * The release work is queued with the mutex held, so that
> +	 * nvmet_rdma_remove_one() finds the queue on the list or its work on
> +	 * the workqueue.

Odd sentence structure once again:

	/*
	 * Queue the release work is with &nvmet_rdma_queue_mutex held, so that
	 * nvmet_rdma_remove_one() either finds the queue on the list or its
	 * the work has already been queued.
	 */

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 15/19] nvmet-rdma: disconnect a queue on device removal

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 16/19] nvmet-rdma: reject queues larger than max_cqe

[Christoph Hellwig]

On Thu, Oct 29, 2026 at 08:52:49AM -0700, Keith Busch wrote:
>  	 */
>  	nr_cqe = queue->recv_queue_size + 2 * queue->send_queue_size;
>  
> -	queue->cq = ib_cq_pool_get(ndev->device, nr_cqe + 1,
> -				   queue->comp_vector, IB_POLL_WORKQUEUE);
> +	/*
> +	 * The host picks both sizes. Never ask ib_cq_pool_get() for more
> +	 * entries than one CQ of the device can have: no CQ would ever fit,
> +	 * and it would allocate new CQs until an allocation fails.
> +	 */

"The host picks both sizes" read really weird, but doesn't actually
add any information.  In fact none of this comment seems overly
useful.

> +	if (nr_cqe + 1 > ndev->device->attrs.max_cqe)
> +		queue->cq = ERR_PTR(-EINVAL);
> +	else
> +		queue->cq = ib_cq_pool_get(ndev->device, nr_cqe + 1,
> +					   queue->comp_vector,
> +					   IB_POLL_WORKQUEUE);
>  	if (IS_ERR(queue->cq)) {
>  		ret = PTR_ERR(queue->cq);
>  		pr_err("failed to create CQ cqe= %d ret= %d\n",

The out of bounds case should have it's own error message instead of
falling through to this miseading error.

Re: [PATCH 19/19] nvmet: wait only for executing I/O in preempt and abort

[Christoph Hellwig]

Adding Guixin for the nvmet PR code.  It would have been useful
to group things a bit more..

On Thu, Oct 29, 2026 at 08:52:52AM -0700, Keith Busch wrote:
> From: Chris Mason <mason@kernel.org>
> 
> nvmet_parse_io_cmd() takes the pc_ref for an I/O command as soon as it's
> parsed, and "preempt and abort" waits for all pc_refs of the preempted
> controllers before it releases pr_sem. But a write that still needs its
> data can't complete without the host sending it and without the
> transport context receiving it, and that same context is where Register,
> Acquire and Release sleep on pr_sem. So a host that sends a write and
> holds the data back, then preempt and abort with its own key, then
> Register on the same queue, deadlocks io_work, the abort work and the
> write with each other. Two well-behaved hosts get there by accident
> when one fences the other while it re-registers under I/O. The
> connection can't be torn down, the namespace can't be disabled, and
> with nvmet_config_sem held across that, nobody can connect.
> 
> Take the pc_ref when the transport calls ->execute(), once all the data
> is there: nvmet_parse_io_cmd() stashes the real execute method and
> installs nvmet_pr_execute_io(), which gets the reference, rechecks the
> reservation access since the host may have lost it since parse time,
> and calls through. Preempt and abort then only waits for commands that
> have started executing, which need neither the host nor the transport
> to finish. A write whose data arrives after the abort now fails with
> Reservation Conflict where it was previously executed, that's the
> input the hang needs.
> 
> 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/core.c  |  2 +-
>  drivers/nvme/target/nvmet.h |  4 +++-
>  drivers/nvme/target/pr.c    | 42 ++++++++++++++++++++++++++++++++++++-
>  3 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 1ed250cd4b56e..4f6fb7c4181ab 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -1176,7 +1176,7 @@ static u16 nvmet_parse_io_cmd(struct nvmet_req *req)
>  		if (ret)
>  			return ret;
>  
> -		ret = nvmet_pr_get_ns_pc_ref(req);
> +		nvmet_pr_set_execute(req);
>  	}
>  	return ret;
>  }
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index 7b429cc16d491..99b3bf5cbd3b6 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -513,6 +513,8 @@ struct nvmet_req {
>  	struct nvmet_port	*port;
>  
>  	void (*execute)(struct nvmet_req *req);
> +	/* what nvmet_pr_execute_io() calls once it has the pc_ref */
> +	void (*pr_execute)(struct nvmet_req *req);
>  	const struct nvmet_fabrics_ops *ops;
>  
>  	struct pci_dev		*p2p_dev;
> @@ -1041,7 +1043,7 @@ void nvmet_pr_exit_ns(struct nvmet_ns *ns);
>  void nvmet_execute_get_log_page_resv(struct nvmet_req *req);
>  u16 nvmet_set_feat_resv_notif_mask(struct nvmet_req *req, u32 mask);
>  u16 nvmet_get_feat_resv_notif_mask(struct nvmet_req *req);
> -u16 nvmet_pr_get_ns_pc_ref(struct nvmet_req *req);
> +void nvmet_pr_set_execute(struct nvmet_req *req);
>  static inline void nvmet_pr_put_ns_pc_ref(struct nvmet_pr_per_ctrl_ref *pc_ref)
>  {
>  	percpu_ref_put(&pc_ref->ref);
> diff --git a/drivers/nvme/target/pr.c b/drivers/nvme/target/pr.c
> index 1a03f74c2e14f..798f5ce9011f8 100644
> --- a/drivers/nvme/target/pr.c
> +++ b/drivers/nvme/target/pr.c
> @@ -1060,7 +1060,7 @@ u16 nvmet_pr_check_cmd_access(struct nvmet_req *req)
>  	return status;
>  }
>  
> -u16 nvmet_pr_get_ns_pc_ref(struct nvmet_req *req)
> +static u16 nvmet_pr_get_ns_pc_ref(struct nvmet_req *req)
>  {
>  	struct nvmet_pr_per_ctrl_ref *pc_ref;
>  
> @@ -1072,6 +1072,46 @@ u16 nvmet_pr_get_ns_pc_ref(struct nvmet_req *req)
>  	return NVME_SC_SUCCESS;
>  }
>  
> +static void nvmet_pr_execute_io(struct nvmet_req *req)
> +{
> +	u16 status;
> +
> +	status = nvmet_pr_get_ns_pc_ref(req);
> +	if (!status) {
> +		/*
> +		 * The host can have lost its registration since the command was
> +		 * parsed. A get that succeeds after "preempt and abort" has
> +		 * resurrected the pc_ref has to see that. In percpu mode the
> +		 * get orders no later load, so this pairs with the
> +		 * smp_store_release() in __percpu_ref_switch_to_percpu(). While
> +		 * the resurrect is under way the get is a fully ordered
> +		 * atomic_long_inc_not_zero(). queue_work() in
> +		 * nvmet_execute_pr_acquire() orders the unregistration before
> +		 * the resurrect.
> +		 */
> +		smp_rmb();
> +		status = nvmet_pr_check_cmd_access(req);
> +	}
> +	if (status) {
> +		nvmet_req_complete(req, status);
> +		return;
> +	}
> +
> +	req->pr_execute(req);
> +}
> +
> +/*
> + * "Preempt and abort" waits for the commands that hold a pc_ref. Until the
> + * transport calls ->execute(), the end of a command can depend on the host,
> + * which may still have to send the data, and on the context and the resources
> + * of the transport. So a command gets its pc_ref only then.
> + */
> +void nvmet_pr_set_execute(struct nvmet_req *req)
> +{
> +	req->pr_execute = req->execute;
> +	req->execute = nvmet_pr_execute_io;
> +}
> +
>  static void nvmet_pr_ctrl_ns_all_cmds_done(struct percpu_ref *ref)
>  {
>  	struct nvmet_pr_per_ctrl_ref *pc_ref =
> -- 
> 2.52.0
---end quoted text---