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

[Keith Busch]

From: Chris Mason <mason at 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 at lst.de>
Cc: Sagi Grimberg <sagi at grimberg.me>
Cc: Chaitanya Kulkarni <kch at nvidia.com>
Signed-off-by: Chris Mason <mason at kernel.org>
Signed-off-by: Jens Axboe <axboe at kernel.dk>
Signed-off-by: Keith Busch <kbusch at kernel.org>
---
 drivers/nvme/target/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