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

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

[Hari Mishal]

nvme_ns_head can be shared across multiple nvme_ns paths (multipath,
or dual-port/multi-controller subsystems). nvme_query_fdp_info()
populates head->nr_plids/head->plids the first time a namespace's
FDP configuration is registered, but nothing protects that pair from
concurrent access - two paths scanning the same namespace at the
same time can race to populate it, and depending on how the writes
interleave a concurrent reader can hit a NULL dereference or an
out-of-bounds read.

Patch 1 adds a spinlock and closes the race.

Patch 2 is a small, unrelated cleanup on code sitting right next to
what patch 1 touches: it drops a WARN_ON_ONCE that turns out to be
unreachable through any current path, since the value it guards
against is already validated upstream in the block layer and F2FS
before a bio ever carries it. Happy to drop this one or send it
separately if you'd rather keep it out of this series.

Hari Mishal (2):
  nvme: fix racy access to FDP placement ID array
  nvme: drop WARN_ON_ONCE on write_stream bounds check

 drivers/nvme/host/core.c | 63 ++++++++++++++++++++++++++--------------
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 43 insertions(+), 21 deletions(-)

-- 
2.43.0

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

[Hari Mishal]

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

[PATCH 2/2] nvme: drop WARN_ON_ONCE on write_stream bounds check

[Hari Mishal]

write_stream is validated against bdev_max_write_streams() in both
generic block direct I/O (block/fops.c) and F2FS before a bio
carrying it is ever built, so write_stream > nr_plids shouldn't be
reachable through any current legitimate path. The remaining users
of bio->bi_write_stream elsewhere in the block layer only copy an
already-validated value between bios (bio.c, blk-crypto-fallback.c)
or compare it for merge eligibility (blk-merge.c); none of them
introduce a new, unvalidated value.

Using WARN_ON_ONCE as the backstop for that assumption isn't worth
it given how many deployed systems run with panic-on-warn enabled;
the existing graceful return BLK_STS_INVAL already handles it on
its own.

Signed-off-by: Hari Mishal <harimishal1@gmail.com>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index bdc5f07f5bf0..48ae0ffbbe6e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1023,7 +1023,7 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
 			u16 write_stream = req->bio->bi_write_stream;
 
 			if (ns->head->nr_plids) {
-				if (WARN_ON_ONCE(write_stream > ns->head->nr_plids))
+				if (write_stream > ns->head->nr_plids)
 					return BLK_STS_INVAL;
 
 				if (write_stream) {
-- 
2.43.0

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

[Kanchan Joshi]

On 7/25/2026 7:21 PM, Hari Mishal wrote:
> --- 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;
> +				}
> +			}

It will be nice not to have the impact on the fast-path performance.
The spin-lock is being taken for each write, regardless of FDP-enabled 
config. And for fdp-enabled config also, bi_write_stream may just be 0.

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

[Kanchan Joshi]

Hari,

Can you please try this patch and see if this solves what you saw?

  drivers/nvme/host/core.c | 38 +++++++++++++++++---------------------
  drivers/nvme/host/nvme.h |  1 +
  2 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 453c1f0b2dd0..09cb22f6509c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2320,14 +2320,6 @@ static int nvme_query_fdp_info(struct nvme_ns 
*ns, struct nvme_ns_info *info)
         size_t size;
         int i, ret;

-       /*
-        * The FDP configuration is static for the lifetime of the 
namespace,
-        * so return immediately if we've already registered this 
namespace's
-        * streams.
-        */
-       if (head->nr_plids)
-               return 0;
-
         ret = nvme_get_features(ctrl, NVME_FEAT_FDP, info->endgid, NULL, 0,
                                 &fdp);
         if (ret) {
@@ -2374,6 +2366,7 @@ static int nvme_query_fdp_info(struct nvme_ns *ns, 
struct nvme_ns_info *info)

         for (i = 0; i < head->nr_plids; i++)
                 head->plids[i] = le16_to_cpu(ruhs->ruhsd[i].pid);
+       head->write_stream_granularity = min(info->runs, U32_MAX);
  free:
         kfree(ruhs);
         return ret;
@@ -2421,12 +2414,6 @@ static int nvme_update_ns_info_block(struct 
nvme_ns *ns,
                         goto out;
         }

-       if (ns->ctrl->ctratt & NVME_CTRL_ATTR_FDPS) {
-               ret = nvme_query_fdp_info(ns, info);
-               if (ret < 0)
-                       goto out;
-       }
-
         if (nvme_invalid_lba_sz(le64_to_cpu(id->nsze),
                         id->lbaf[lbaf].ds - SECTOR_SHIFT, &capacity)) {
                 dev_warn_once(ns->ctrl->device,
@@ -2469,10 +2456,7 @@ static int nvme_update_ns_info_block(struct 
nvme_ns *ns,
                 capacity = 0;

         lim.max_write_streams = ns->head->nr_plids;
-       if (lim.max_write_streams)
-               lim.write_stream_granularity = min(info->runs, U32_MAX);
-       else
-               lim.write_stream_granularity = 0;
+       lim.write_stream_granularity = ns->head->write_stream_granularity;

         /*
          * Only set the DEAC bit if the device guarantees that reads from
@@ -3964,9 +3948,10 @@ static void nvme_add_ns_cdev(struct nvme_ns *ns)
         set_bit(NVME_NS_CDEV_LIVE, &ns->flags);
  }

-static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
+static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ns *ns,
                 struct nvme_ns_info *info)
  {
+       struct nvme_ctrl *ctrl = ns->ctrl;
         struct nvme_ns_head *head;
         size_t size = sizeof(*head);
         int ret = -ENOMEM;
@@ -4002,15 +3987,26 @@ static struct nvme_ns_head 
*nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
         } else
                 head->effects = ctrl->effects;

+       if (ctrl->ctratt & NVME_CTRL_ATTR_FDPS) {
+               ns->head = head;
+               ret = nvme_query_fdp_info(ns, info);
+               if (ret < 0)
+                       goto out_clear_ns_head;
+       }
+
         ret = nvme_mpath_alloc_disk(ctrl, head);
         if (ret)
-               goto out_cleanup_srcu;
+               goto out_cleanup_fdp;

         list_add_tail(&head->entry, &ctrl->subsys->nsheads);

         kref_get(&ctrl->subsys->ref);

         return head;
+out_cleanup_fdp:
+       kfree(head->plids);
+out_clear_ns_head:
+       ns->head = NULL;
  out_cleanup_srcu:
         cleanup_srcu_struct(&head->srcu);
  out_ida_remove:
@@ -4103,7 +4099,7 @@ static int nvme_init_ns_head(struct nvme_ns *ns, 
struct nvme_ns_info *info)
                                 info->nsid);
                         goto out_unlock;
                 }
-               head = nvme_alloc_ns_head(ctrl, info);
+               head = nvme_alloc_ns_head(ns, info);
                 if (IS_ERR(head)) {
                         ret = PTR_ERR(head);
                         goto out_unlock;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 824651cc898d..3ae73b36866d 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;
+       u32                     write_stream_granularity;
  #ifdef CONFIG_NVME_MULTIPATH
         struct bio_list         requeue_list;
         spinlock_t              requeue_lock;
--
2.25.1

Re: [PATCH 2/2] nvme: drop WARN_ON_ONCE on write_stream bounds check

[Keith Busch]

On Sat, Jul 25, 2026 at 03:51:11PM +0200, Hari Mishal wrote:
> write_stream is validated against bdev_max_write_streams() in both
> generic block direct I/O (block/fops.c) and F2FS before a bio
> carrying it is ever built, so write_stream > nr_plids shouldn't be
> reachable through any current legitimate path. The remaining users
> of bio->bi_write_stream elsewhere in the block layer only copy an
> already-validated value between bios (bio.c, blk-crypto-fallback.c)
> or compare it for merge eligibility (blk-merge.c); none of them
> introduce a new, unvalidated value.
> 
> Using WARN_ON_ONCE as the backstop for that assumption isn't worth
> it given how many deployed systems run with panic-on-warn enabled;
> the existing graceful return BLK_STS_INVAL already handles it on
> its own.

That's not a very good reason to remove a WARN_ON. You've left the check
in for a condition that should never happen, so when it does happen,
it'll be impossible to debug without the WARN.

And the WARN also annotates the branch as unlikely, which is desirable
for this case.

Re: [PATCH 2/2] nvme: drop WARN_ON_ONCE on write_stream bounds check

[Greg Kroah-Hartman]

On Mon, Jul 27, 2026 at 08:24:40AM -0600, Keith Busch wrote:
> On Sat, Jul 25, 2026 at 03:51:11PM +0200, Hari Mishal wrote:
> > write_stream is validated against bdev_max_write_streams() in both
> > generic block direct I/O (block/fops.c) and F2FS before a bio
> > carrying it is ever built, so write_stream > nr_plids shouldn't be
> > reachable through any current legitimate path. The remaining users
> > of bio->bi_write_stream elsewhere in the block layer only copy an
> > already-validated value between bios (bio.c, blk-crypto-fallback.c)
> > or compare it for merge eligibility (blk-merge.c); none of them
> > introduce a new, unvalidated value.
> > 
> > Using WARN_ON_ONCE as the backstop for that assumption isn't worth
> > it given how many deployed systems run with panic-on-warn enabled;
> > the existing graceful return BLK_STS_INVAL already handles it on
> > its own.
> 
> That's not a very good reason to remove a WARN_ON. You've left the check
> in for a condition that should never happen, so when it does happen,
> it'll be impossible to debug without the WARN.
> 
> And the WARN also annotates the branch as unlikely, which is desirable
> for this case.

But, if it ever does happen, a WARN_ON will reboot the box, given that
billions of Linux systems have panic-on-warn enabled.  So if this can
ever happen, just properly handle it and recover and don't loose user
data.

thanks,

greg k-h

Re: [PATCH 2/2] nvme: drop WARN_ON_ONCE on write_stream bounds check

[Keith Busch]

On Mon, Jul 27, 2026 at 09:19:27PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Jul 27, 2026 at 08:24:40AM -0600, Keith Busch wrote:
> > On Sat, Jul 25, 2026 at 03:51:11PM +0200, Hari Mishal wrote:
> > > write_stream is validated against bdev_max_write_streams() in both
> > > generic block direct I/O (block/fops.c) and F2FS before a bio
> > > carrying it is ever built, so write_stream > nr_plids shouldn't be
> > > reachable through any current legitimate path. The remaining users
> > > of bio->bi_write_stream elsewhere in the block layer only copy an
> > > already-validated value between bios (bio.c, blk-crypto-fallback.c)
> > > or compare it for merge eligibility (blk-merge.c); none of them
> > > introduce a new, unvalidated value.
> > > 
> > > Using WARN_ON_ONCE as the backstop for that assumption isn't worth
> > > it given how many deployed systems run with panic-on-warn enabled;
> > > the existing graceful return BLK_STS_INVAL already handles it on
> > > its own.
> > 
> > That's not a very good reason to remove a WARN_ON. You've left the check
> > in for a condition that should never happen, so when it does happen,
> > it'll be impossible to debug without the WARN.
> > 
> > And the WARN also annotates the branch as unlikely, which is desirable
> > for this case.
> 
> But, if it ever does happen, a WARN_ON will reboot the box, given that
> billions of Linux systems have panic-on-warn enabled.  

So WARN_ON is the new BUG_ON now? If the condition happens we need to
know how we got here and make it obvious something is wrong. So I guess
we'd have to replace every one of these:

	if (WARN_ON_ONCE(condition)) ...

With an open-coded version like:

	if (unlikely(condition)) {
		do_once(dump_stack());
		...
	}

?

That doesn't seem right, so if that is the suggestion, then I think we
need a new macro to provide the result that the WARN_ON usage expected.

> So if this can ever happen, 

But it can't ever happen. This patch's commit message reasoned that as
justification to remove the warn, but we need to know how we got here
when it does happen because it means somebody broke contract.

> just properly handle it and recover and don't loose user
> data.

We can't save the data from this specific condition: the data from the
request is unwritable and lost. We've also learned that EINVAL errors
are not handled for many DM stacking drivers in very bad ways, so again,
we need to know how we got here when something breaks the API contract
otherwise it'll be a difficult problem to debug without that visibility.