[PATCH 1/2] nvme: fix racy access to FDP placement ID array

[Hari Mishal <harimishal1@gmail.com>]

nvme_query_fdp_info() populates head->nr_plids and head->plids the first
time a namespace's FDP configuration is registered, guarded only by a
check-then-act "if (head->nr_plids) return 0" with no locking. Since a
namespace's nvme_ns_head can be shared across multiple nvme_ns paths,
two controller paths scanning the same namespace at the same time can
race to populate this pair concurrently:

 - Two unsynchronized writers can each set nr_plids/plids
   independently, so the last writer of each field can differ,
   producing a count that doesn't match the actual size of the
   published array.
 - A concurrent reader in nvme_setup_rw() or
   nvme_update_ns_info_block() can observe a non-zero nr_plids
   while plids is still NULL, or sized for a different count,
   leading to a NULL dereference or an out-of-bounds read of
   ns->head->plids[].

Add a spinlock to nvme_ns_head and take it around every access to
nr_plids/plids, both the writer in nvme_query_fdp_info() and the
readers, so the pair is always observed and updated as a single
consistent unit. Use scoped_guard() so the lock covers the entire
read-and-use in both readers rather than being released before the
values are actually used.

Signed-off-by: Hari Mishal <harimishal1@gmail.com>
---
 drivers/nvme/host/core.c | 63 ++++++++++++++++++++++++++--------------
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 43 insertions(+), 21 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 453c1f0b2dd0..bdc5f07f5bf0 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1018,15 +1018,19 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
 	if (req->cmd_flags & REQ_RAHEAD)
 		dsmgmt |= NVME_RW_DSM_FREQ_PREFETCH;
 
-	if (op == nvme_cmd_write && ns->head->nr_plids) {
-		u16 write_stream = req->bio->bi_write_stream;
-
-		if (WARN_ON_ONCE(write_stream > ns->head->nr_plids))
-			return BLK_STS_INVAL;
-
-		if (write_stream) {
-			dsmgmt |= ns->head->plids[write_stream - 1] << 16;
-			control |= NVME_RW_DTYPE_DPLCMT;
+	if (op == nvme_cmd_write) {
+		scoped_guard(spinlock, &ns->head->fdp_lock) {
+			u16 write_stream = req->bio->bi_write_stream;
+
+			if (ns->head->nr_plids) {
+				if (WARN_ON_ONCE(write_stream > ns->head->nr_plids))
+					return BLK_STS_INVAL;
+
+				if (write_stream) {
+					dsmgmt |= ns->head->plids[write_stream - 1] << 16;
+					control |= NVME_RW_DTYPE_DPLCMT;
+				}
+			}
 		}
 	}
 
@@ -2317,6 +2321,8 @@ static int nvme_query_fdp_info(struct nvme_ns *ns, struct nvme_ns_info *info)
 	struct nvme_fdp_ruh_status *ruhs;
 	struct nvme_fdp_config fdp;
 	struct nvme_command c = {};
+	u16 nr_plids;
+	u16 *plids;
 	size_t size;
 	int i, ret;
 
@@ -2325,8 +2331,10 @@ static int nvme_query_fdp_info(struct nvme_ns *ns, struct nvme_ns_info *info)
 	 * so return immediately if we've already registered this namespace's
 	 * streams.
 	 */
-	if (head->nr_plids)
-		return 0;
+	scoped_guard(spinlock, &head->fdp_lock) {
+		if (head->nr_plids)
+			return 0;
+	}
 
 	ret = nvme_get_features(ctrl, NVME_FEAT_FDP, info->endgid, NULL, 0,
 				&fdp);
@@ -2357,23 +2365,34 @@ static int nvme_query_fdp_info(struct nvme_ns *ns, struct nvme_ns_info *info)
 		goto free;
 	}
 
-	head->nr_plids = le16_to_cpu(ruhs->nruhsd);
-	if (!head->nr_plids)
+	nr_plids = le16_to_cpu(ruhs->nruhsd);
+	if (!nr_plids)
 		goto free;
 
-	head->plids = kcalloc(head->nr_plids, sizeof(*head->plids),
-			      GFP_KERNEL);
-	if (!head->plids) {
+	plids = kcalloc(nr_plids, sizeof(*plids), GFP_KERNEL);
+	if (!plids) {
 		dev_warn(ctrl->device,
 			 "failed to allocate %u FDP placement IDs\n",
-			 head->nr_plids);
-		head->nr_plids = 0;
+			 nr_plids);
 		ret = -ENOMEM;
 		goto free;
 	}
 
-	for (i = 0; i < head->nr_plids; i++)
-		head->plids[i] = le16_to_cpu(ruhs->ruhsd[i].pid);
+	for (i = 0; i < nr_plids; i++)
+		plids[i] = le16_to_cpu(ruhs->ruhsd[i].pid);
+
+	/*
+	 * Publish the fully-populated array; if another path already won
+	 * the race, drop our redundant copy.
+	 */
+	scoped_guard(spinlock, &head->fdp_lock) {
+		if (head->nr_plids) {
+			kfree(plids);
+			goto free;
+		}
+		head->plids = plids;
+		head->nr_plids = nr_plids;
+	}
 free:
 	kfree(ruhs);
 	return ret;
@@ -2468,7 +2487,8 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
 	if (!nvme_init_integrity(ns->head, &lim, info))
 		capacity = 0;
 
-	lim.max_write_streams = ns->head->nr_plids;
+	scoped_guard(spinlock, &ns->head->fdp_lock)
+		lim.max_write_streams = ns->head->nr_plids;
 	if (lim.max_write_streams)
 		lim.write_stream_granularity = min(info->runs, U32_MAX);
 	else
@@ -3991,6 +4011,7 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
 	head->ids = info->ids;
 	head->shared = info->is_shared;
 	head->rotational = info->is_rotational;
+	spin_lock_init(&head->fdp_lock);
 	ratelimit_state_init(&head->rs_nuse, 5 * HZ, 1);
 	ratelimit_set_flags(&head->rs_nuse, RATELIMIT_MSG_ON_RELEASE);
 	kref_init(&head->ref);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 824651cc898d..22a68e09b065 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -560,6 +560,7 @@ struct nvme_ns_head {
 
 	u16			nr_plids;
 	u16			*plids;
+	spinlock_t		fdp_lock;	/* protects nr_plids and plids */
 #ifdef CONFIG_NVME_MULTIPATH
 	struct bio_list		requeue_list;
 	spinlock_t		requeue_lock;
-- 
2.43.0