* [PATCH 01/11] nvme: add the FDP wire structures for the I/O management send command
2026-10-19 3:36 [PATCH RESEND 0/11] nvmet: FDP bridge for NVMe-oF targets Guixin Liu
@ 2026-10-19 3:36 ` Guixin Liu
2026-10-19 3:36 ` [PATCH 02/11] block: add bdev_write_stream_granularity() Guixin Liu
` (9 subsequent siblings)
10 siblings, 0 replies; 22+ messages in thread
From: Guixin Liu @ 2026-10-19 3:36 UTC (permalink / raw)
To: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg,
Nilay Shroff, Daniel Wagner, Hannes Reinecke
Cc: linux-nvme
Add the command definition of the I/O Management Send command (opcode
0x1D) to the command opcode enum and the command union, together with
its management operation, and the log page identifiers of the FDP
pages that the target serves, so that initiators and targets share
the same wire format.
No in-kernel initiator uses the command yet; the NVMe target FDP
support that consumes these structures follows in this series.
Assisted-by: LLM
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
include/linux/nvme.h | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 91ce434a7e8d9..9141fd386012b 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -967,6 +967,7 @@ enum nvme_opcode {
nvme_cmd_resv_acquire = 0x11,
nvme_cmd_io_mgmt_recv = 0x12,
nvme_cmd_resv_release = 0x15,
+ nvme_cmd_io_mgmt_send = 0x1d,
nvme_cmd_zone_mgmt_send = 0x79,
nvme_cmd_zone_mgmt_recv = 0x7a,
nvme_cmd_zone_append = 0x7d,
@@ -989,6 +990,7 @@ enum nvme_opcode {
nvme_opcode_name(nvme_cmd_resv_acquire), \
nvme_opcode_name(nvme_cmd_io_mgmt_recv), \
nvme_opcode_name(nvme_cmd_resv_release), \
+ nvme_opcode_name(nvme_cmd_io_mgmt_send), \
nvme_opcode_name(nvme_cmd_zone_mgmt_send), \
nvme_opcode_name(nvme_cmd_zone_mgmt_recv), \
nvme_opcode_name(nvme_cmd_zone_append))
@@ -1245,6 +1247,23 @@ enum {
NVME_IO_MGMT_RECV_MO_RUHS = 1,
};
+enum {
+ NVME_IO_MGMT_SEND_MO_RUH_UPDATE = 1,
+};
+
+struct nvme_io_mgmt_send_cmd {
+ __u8 opcode;
+ __u8 flags;
+ __le16 command_id;
+ __le32 nsid;
+ __le64 rsvd2[2];
+ union nvme_data_ptr dptr;
+ __u8 mo;
+ __u8 rsvd10;
+ __le16 mos;
+ __le32 rsvd11[5];
+};
+
struct nvme_fdp_ruh_status_desc {
__le16 pid;
__le16 ruhid;
@@ -1416,6 +1435,9 @@ enum {
NVME_LOG_FEATURES = 0x12,
NVME_LOG_RMI = 0x16,
NVME_LOG_FDP_CONFIGS = 0x20,
+ NVME_LOG_FDP_RUH_USAGE = 0x21,
+ NVME_LOG_FDP_STATS = 0x22,
+ NVME_LOG_FDP_EVENTS = 0x23,
NVME_LOG_DISC = 0x70,
NVME_LOG_RESERVATION = 0x80,
NVME_FWACT_REPL = (0 << 3),
@@ -1437,6 +1459,7 @@ struct nvme_supported_features_log {
enum {
NVME_FIS_FSUPP = 1 << 0,
+ NVME_FIS_EGSCPE = 1 << 23,
NVME_FIS_NSCPE = 1 << 20,
NVME_FIS_CSCPE = 1 << 21,
};
@@ -2016,6 +2039,7 @@ struct nvme_command {
struct nvme_dbbuf dbbuf;
struct nvme_directive_cmd directive;
struct nvme_io_mgmt_recv_cmd imr;
+ struct nvme_io_mgmt_send_cmd ims;
};
};
--
2.43.7
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 02/11] block: add bdev_write_stream_granularity()
2026-10-19 3:36 [PATCH RESEND 0/11] nvmet: FDP bridge for NVMe-oF targets Guixin Liu
2026-10-19 3:36 ` [PATCH 01/11] nvme: add the FDP wire structures for the I/O management send command Guixin Liu
@ 2026-10-19 3:36 ` Guixin Liu
2026-10-19 3:36 ` [PATCH 03/11] nvme: export nvme_bdev_ops Guixin Liu
` (8 subsequent siblings)
10 siblings, 0 replies; 22+ messages in thread
From: Guixin Liu @ 2026-10-19 3:36 UTC (permalink / raw)
To: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg,
Nilay Shroff, Daniel Wagner, Hannes Reinecke
Cc: linux-nvme
The write stream granularity queue limit is readable through the
queue_limits structure only. Add a block device accessor next to
bdev_max_write_streams(), so that holders of a block device can read
it without going through the request queue.
Assisted-by: LLM
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
include/linux/blkdev.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 4f7905c3412b8..6eca09058dec6 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1307,6 +1307,14 @@ static inline unsigned short bdev_max_write_streams(struct block_device *bdev)
return bdev_limits(bdev)->max_write_streams;
}
+static inline unsigned int
+bdev_write_stream_granularity(struct block_device *bdev)
+{
+ if (bdev_is_partition(bdev))
+ return 0;
+ return bdev_limits(bdev)->write_stream_granularity;
+}
+
static inline unsigned queue_logical_block_size(const struct request_queue *q)
{
return q->limits.logical_block_size;
--
2.43.7
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 03/11] nvme: export nvme_bdev_ops
2026-10-19 3:36 [PATCH RESEND 0/11] nvmet: FDP bridge for NVMe-oF targets Guixin Liu
2026-10-19 3:36 ` [PATCH 01/11] nvme: add the FDP wire structures for the I/O management send command Guixin Liu
2026-10-19 3:36 ` [PATCH 02/11] block: add bdev_write_stream_granularity() Guixin Liu
@ 2026-10-19 3:36 ` Guixin Liu
2026-10-19 3:36 ` [PATCH 04/11] nvmet: add per-namespace FDP bridge state and configuration Guixin Liu
` (7 subsequent siblings)
10 siblings, 0 replies; 22+ messages in thread
From: Guixin Liu @ 2026-10-19 3:36 UTC (permalink / raw)
To: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg,
Nilay Shroff, Daniel Wagner, Hannes Reinecke
Cc: linux-nvme
Export the NVMe block device operations so that other subsystems can
recognize a block device as an NVMe namespace through its operations
pointer and reach the NVMe structures behind it. The NVMe target FDP
bridge uses this to detect an NVMe backing device of a namespace.
Assisted-by: LLM
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
drivers/nvme/host/core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 1322c678f4eb8..b22aebc802920 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2712,6 +2712,7 @@ const struct block_device_operations nvme_bdev_ops = {
.report_zones = nvme_report_zones,
.pr_ops = &nvme_pr_ops,
};
+EXPORT_SYMBOL_GPL(nvme_bdev_ops);
static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 mask, u32 val,
u32 timeout, const char *op)
--
2.43.7
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 04/11] nvmet: add per-namespace FDP bridge state and configuration
2026-10-19 3:36 [PATCH RESEND 0/11] nvmet: FDP bridge for NVMe-oF targets Guixin Liu
` (2 preceding siblings ...)
2026-10-19 3:36 ` [PATCH 03/11] nvme: export nvme_bdev_ops Guixin Liu
@ 2026-10-19 3:36 ` Guixin Liu
2026-10-19 3:36 ` [PATCH 05/11] nvmet: report the Endurance Group Identifier in the CS-INDEP identify data Guixin Liu
` (6 subsequent siblings)
10 siblings, 0 replies; 22+ messages in thread
From: Guixin Liu @ 2026-10-19 3:36 UTC (permalink / raw)
To: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg,
Nilay Shroff, Daniel Wagner, Hannes Reinecke
Cc: linux-nvme
Flexible Data Placement lets a host direct writes into reclaim units
through placement identifiers, so that data with different lifetimes
stays separated in the media. It is defined in the NVMe base
specification and therefore applies to fabrics as well; a storage
system that exposes its local FDP-capable NVMe devices to remote
hosts over NVMe-oF currently has no way to pass that capability
through: nvmet neither advertises FDP nor forwards any FDP command.
Introduce the per-namespace FDP state that the following bridge
patches build on. A namespace opts in through a new fdp_enable
configfs attribute, settable only while the namespace is disabled,
like resv_enable. When the namespace is enabled, the FDP
configuration is snapshotted from the block device write stream
limits of the backing device: the number of reclaim unit handles and
the reclaim unit nominal size, together with the queues of the
backing NVMe namespace, which the FDP command forwarding needs.
The bridge lives behind its own NVME_TARGET_FDP Kconfig option, which
depends on NVME_CORE like the passthru support does: it reads the
host driver internals behind the block device operations pointer to
recognize an NVMe backing device and to reach its controller. The
plain NVMe target keeps no dependency on the NVMe host driver.
The NVMe host driver is the only one that reports write streams
today, so the enable is rejected when the backing device is not an
NVMe namespace. The snapshot is constrained to what the bridge can
serve: the namespace identifier has to fit the 16 bit Endurance
Group Identifier it is reported as, and the number of reclaim unit
handles is limited to the 8 bit write stream identifiers of the
block layer.
Assisted-by: LLM
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
drivers/nvme/target/Kconfig | 14 ++++++
drivers/nvme/target/Makefile | 1 +
drivers/nvme/target/configfs.c | 31 +++++++++++++
drivers/nvme/target/core.c | 12 ++++-
drivers/nvme/target/fdp.c | 85 ++++++++++++++++++++++++++++++++++
drivers/nvme/target/nvmet.h | 37 +++++++++++++++
6 files changed, 179 insertions(+), 1 deletion(-)
create mode 100644 drivers/nvme/target/fdp.c
diff --git a/drivers/nvme/target/Kconfig b/drivers/nvme/target/Kconfig
index 69bde270115e2..974878e345cb5 100644
--- a/drivers/nvme/target/Kconfig
+++ b/drivers/nvme/target/Kconfig
@@ -26,6 +26,20 @@ config NVME_TARGET_DEBUGFS
If unsure, say N.
+config NVME_TARGET_FDP
+ bool "NVMe Target Flexible Data Placement bridge support"
+ depends on NVME_TARGET
+ depends on NVME_CORE=y || NVME_CORE=NVME_TARGET
+ help
+ This enables the target side bridge for the Flexible Data
+ Placement (FDP) capability: namespaces backed by an FDP capable
+ NVMe namespace advertise the capability to hosts over NVMe-oF,
+ translate the placement identifiers of write commands to
+ backing write streams, and forward the FDP management commands
+ and log pages to the backing controller.
+
+ If unsure, say N.
+
config NVME_TARGET_PASSTHRU
bool "NVMe Target Passthrough support"
depends on NVME_TARGET
diff --git a/drivers/nvme/target/Makefile b/drivers/nvme/target/Makefile
index ed8522911d1f7..94814e015c2f5 100644
--- a/drivers/nvme/target/Makefile
+++ b/drivers/nvme/target/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_NVME_TARGET_PCI_EPF) += nvmet-pci-epf.o
nvmet-y += core.o configfs.o admin-cmd.o fabrics-cmd.o \
discovery.o io-cmd-file.o io-cmd-bdev.o pr.o
nvmet-$(CONFIG_NVME_TARGET_DEBUGFS) += debugfs.o
+nvmet-$(CONFIG_NVME_TARGET_FDP) += fdp.o
nvmet-$(CONFIG_NVME_TARGET_PASSTHRU) += passthru.o
nvmet-$(CONFIG_BLK_DEV_ZONED) += zns.o
nvmet-$(CONFIG_NVME_TARGET_AUTH) += fabrics-cmd-auth.o auth.o
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 413ee2d16d29c..a31a41afd736a 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -824,6 +824,34 @@ static ssize_t nvmet_ns_resv_enable_store(struct config_item *item,
}
CONFIGFS_ATTR(nvmet_ns_, resv_enable);
+#ifdef CONFIG_NVME_TARGET_FDP
+static ssize_t nvmet_ns_fdp_enable_show(struct config_item *item, char *page)
+{
+ return sysfs_emit(page, "%d\n", nvmet_fdp_enabled(to_nvmet_ns(item)));
+}
+
+static ssize_t nvmet_ns_fdp_enable_store(struct config_item *item,
+ const char *page, size_t count)
+{
+ struct nvmet_ns *ns = to_nvmet_ns(item);
+ bool val;
+
+ if (kstrtobool(page, &val))
+ return -EINVAL;
+
+ mutex_lock(&ns->subsys->lock);
+ if (ns->enabled) {
+ pr_err("the ns:%u is already enabled.\n", ns->nsid);
+ mutex_unlock(&ns->subsys->lock);
+ return -EINVAL;
+ }
+ ns->fdp.enable = val;
+ mutex_unlock(&ns->subsys->lock);
+ return count;
+}
+CONFIGFS_ATTR(nvmet_ns_, fdp_enable);
+#endif
+
static struct configfs_attribute *nvmet_ns_attrs[] = {
&nvmet_ns_attr_device_path,
&nvmet_ns_attr_device_nguid,
@@ -833,6 +861,9 @@ static struct configfs_attribute *nvmet_ns_attrs[] = {
&nvmet_ns_attr_buffered_io,
&nvmet_ns_attr_revalidate_size,
&nvmet_ns_attr_resv_enable,
+#ifdef CONFIG_NVME_TARGET_FDP
+ &nvmet_ns_attr_fdp_enable,
+#endif
#ifdef CONFIG_PCI_P2PDMA
&nvmet_ns_attr_p2pmem,
#endif
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index d74c01c98f194..4a6444fdfb2f8 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -610,9 +610,15 @@ int nvmet_ns_enable(struct nvmet_ns *ns)
goto out_dev_put;
}
+ if (nvmet_fdp_enabled(ns)) {
+ ret = nvmet_fdp_init_ns(ns);
+ if (ret)
+ goto out_pr_exit;
+ }
+
ret = percpu_ref_init(&ns->ref, nvmet_destroy_namespace, 0, GFP_KERNEL);
if (ret)
- goto out_pr_exit;
+ goto out_fdp_exit;
nvmet_ns_changed(subsys, ns->nsid);
ns->enabled = true;
@@ -622,6 +628,8 @@ int nvmet_ns_enable(struct nvmet_ns *ns)
out_unlock:
mutex_unlock(&subsys->lock);
return ret;
+out_fdp_exit:
+ nvmet_fdp_exit_ns(ns);
out_pr_exit:
if (ns->pr.enable)
nvmet_pr_exit_ns(ns);
@@ -667,6 +675,8 @@ void nvmet_ns_disable(struct nvmet_ns *ns)
if (ns->pr.enable)
nvmet_pr_exit_ns(ns);
+ nvmet_fdp_exit_ns(ns);
+
mutex_lock(&subsys->lock);
nvmet_ns_changed(subsys, ns->nsid);
nvmet_ns_dev_disable(ns);
diff --git a/drivers/nvme/target/fdp.c b/drivers/nvme/target/fdp.c
new file mode 100644
index 0000000000000..17949e3665293
--- /dev/null
+++ b/drivers/nvme/target/fdp.c
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * NVMe over Fabrics Flexible Data Placement bridge.
+ *
+ * Copyright (c) 2026 Guixin Liu, Alibaba Group.
+ * All rights reserved.
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/blkdev.h>
+#include <linux/nvme.h>
+
+#include "../host/nvme.h"
+#include "nvmet.h"
+
+bool nvmet_fdp_subsys_configured(struct nvmet_subsys *subsys)
+{
+ struct nvmet_ns *ns;
+ unsigned long idx;
+ bool ret = false;
+
+ mutex_lock(&subsys->lock);
+ nvmet_for_each_enabled_ns(&subsys->namespaces, idx, ns) {
+ if (nvmet_fdp_enabled(ns)) {
+ ret = true;
+ break;
+ }
+ }
+ mutex_unlock(&subsys->lock);
+
+ return ret;
+}
+
+int nvmet_fdp_init_ns(struct nvmet_ns *ns)
+{
+ struct block_device *bdev = ns->bdev;
+ struct nvme_ns *backend_ns;
+
+ if (!bdev) {
+ pr_err("the ns:%u has no block device for FDP\n", ns->nsid);
+ return -EINVAL;
+ }
+
+ /* the endgid equals the nsid, which is 16 bit on the wire */
+ if (ns->nsid > U16_MAX) {
+ pr_err("the ns:%u cannot fit an endurance group identifier\n",
+ ns->nsid);
+ return -EINVAL;
+ }
+
+ ns->fdp.nruh = bdev_max_write_streams(bdev);
+ if (!ns->fdp.nruh || ns->fdp.nruh > U8_MAX) {
+ pr_err("the ns:%u has no usable backing write streams: %u\n",
+ ns->nsid, ns->fdp.nruh);
+ return -EINVAL;
+ }
+
+ ns->fdp.runs = bdev_write_stream_granularity(bdev);
+ if (!ns->fdp.runs) {
+ pr_err("the ns:%u has no backing write streams\n", ns->nsid);
+ return -EINVAL;
+ }
+
+ /*
+ * The NVMe host driver is the only one that reports write streams
+ * today, so an FDP backing device is an NVMe namespace and the
+ * bridge needs its controller to forward the FDP commands to.
+ */
+ if (bdev->bd_disk->fops != &nvme_bdev_ops) {
+ pr_err("the ns:%u has no NVMe backing device for FDP\n",
+ ns->nsid);
+ return -EINVAL;
+ }
+
+ backend_ns = bdev->bd_disk->private_data;
+ ns->fdp.backend_q = backend_ns->queue;
+ ns->fdp.backend_ctrl = backend_ns->ctrl;
+
+ return 0;
+}
+
+void nvmet_fdp_exit_ns(struct nvmet_ns *ns)
+{
+ ns->fdp.backend_q = NULL;
+ ns->fdp.backend_ctrl = NULL;
+}
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index e362d7913a380..07880db4468ad 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -97,6 +97,14 @@ struct nvmet_pr_per_ctrl_ref {
uuid_t hostid;
};
+struct nvmet_fdp {
+ bool enable;
+ u16 nruh;
+ u64 runs;
+ struct request_queue *backend_q;
+ struct nvme_ctrl *backend_ctrl;
+};
+
struct nvmet_ns {
struct percpu_ref ref;
struct file *bdev_file;
@@ -128,6 +136,9 @@ struct nvmet_ns {
u8 csi;
struct nvmet_pr pr;
struct xarray pr_per_ctrl_refs;
+#ifdef CONFIG_NVME_TARGET_FDP
+ struct nvmet_fdp fdp;
+#endif
#ifdef CONFIG_NVME_TARGET_DEBUGFS
struct dentry *debugfs_dir;
#endif
@@ -978,6 +989,32 @@ static inline void nvmet_pr_put_ns_pc_ref(struct nvmet_pr_per_ctrl_ref *pc_ref)
percpu_ref_put(&pc_ref->ref);
}
+#ifdef CONFIG_NVME_TARGET_FDP
+static inline bool nvmet_fdp_enabled(struct nvmet_ns *ns)
+{
+ return ns->fdp.enable;
+}
+int nvmet_fdp_init_ns(struct nvmet_ns *ns);
+void nvmet_fdp_exit_ns(struct nvmet_ns *ns);
+bool nvmet_fdp_subsys_configured(struct nvmet_subsys *subsys);
+#else
+static inline bool nvmet_fdp_enabled(struct nvmet_ns *ns)
+{
+ return false;
+}
+static inline int nvmet_fdp_init_ns(struct nvmet_ns *ns)
+{
+ return -EINVAL;
+}
+static inline void nvmet_fdp_exit_ns(struct nvmet_ns *ns)
+{
+}
+static inline bool nvmet_fdp_subsys_configured(struct nvmet_subsys *subsys)
+{
+ return false;
+}
+#endif
+
/*
* Data for the get_feature() and set_feature() operations of PCI target
* controllers.
--
2.43.7
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 05/11] nvmet: report the Endurance Group Identifier in the CS-INDEP identify data
2026-10-19 3:36 [PATCH RESEND 0/11] nvmet: FDP bridge for NVMe-oF targets Guixin Liu
` (3 preceding siblings ...)
2026-10-19 3:36 ` [PATCH 04/11] nvmet: add per-namespace FDP bridge state and configuration Guixin Liu
@ 2026-10-19 3:36 ` Guixin Liu
2026-10-19 3:36 ` [PATCH 06/11] nvmet: advertise the FDP capability Guixin Liu
` (5 subsequent siblings)
10 siblings, 0 replies; 22+ messages in thread
From: Guixin Liu @ 2026-10-19 3:36 UTC (permalink / raw)
To: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg,
Nilay Shroff, Daniel Wagner, Hannes Reinecke
Cc: linux-nvme
The Identify Namespace data structure (CNS 00h) reports the Endurance
Group Identifier of the namespace, but the I/O Command Set Independent
Identify Namespace data structure (CNS 08h) leaves the field zeroed.
Hosts that implement NVMe 2.0 or later, or that cannot use CNS 00h,
take the Endurance Group Identifier from the CS-INDEP data structure
and thus always see 0, which breaks the consumers of the identifier:
the FDP feature and log pages, and the Rotational Media Information
log page, are addressed by it.
Report the same Endurance Group Identifier as CNS 00h: each namespace
is its own endurance group, so the identifier equals the namespace
identifier.
Assisted-by: LLM
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
drivers/nvme/target/admin-cmd.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 7764a3c0195c9..aad5abdb6ef96 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -1081,6 +1081,7 @@ static void nvmet_execute_id_cs_indep(struct nvmet_req *req)
id->nstat = NVME_NSTAT_NRDY;
id->anagrpid = cpu_to_le32(req->ns->anagrpid);
id->nmic = NVME_NS_NMIC_SHARED;
+ id->endgid = cpu_to_le16(req->ns->nsid);
if (req->ns->readonly)
id->nsattr |= NVME_NS_ATTR_RO;
if (req->ns->bdev && bdev_rot(req->ns->bdev))
--
2.43.7
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 06/11] nvmet: advertise the FDP capability
2026-10-19 3:36 [PATCH RESEND 0/11] nvmet: FDP bridge for NVMe-oF targets Guixin Liu
` (4 preceding siblings ...)
2026-10-19 3:36 ` [PATCH 05/11] nvmet: report the Endurance Group Identifier in the CS-INDEP identify data Guixin Liu
@ 2026-10-19 3:36 ` Guixin Liu
2026-10-19 3:36 ` [PATCH 07/11] nvmet: implement the FDP feature Guixin Liu
` (4 subsequent siblings)
10 siblings, 0 replies; 22+ messages in thread
From: Guixin Liu @ 2026-10-19 3:36 UTC (permalink / raw)
To: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg,
Nilay Shroff, Daniel Wagner, Hannes Reinecke
Cc: linux-nvme
Set the Flexible Data Placement Supported (FDPS) bit in the Identify
Controller ctratt field whenever at least one enabled namespace of
the subsystem bridges FDP. Hosts use the FDPS bit to decide whether
to query the FDP feature and configuration for each namespace, so
without it the bridge stays invisible. The FDP log pages are listed
in the Supported Log Pages log page by the patches that serve them.
Assisted-by: LLM
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
drivers/nvme/target/admin-cmd.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index aad5abdb6ef96..b293ce418e2c3 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -699,6 +699,8 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
ctratt = NVME_CTRL_ATTR_HID_128_BIT | NVME_CTRL_ATTR_TBKAS;
if (nvmet_is_pci_ctrl(ctrl))
ctratt |= NVME_CTRL_ATTR_RHII;
+ if (nvmet_fdp_subsys_configured(ctrl->subsys))
+ ctratt |= NVME_CTRL_ATTR_FDPS;
id->ctratt = cpu_to_le32(ctratt);
id->oacs = 0;
--
2.43.7
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 07/11] nvmet: implement the FDP feature
2026-10-19 3:36 [PATCH RESEND 0/11] nvmet: FDP bridge for NVMe-oF targets Guixin Liu
` (5 preceding siblings ...)
2026-10-19 3:36 ` [PATCH 06/11] nvmet: advertise the FDP capability Guixin Liu
@ 2026-10-19 3:36 ` Guixin Liu
2026-10-19 3:45 ` sashiko-bot
2026-10-19 3:36 ` [PATCH 08/11] nvmet: forward the FDP I/O management commands to the backing device Guixin Liu
` (3 subsequent siblings)
10 siblings, 1 reply; 22+ messages in thread
From: Guixin Liu @ 2026-10-19 3:36 UTC (permalink / raw)
To: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg,
Nilay Shroff, Daniel Wagner, Hannes Reinecke
Cc: linux-nvme
Implement Set Features and Get Features for FID 1Dh. The Endurance
Group Identifier in Command Dword 11 names the namespace directly,
since nvmet identifies each namespace as its own endurance group
(endgid = nsid). Get Features reports FDPE set with FDPCIDX 0.
Set Features is served locally: the bridge runs the one configuration
that the backing device is already in, so setting the feature to that
same value is a no-op that succeeds, and any other value is a change
of a feature whose endurance group has namespaces, which the spec
aborts with Command Sequence Error. That also covers disabling,
which would strand the write streams already handed out to hosts.
The namespace lookup takes a namespace reference, so that a namespace
being disabled concurrently with the command cannot go away under
it. Report the feature in the Features log page under the same
per-subsystem condition as the FDPS bit.
Assisted-by: LLM
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
drivers/nvme/target/admin-cmd.c | 9 ++++
drivers/nvme/target/fdp.c | 77 +++++++++++++++++++++++++++++++++
drivers/nvme/target/nvmet.h | 10 +++++
3 files changed, 96 insertions(+)
diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index b293ce418e2c3..93fb9c3d500a3 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -600,6 +600,9 @@ static void nvmet_execute_get_log_page_features(struct nvmet_req *req)
cpu_to_le32(NVME_FIS_FSUPP | NVME_FIS_NSCPE);
features->fis[NVME_FEAT_RESV_MASK] =
cpu_to_le32(NVME_FIS_FSUPP | NVME_FIS_NSCPE);
+ if (IS_ENABLED(CONFIG_NVME_TARGET_FDP))
+ features->fis[NVME_FEAT_FDP] =
+ cpu_to_le32(NVME_FIS_FSUPP | NVME_FIS_EGSCPE);
status = nvmet_copy_to_sgl(req, 0, features, sizeof(*features));
kfree(features);
@@ -1385,6 +1388,9 @@ void nvmet_execute_set_features(struct nvmet_req *req)
case NVME_FEAT_RESV_MASK:
status = nvmet_set_feat_resv_notif_mask(req, cdw11);
break;
+ case NVME_FEAT_FDP:
+ status = nvmet_set_feat_fdp(req);
+ break;
default:
req->error_loc = offsetof(struct nvme_common_command, cdw10);
status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
@@ -1562,6 +1568,9 @@ void nvmet_execute_get_features(struct nvmet_req *req)
case NVME_FEAT_RESV_MASK:
status = nvmet_get_feat_resv_notif_mask(req);
break;
+ case NVME_FEAT_FDP:
+ status = nvmet_get_feat_fdp(req);
+ break;
default:
req->error_loc =
offsetof(struct nvme_common_command, cdw10);
diff --git a/drivers/nvme/target/fdp.c b/drivers/nvme/target/fdp.c
index 17949e3665293..0327970a9cb9d 100644
--- a/drivers/nvme/target/fdp.c
+++ b/drivers/nvme/target/fdp.c
@@ -12,6 +12,10 @@
#include "../host/nvme.h"
#include "nvmet.h"
+/* Flexible Data Placement feature (FID 1Dh), Command Dword 12 */
+#define NVME_FDP_CDW12_FDPE (1U << 0)
+#define NVME_FDP_CDW12_FDPCIDX_MASK (0xffU << 8)
+
bool nvmet_fdp_subsys_configured(struct nvmet_subsys *subsys)
{
struct nvmet_ns *ns;
@@ -83,3 +87,76 @@ void nvmet_fdp_exit_ns(struct nvmet_ns *ns)
ns->fdp.backend_q = NULL;
ns->fdp.backend_ctrl = NULL;
}
+
+
+/*
+ * Each namespace is its own endurance group, so the Endurance Group
+ * Identifier in Command Dword 11, and in the Log Specific Identifier
+ * of the FDP log pages, directly names the namespace. Returns the
+ * namespace with a reference held.
+ */
+static struct nvmet_ns *nvmet_fdp_lookup_ns(struct nvmet_req *req, u32 endgid)
+{
+ struct nvmet_subsys *subsys = nvmet_req_subsys(req);
+ struct nvmet_ns *ns;
+
+ ns = xa_load(&subsys->namespaces, endgid);
+ if (!ns || !ns->enabled || !nvmet_fdp_enabled(ns))
+ return NULL;
+
+ if (!percpu_ref_tryget_live(&ns->ref))
+ return NULL;
+
+ return ns;
+}
+
+/*
+ * The bridge runs the one configuration that the backing device is
+ * already in, so setting the feature to that same value is the only
+ * transition that can be served. The spec aborts any other value
+ * change of a feature whose endurance group has namespaces with
+ * Command Sequence Error, which also covers disabling: pulling FDP
+ * from under a live namespace would strand the write streams already
+ * handed out to hosts.
+ */
+u16 nvmet_set_feat_fdp(struct nvmet_req *req)
+{
+ u32 cdw12 = le32_to_cpu(req->cmd->features.dword12);
+ struct nvmet_ns *ns;
+ u16 status;
+
+ ns = nvmet_fdp_lookup_ns(req,
+ le32_to_cpu(req->cmd->features.dword11));
+ if (!ns)
+ return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
+
+ if (cdw12 & ~(NVME_FDP_CDW12_FDPCIDX_MASK | NVME_FDP_CDW12_FDPE)) {
+ status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
+ } else if ((cdw12 & NVME_FDP_CDW12_FDPE) &&
+ !(cdw12 & NVME_FDP_CDW12_FDPCIDX_MASK)) {
+ nvmet_set_result(req, FDPCFG_FDPE);
+ status = 0;
+ } else {
+ status = NVME_SC_CMD_SEQ_ERROR | NVME_STATUS_DNR;
+ }
+
+ percpu_ref_put(&ns->ref);
+ return status;
+}
+
+u16 nvmet_get_feat_fdp(struct nvmet_req *req)
+{
+ struct nvmet_ns *ns;
+
+ ns = nvmet_fdp_lookup_ns(req,
+ le32_to_cpu(req->cmd->features.dword11));
+ if (!ns)
+ return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
+
+ /* the namespace is up, so FDP is enabled on the backing device */
+ nvmet_set_result(req, FDPCFG_FDPE);
+
+ percpu_ref_put(&ns->ref);
+ return 0;
+}
+
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 07880db4468ad..e95bafe1fb019 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -997,6 +997,8 @@ static inline bool nvmet_fdp_enabled(struct nvmet_ns *ns)
int nvmet_fdp_init_ns(struct nvmet_ns *ns);
void nvmet_fdp_exit_ns(struct nvmet_ns *ns);
bool nvmet_fdp_subsys_configured(struct nvmet_subsys *subsys);
+u16 nvmet_set_feat_fdp(struct nvmet_req *req);
+u16 nvmet_get_feat_fdp(struct nvmet_req *req);
#else
static inline bool nvmet_fdp_enabled(struct nvmet_ns *ns)
{
@@ -1013,6 +1015,14 @@ static inline bool nvmet_fdp_subsys_configured(struct nvmet_subsys *subsys)
{
return false;
}
+static inline u16 nvmet_set_feat_fdp(struct nvmet_req *req)
+{
+ return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
+}
+static inline u16 nvmet_get_feat_fdp(struct nvmet_req *req)
+{
+ return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
+}
#endif
/*
--
2.43.7
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 07/11] nvmet: implement the FDP feature
2026-10-19 3:36 ` [PATCH 07/11] nvmet: implement the FDP feature Guixin Liu
@ 2026-10-19 3:45 ` sashiko-bot
2026-10-19 8:30 ` Guixin Liu
0 siblings, 1 reply; 22+ messages in thread
From: sashiko-bot @ 2026-10-19 3:45 UTC (permalink / raw)
To: Guixin Liu; +Cc: linux-nvme
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] nvmet: missing RCU protection and live check in nvmet_fdp_lookup_ns()
- [Medium] nvmet: unconditional NVME_FEAT_FDP in nvmet_execute_get_log_page_features()
--
commit 121b81584ccf85602ba9ff94dfc9a38064c55462
Author: Guixin Liu <kanie@linux.alibaba.com>
nvmet: implement the FDP feature
This commit implements Set Features and Get Features for the Flexible Data
Placement feature (FID 1Dh) in nvmet, using the Endurance Group Identifier
to look up the corresponding namespace. It also advertises NVME_FEAT_FDP in
the Supported Features log page.
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index b293ce418e2c3..93fb9c3d500a3 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -600,6 +600,9 @@ static void nvmet_execute_get_log_page_features(struct nvmet_req *req)
> cpu_to_le32(NVME_FIS_FSUPP | NVME_FIS_NSCPE);
> features->fis[NVME_FEAT_RESV_MASK] =
> cpu_to_le32(NVME_FIS_FSUPP | NVME_FIS_NSCPE);
> + if (IS_ENABLED(CONFIG_NVME_TARGET_FDP))
> + features->fis[NVME_FEAT_FDP] =
> + cpu_to_le32(NVME_FIS_FSUPP | NVME_FIS_EGSCPE);
[Severity: Medium]
Should nvmet_execute_get_log_page_features() check
nvmet_fdp_subsys_configured(nvmet_req_subsys(req)) here instead of
IS_ENABLED(CONFIG_NVME_TARGET_FDP)?
The commit message notes that this feature is reported in the Features log
page under the same per-subsystem condition as the FDPS bit. In
nvmet_execute_identify_ctrl(), NVME_CTRL_ATTR_FDPS is gated on:
if (nvmet_fdp_subsys_configured(ctrl->subsys))
ctratt |= NVME_CTRL_ATTR_FDPS;
Checking only IS_ENABLED(CONFIG_NVME_TARGET_FDP) causes the log page to
advertise NVME_FEAT_FDP on subsystems that have no FDP-enabled namespaces
and report FDPS as 0.
> status = nvmet_copy_to_sgl(req, 0, features, sizeof(*features));
> kfree(features);
[ ... ]
> diff --git a/drivers/nvme/target/fdp.c b/drivers/nvme/target/fdp.c
> index 17949e3665293..0327970a9cb9d 100644
> --- a/drivers/nvme/target/fdp.c
> +++ b/drivers/nvme/target/fdp.c
[ ... ]
> @@ -83,3 +87,76 @@ void nvmet_fdp_exit_ns(struct nvmet_ns *ns)
> ns->fdp.backend_q = NULL;
> ns->fdp.backend_ctrl = NULL;
> }
> +
> +
> +/*
> + * Each namespace is its own endurance group, so the Endurance Group
> + * Identifier in Command Dword 11, and in the Log Specific Identifier
> + * of the FDP log pages, directly names the namespace. Returns the
> + * namespace with a reference held.
> + */
> +static struct nvmet_ns *nvmet_fdp_lookup_ns(struct nvmet_req *req, u32 endgid)
> +{
> + struct nvmet_subsys *subsys = nvmet_req_subsys(req);
> + struct nvmet_ns *ns;
> +
> + ns = xa_load(&subsys->namespaces, endgid);
> + if (!ns || !ns->enabled || !nvmet_fdp_enabled(ns))
> + return NULL;
> +
> + if (!percpu_ref_tryget_live(&ns->ref))
> + return NULL;
> +
> + return ns;
> +}
[Severity: Critical]
Can nvmet_fdp_lookup_ns() cause a use-after-free on struct nvmet_ns or
ns->ref when a Set Features or Get Features command for NVME_FEAT_FDP races
with namespace disable or removal via nvmet_ns_disable() or
nvmet_ns_free()?
In drivers/nvme/target/core.c, nvmet_ns_disable() tears down the namespace
before clearing ns->enabled:
if (!test_and_clear_bit(NVMET_NS_IO_LIVE, &ns->flags))
return;
...
percpu_ref_kill(&ns->ref);
synchronize_rcu();
wait_for_completion(&ns->disable_done);
percpu_ref_exit(&ns->ref);
if (ns->pr.enable)
nvmet_pr_exit_ns(ns);
nvmet_fdp_exit_ns(ns);
mutex_lock(&subsys->lock);
nvmet_ns_changed(subsys, ns->nsid);
nvmet_ns_dev_disable(ns);
ns->enabled = false;
mutex_unlock(&subsys->lock);
And nvmet_ns_free() then erases and frees ns without an additional RCU
grace period:
nvmet_ns_disable(ns);
mutex_lock(&subsys->lock);
xa_erase(&subsys->namespaces, ns->nsid);
...
kfree(ns->device_path);
kfree(ns);
Because xa_load() releases its internal RCU read lock before returning,
nvmet_fdp_lookup_ns() holds an unpinned pointer to ns outside of an
rcu_read_lock() critical section. A concurrent nvmet_ns_free() is not
blocked by synchronize_rcu() in nvmet_ns_disable() and can free ns before
nvmet_fdp_lookup_ns() dereferences ns->enabled, ns->fdp.enable, or
&ns->ref.
Additionally, because ns->enabled remains true until after
percpu_ref_exit(&ns->ref) has freed the per-CPU counter data, checking
ns->enabled instead of test_bit(NVMET_NS_IO_LIVE, &ns->flags) allows
nvmet_fdp_lookup_ns() to call percpu_ref_tryget_live(&ns->ref) concurrently
with or after percpu_ref_exit(&ns->ref).
Should nvmet_fdp_lookup_ns() wrap the lookup and reference acquisition in
rcu_read_lock() / rcu_read_unlock(), check test_bit(NVMET_NS_IO_LIVE,
&ns->flags), and use percpu_ref_tryget_live_rcu(&ns->ref) in the same way
as nvmet_req_find_ns()?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261019033624.402337-1-kanie@linux.alibaba.com?part=7
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 07/11] nvmet: implement the FDP feature
2026-10-19 3:45 ` sashiko-bot
@ 2026-10-19 8:30 ` Guixin Liu
0 siblings, 0 replies; 22+ messages in thread
From: Guixin Liu @ 2026-10-19 8:30 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-nvme
在 2026/10/19 11:45, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] nvmet: missing RCU protection and live check in nvmet_fdp_lookup_ns()
> - [Medium] nvmet: unconditional NVME_FEAT_FDP in nvmet_execute_get_log_page_features()
> --
>
> commit 121b81584ccf85602ba9ff94dfc9a38064c55462
> Author: Guixin Liu <kanie@linux.alibaba.com>
>
> nvmet: implement the FDP feature
>
> This commit implements Set Features and Get Features for the Flexible Data
> Placement feature (FID 1Dh) in nvmet, using the Endurance Group Identifier
> to look up the corresponding namespace. It also advertises NVME_FEAT_FDP in
> the Supported Features log page.
>
>> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
>> index b293ce418e2c3..93fb9c3d500a3 100644
>> --- a/drivers/nvme/target/admin-cmd.c
>> +++ b/drivers/nvme/target/admin-cmd.c
>> @@ -600,6 +600,9 @@ static void nvmet_execute_get_log_page_features(struct nvmet_req *req)
>> cpu_to_le32(NVME_FIS_FSUPP | NVME_FIS_NSCPE);
>> features->fis[NVME_FEAT_RESV_MASK] =
>> cpu_to_le32(NVME_FIS_FSUPP | NVME_FIS_NSCPE);
>> + if (IS_ENABLED(CONFIG_NVME_TARGET_FDP))
>> + features->fis[NVME_FEAT_FDP] =
>> + cpu_to_le32(NVME_FIS_FSUPP | NVME_FIS_EGSCPE);
> [Severity: Medium]
> Should nvmet_execute_get_log_page_features() check
> nvmet_fdp_subsys_configured(nvmet_req_subsys(req)) here instead of
> IS_ENABLED(CONFIG_NVME_TARGET_FDP)?
>
> The commit message notes that this feature is reported in the Features log
> page under the same per-subsystem condition as the FDPS bit. In
> nvmet_execute_identify_ctrl(), NVME_CTRL_ATTR_FDPS is gated on:
>
> if (nvmet_fdp_subsys_configured(ctrl->subsys))
> ctratt |= NVME_CTRL_ATTR_FDPS;
>
> Checking only IS_ENABLED(CONFIG_NVME_TARGET_FDP) causes the log page to
> advertise NVME_FEAT_FDP on subsystems that have no FDP-enabled namespaces
> and report FDPS as 0.
Yes, fixed in v2.
>> status = nvmet_copy_to_sgl(req, 0, features, sizeof(*features));
>> kfree(features);
> [ ... ]
>
>> diff --git a/drivers/nvme/target/fdp.c b/drivers/nvme/target/fdp.c
>> index 17949e3665293..0327970a9cb9d 100644
>> --- a/drivers/nvme/target/fdp.c
>> +++ b/drivers/nvme/target/fdp.c
> [ ... ]
>
>> @@ -83,3 +87,76 @@ void nvmet_fdp_exit_ns(struct nvmet_ns *ns)
>> ns->fdp.backend_q = NULL;
>> ns->fdp.backend_ctrl = NULL;
>> }
>> +
>> +
>> +/*
>> + * Each namespace is its own endurance group, so the Endurance Group
>> + * Identifier in Command Dword 11, and in the Log Specific Identifier
>> + * of the FDP log pages, directly names the namespace. Returns the
>> + * namespace with a reference held.
>> + */
>> +static struct nvmet_ns *nvmet_fdp_lookup_ns(struct nvmet_req *req, u32 endgid)
>> +{
>> + struct nvmet_subsys *subsys = nvmet_req_subsys(req);
>> + struct nvmet_ns *ns;
>> +
>> + ns = xa_load(&subsys->namespaces, endgid);
>> + if (!ns || !ns->enabled || !nvmet_fdp_enabled(ns))
>> + return NULL;
>> +
>> + if (!percpu_ref_tryget_live(&ns->ref))
>> + return NULL;
>> +
>> + return ns;
>> +}
> [Severity: Critical]
> Can nvmet_fdp_lookup_ns() cause a use-after-free on struct nvmet_ns or
> ns->ref when a Set Features or Get Features command for NVME_FEAT_FDP races
> with namespace disable or removal via nvmet_ns_disable() or
> nvmet_ns_free()?
>
> In drivers/nvme/target/core.c, nvmet_ns_disable() tears down the namespace
> before clearing ns->enabled:
>
> if (!test_and_clear_bit(NVMET_NS_IO_LIVE, &ns->flags))
> return;
> ...
> percpu_ref_kill(&ns->ref);
> synchronize_rcu();
> wait_for_completion(&ns->disable_done);
> percpu_ref_exit(&ns->ref);
>
> if (ns->pr.enable)
> nvmet_pr_exit_ns(ns);
>
> nvmet_fdp_exit_ns(ns);
>
> mutex_lock(&subsys->lock);
> nvmet_ns_changed(subsys, ns->nsid);
> nvmet_ns_dev_disable(ns);
> ns->enabled = false;
> mutex_unlock(&subsys->lock);
>
> And nvmet_ns_free() then erases and frees ns without an additional RCU
> grace period:
>
> nvmet_ns_disable(ns);
>
> mutex_lock(&subsys->lock);
>
> xa_erase(&subsys->namespaces, ns->nsid);
> ...
> kfree(ns->device_path);
> kfree(ns);
>
> Because xa_load() releases its internal RCU read lock before returning,
> nvmet_fdp_lookup_ns() holds an unpinned pointer to ns outside of an
> rcu_read_lock() critical section. A concurrent nvmet_ns_free() is not
> blocked by synchronize_rcu() in nvmet_ns_disable() and can free ns before
> nvmet_fdp_lookup_ns() dereferences ns->enabled, ns->fdp.enable, or
> &ns->ref.
>
> Additionally, because ns->enabled remains true until after
> percpu_ref_exit(&ns->ref) has freed the per-CPU counter data, checking
> ns->enabled instead of test_bit(NVMET_NS_IO_LIVE, &ns->flags) allows
> nvmet_fdp_lookup_ns() to call percpu_ref_tryget_live(&ns->ref) concurrently
> with or after percpu_ref_exit(&ns->ref).
>
> Should nvmet_fdp_lookup_ns() wrap the lookup and reference acquisition in
> rcu_read_lock() / rcu_read_unlock(), check test_bit(NVMET_NS_IO_LIVE,
> &ns->flags), and use percpu_ref_tryget_live_rcu(&ns->ref) in the same way
> as nvmet_req_find_ns()?
Sashiko is right, fixed in v2.
> [ ... ]
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 08/11] nvmet: forward the FDP I/O management commands to the backing device
2026-10-19 3:36 [PATCH RESEND 0/11] nvmet: FDP bridge for NVMe-oF targets Guixin Liu
` (6 preceding siblings ...)
2026-10-19 3:36 ` [PATCH 07/11] nvmet: implement the FDP feature Guixin Liu
@ 2026-10-19 3:36 ` Guixin Liu
2026-10-19 3:57 ` sashiko-bot
2026-10-19 3:36 ` [PATCH 09/11] nvmet: serve the FDP log pages Guixin Liu
` (2 subsequent siblings)
10 siblings, 1 reply; 22+ messages in thread
From: Guixin Liu @ 2026-10-19 3:36 UTC (permalink / raw)
To: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg,
Nilay Shroff, Daniel Wagner, Hannes Reinecke
Cc: linux-nvme
Dispatch the I/O Management Receive and Send commands from the I/O
command parser, with the transfer length derived from NUMD and NPID,
respectively, and report both commands as supported in the Commands
Supported and Effects log page.
Both are forwarded to the backing namespace: the data is staged
through a linear kernel buffer, which is what the backend submission
path takes, so the reclaim unit handle status hosts read reflects
the real reclaim units of the backing device, and a Reclaim Unit
Handle Update is carried out by the controller that owns that state.
When the namespace is enabled, learn the backing FDP geometry that
the forwarding needs: the Endurance Group Identifier the backing FDP
state is addressed by, and the placement identifier list of the
Reclaim Unit Handle Status. The bridge translates placement
identifiers to write streams by their index, so the enable fails
when the backing placement identifiers are not identity mapped.
Assisted-by: LLM
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
drivers/nvme/target/admin-cmd.c | 5 +
drivers/nvme/target/core.c | 26 ++++-
drivers/nvme/target/fdp.c | 177 +++++++++++++++++++++++++++++++-
drivers/nvme/target/nvmet.h | 12 +++
4 files changed, 217 insertions(+), 3 deletions(-)
diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 93fb9c3d500a3..34492cf02875a 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -394,6 +394,11 @@ static void nvmet_get_cmd_effects_nvm(struct nvme_effects_log *log)
log->iocs[nvme_cmd_write] =
log->iocs[nvme_cmd_write_zeroes] =
cpu_to_le32(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC);
+ if (IS_ENABLED(CONFIG_NVME_TARGET_FDP)) {
+ log->iocs[nvme_cmd_io_mgmt_recv] =
+ log->iocs[nvme_cmd_io_mgmt_send] =
+ cpu_to_le32(NVME_CMD_EFFECTS_CSUPP);
+ }
}
static void nvmet_get_cmd_effects_zns(struct nvme_effects_log *log)
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 4a6444fdfb2f8..70ae2581812bd 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1097,9 +1097,22 @@ static u32 nvmet_io_cmd_transfer_len(struct nvmet_req *req)
return nvmet_dsm_len(req);
case nvme_cmd_zone_mgmt_recv:
return (le32_to_cpu(req->cmd->zmr.numd) + 1) << 2;
+ case nvme_cmd_io_mgmt_recv:
+ if (IS_ENABLED(CONFIG_NVME_TARGET_FDP))
+ return (le32_to_cpu(req->cmd->imr.numd) + 1) << 2;
+ break;
+ case nvme_cmd_io_mgmt_send:
+ if (IS_ENABLED(CONFIG_NVME_TARGET_FDP)) {
+ u32 npid = le16_to_cpu(req->cmd->ims.mos);
+
+ return (npid + 1) * sizeof(__le16);
+ }
+ break;
default:
- return 0;
+ break;
}
+
+ return 0;
}
static u16 nvmet_parse_io_cmd(struct nvmet_req *req)
@@ -1141,6 +1154,17 @@ static u16 nvmet_parse_io_cmd(struct nvmet_req *req)
return ret;
}
+ if (IS_ENABLED(CONFIG_NVME_TARGET_FDP)) {
+ switch (req->cmd->common.opcode) {
+ case nvme_cmd_io_mgmt_recv:
+ req->execute = nvmet_execute_io_mgmt_recv;
+ return 0;
+ case nvme_cmd_io_mgmt_send:
+ req->execute = nvmet_execute_io_mgmt_send;
+ return 0;
+ }
+ }
+
switch (req->ns->csi) {
case NVME_CSI_NVM:
if (req->ns->file)
diff --git a/drivers/nvme/target/fdp.c b/drivers/nvme/target/fdp.c
index 0327970a9cb9d..6458efadbcb8e 100644
--- a/drivers/nvme/target/fdp.c
+++ b/drivers/nvme/target/fdp.c
@@ -34,6 +34,8 @@ bool nvmet_fdp_subsys_configured(struct nvmet_subsys *subsys)
return ret;
}
+static int nvmet_fdp_init_backend(struct nvmet_ns *ns);
+
int nvmet_fdp_init_ns(struct nvmet_ns *ns)
{
struct block_device *bdev = ns->bdev;
@@ -78,8 +80,9 @@ int nvmet_fdp_init_ns(struct nvmet_ns *ns)
backend_ns = bdev->bd_disk->private_data;
ns->fdp.backend_q = backend_ns->queue;
ns->fdp.backend_ctrl = backend_ns->ctrl;
+ ns->fdp.backend_nsid = backend_ns->head->ns_id;
- return 0;
+ return nvmet_fdp_init_backend(ns);
}
void nvmet_fdp_exit_ns(struct nvmet_ns *ns)
@@ -88,7 +91,6 @@ void nvmet_fdp_exit_ns(struct nvmet_ns *ns)
ns->fdp.backend_ctrl = NULL;
}
-
/*
* Each namespace is its own endurance group, so the Endurance Group
* Identifier in Command Dword 11, and in the Log Specific Identifier
@@ -160,3 +162,174 @@ u16 nvmet_get_feat_fdp(struct nvmet_req *req)
return 0;
}
+
+static u16 nvmet_fdp_io_mgmt_check(struct nvmet_req *req)
+{
+ if (!nvmet_fdp_enabled(req->ns))
+ return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
+
+ return 0;
+}
+
+/*
+ * The backend submission path takes a linear kernel buffer, so the
+ * request data is staged through one.
+ */
+static u16 nvmet_fdp_io_mgmt_forward(struct nvmet_req *req, bool recv)
+{
+ struct nvme_command c = {};
+ u16 status = 0;
+ void *buf;
+ int ret;
+
+ buf = kzalloc(req->transfer_len, GFP_KERNEL);
+ if (!buf)
+ return NVME_SC_INTERNAL;
+
+ if (!recv) {
+ status = nvmet_copy_from_sgl(req, 0, buf, req->transfer_len);
+ if (status)
+ goto free;
+ }
+
+ c.common = req->cmd->common;
+ c.common.nsid = cpu_to_le32(req->ns->fdp.backend_nsid);
+ ret = nvme_submit_sync_cmd(req->ns->fdp.backend_q, &c, buf,
+ req->transfer_len);
+ if (ret) {
+ status = ret > 0 ? ret : NVME_SC_INTERNAL;
+ goto free;
+ }
+
+ if (recv)
+ status = nvmet_copy_to_sgl(req, 0, buf, req->transfer_len);
+free:
+ kfree(buf);
+ return status;
+}
+
+void nvmet_execute_io_mgmt_recv(struct nvmet_req *req)
+{
+ u16 status;
+
+ status = nvmet_fdp_io_mgmt_check(req);
+ if (status)
+ goto out;
+
+ if (req->cmd->imr.mo != NVME_IO_MGMT_RECV_MO_RUHS) {
+ status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
+ goto out;
+ }
+
+ status = nvmet_fdp_io_mgmt_forward(req, true);
+out:
+ nvmet_req_complete(req, status);
+}
+
+void nvmet_execute_io_mgmt_send(struct nvmet_req *req)
+{
+ u16 status;
+
+ status = nvmet_fdp_io_mgmt_check(req);
+ if (status)
+ goto out;
+
+ if (req->cmd->ims.mo != NVME_IO_MGMT_SEND_MO_RUH_UPDATE) {
+ status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
+ goto out;
+ }
+
+ status = nvmet_fdp_io_mgmt_forward(req, false);
+out:
+ nvmet_req_complete(req, status);
+}
+
+static int nvmet_fdp_backend_identify(struct nvmet_ns *ns,
+ struct nvme_id_ns *id)
+{
+ struct nvme_command c = {};
+
+ c.identify.opcode = nvme_admin_identify;
+ c.identify.nsid = cpu_to_le32(ns->fdp.backend_nsid);
+ c.identify.cns = NVME_ID_CNS_NS;
+
+ return nvme_submit_sync_cmd(ns->fdp.backend_ctrl->admin_q, &c, id,
+ sizeof(*id));
+}
+
+static int nvmet_fdp_backend_ruhs(struct nvmet_ns *ns,
+ struct nvme_fdp_ruh_status *st)
+{
+ size_t len = struct_size(st, ruhsd, ns->fdp.nruh);
+ struct nvme_command c = {};
+
+ c.imr.opcode = nvme_cmd_io_mgmt_recv;
+ c.imr.nsid = cpu_to_le32(ns->fdp.backend_nsid);
+ c.imr.mo = NVME_IO_MGMT_RECV_MO_RUHS;
+ c.imr.numd = cpu_to_le32(nvme_bytes_to_numd(len));
+
+ return nvme_submit_sync_cmd(ns->fdp.backend_q, &c, st, len);
+}
+
+/*
+ * Learn the FDP geometry of the backing NVMe namespace: the
+ * Endurance Group Identifier its FDP state is addressed by, and the
+ * placement identifier list of the Reclaim Unit Handle Status. The
+ * placement translation relies on the identity mapping, so the
+ * enable fails when the backing geometry deviates from it.
+ */
+static int nvmet_fdp_init_backend(struct nvmet_ns *ns)
+{
+ struct nvme_fdp_ruh_status *st;
+ struct nvme_id_ns *id;
+ int ret, i;
+
+ id = kzalloc_obj(*id, GFP_KERNEL);
+ if (!id)
+ return -ENOMEM;
+
+ ret = nvmet_fdp_backend_identify(ns, id);
+ if (ret) {
+ pr_err("the ns:%u backing identify failed: %d\n",
+ ns->nsid, ret);
+ goto free_id;
+ }
+
+ ns->fdp.backend_endgid = le16_to_cpu(id->endgid);
+
+ st = kzalloc(struct_size(st, ruhsd, ns->fdp.nruh), GFP_KERNEL);
+ if (!st) {
+ ret = -ENOMEM;
+ goto free_id;
+ }
+
+ ret = nvmet_fdp_backend_ruhs(ns, st);
+ if (ret) {
+ pr_err("the ns:%u backing reclaim unit handle status failed: %d\n",
+ ns->nsid, ret);
+ goto free_st;
+ }
+
+ if (le16_to_cpu(st->nruhsd) < ns->fdp.nruh) {
+ pr_err("the ns:%u backing reports fewer handles than write streams\n",
+ ns->nsid);
+ ret = -EINVAL;
+ goto free_st;
+ }
+
+ for (i = 0; i < ns->fdp.nruh; i++) {
+ if (le16_to_cpu(st->ruhsd[i].pid) != i) {
+ pr_err("the ns:%u backing placement identifiers are not identity mapped\n",
+ ns->nsid);
+ ret = -EINVAL;
+ goto free_st;
+ }
+ }
+
+free_st:
+ kfree(st);
+free_id:
+ kfree(id);
+ return ret;
+}
+
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index e95bafe1fb019..037c3862062ab 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -103,6 +103,8 @@ struct nvmet_fdp {
u64 runs;
struct request_queue *backend_q;
struct nvme_ctrl *backend_ctrl;
+ u32 backend_nsid;
+ u16 backend_endgid;
};
struct nvmet_ns {
@@ -999,6 +1001,8 @@ void nvmet_fdp_exit_ns(struct nvmet_ns *ns);
bool nvmet_fdp_subsys_configured(struct nvmet_subsys *subsys);
u16 nvmet_set_feat_fdp(struct nvmet_req *req);
u16 nvmet_get_feat_fdp(struct nvmet_req *req);
+void nvmet_execute_io_mgmt_recv(struct nvmet_req *req);
+void nvmet_execute_io_mgmt_send(struct nvmet_req *req);
#else
static inline bool nvmet_fdp_enabled(struct nvmet_ns *ns)
{
@@ -1023,6 +1027,14 @@ static inline u16 nvmet_get_feat_fdp(struct nvmet_req *req)
{
return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
}
+static inline void nvmet_execute_io_mgmt_recv(struct nvmet_req *req)
+{
+ nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_STATUS_DNR);
+}
+static inline void nvmet_execute_io_mgmt_send(struct nvmet_req *req)
+{
+ nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_STATUS_DNR);
+}
#endif
/*
--
2.43.7
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 08/11] nvmet: forward the FDP I/O management commands to the backing device
2026-10-19 3:36 ` [PATCH 08/11] nvmet: forward the FDP I/O management commands to the backing device Guixin Liu
@ 2026-10-19 3:57 ` sashiko-bot
2026-10-19 9:44 ` Guixin Liu
0 siblings, 1 reply; 22+ messages in thread
From: sashiko-bot @ 2026-10-19 3:57 UTC (permalink / raw)
To: Guixin Liu; +Cc: linux-nvme
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] nvmet: missing transfer_len check and numd overflow in FDP io_mgmt
- [High] nvmet: FDP io_mgmt bypasses PR checks and fails recv on readonly ns
- [Medium] nvmet: nvmet_fdp_init_ns() returns positive status and leaks ptrs
--
commit 7240be6ef505919551b3b5eef502431a1b23ca3e
Author: Guixin Liu <kanie@linux.alibaba.com>
nvmet: forward the FDP I/O management commands to the backing device
This commit learns the backing namespace FDP geometry when enabling an
FDP namespace and forwards I/O Management Receive and Send commands to
the backing NVMe device via a linear kernel buffer.
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 14f55631cf502..cdf5a4c183edd 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -1110,9 +1110,22 @@ static u32 nvmet_io_cmd_transfer_len(struct nvmet_req *req)
> return nvmet_dsm_len(req);
> case nvme_cmd_zone_mgmt_recv:
> return (le32_to_cpu(req->cmd->zmr.numd) + 1) << 2;
> + case nvme_cmd_io_mgmt_recv:
> + if (IS_ENABLED(CONFIG_NVME_TARGET_FDP))
> + return (le32_to_cpu(req->cmd->imr.numd) + 1) << 2;
> + break;
> + case nvme_cmd_io_mgmt_send:
> + if (IS_ENABLED(CONFIG_NVME_TARGET_FDP)) {
> + u32 npid = le16_to_cpu(req->cmd->ims.mos);
> +
> + return (npid + 1) * sizeof(__le16);
> + }
> + break;
[Severity: Critical]
In nvmet_io_cmd_transfer_len(), can the 32-bit unsigned calculation
(le32_to_cpu(req->cmd->imr.numd) + 1) << 2 overflow and wrap to 0 or a
small truncated value when req->cmd->imr.numd is >= 0x3fffffff? Also, for
nvme_cmd_io_mgmt_send, does computing (npid + 1) * sizeof(__le16) assume
req->cmd->ims.mo is NVME_IO_MGMT_SEND_MO_RUH_UPDATE before checking mo?
> default:
> - return 0;
> + break;
> }
> +
> + return 0;
> }
>
> static u16 nvmet_parse_io_cmd(struct nvmet_req *req)
> @@ -1154,6 +1167,17 @@ static u16 nvmet_parse_io_cmd(struct nvmet_req *req)
> return ret;
> }
>
> + if (IS_ENABLED(CONFIG_NVME_TARGET_FDP)) {
> + switch (req->cmd->common.opcode) {
> + case nvme_cmd_io_mgmt_recv:
> + req->execute = nvmet_execute_io_mgmt_recv;
> + return 0;
> + case nvme_cmd_io_mgmt_send:
> + req->execute = nvmet_execute_io_mgmt_send;
> + return 0;
> + }
> + }
> +
[Severity: High]
Does returning 0 early here in nvmet_parse_io_cmd() bypass the persistent
reservation checks at the end of the function:
if (req->ns->pr.enable) {
ret = nvmet_pr_check_cmd_access(req);
if (ret)
return ret;
ret = nvmet_pr_get_ns_pc_ref(req);
}
In addition, neither nvme_cmd_io_mgmt_recv nor nvme_cmd_io_mgmt_send is
handled in nvmet_is_req_write_cmd_group() or nvmet_is_req_read_cmd_group()
in drivers/nvme/target/pr.c, so unauthorized hosts can issue Reclaim Unit
Handle Updates and reads despite an active reservation, and
nvmet_pr_do_abort() will not wait for in-flight I/O management commands.
Separately, earlier in nvmet_parse_io_cmd(), nvmet_io_cmd_check_access()
only allows nvme_cmd_read and nvme_cmd_flush when req->ns->readonly is
set:
if (unlikely(req->ns->readonly)) {
switch (req->cmd->common.opcode) {
case nvme_cmd_read:
case nvme_cmd_flush:
break;
default:
return NVME_SC_NS_WRITE_PROTECTED;
}
}
Should nvme_cmd_io_mgmt_recv also be permitted on a read-only namespace
instead of failing with NVME_SC_NS_WRITE_PROTECTED?
[ ... ]
> diff --git a/drivers/nvme/target/fdp.c b/drivers/nvme/target/fdp.c
> index 0327970a9cb9d..6458efadbcb8e 100644
> --- a/drivers/nvme/target/fdp.c
> +++ b/drivers/nvme/target/fdp.c
> @@ -34,6 +34,8 @@ bool nvmet_fdp_subsys_configured(struct nvmet_subsys *subsys)
> return ret;
> }
>
> +static int nvmet_fdp_init_backend(struct nvmet_ns *ns);
> +
> int nvmet_fdp_init_ns(struct nvmet_ns *ns)
> {
> struct block_device *bdev = ns->bdev;
> @@ -78,8 +80,9 @@ int nvmet_fdp_init_ns(struct nvmet_ns *ns)
> backend_ns = bdev->bd_disk->private_data;
> ns->fdp.backend_q = backend_ns->queue;
> ns->fdp.backend_ctrl = backend_ns->ctrl;
> + ns->fdp.backend_nsid = backend_ns->head->ns_id;
>
> - return 0;
> + return nvmet_fdp_init_backend(ns);
> }
[ ... ]
> +/*
> + * The backend submission path takes a linear kernel buffer, so the
> + * request data is staged through one.
> + */
> +static u16 nvmet_fdp_io_mgmt_forward(struct nvmet_req *req, bool recv)
> +{
> + struct nvme_command c = {};
> + u16 status = 0;
> + void *buf;
> + int ret;
> +
> + buf = kzalloc(req->transfer_len, GFP_KERNEL);
> + if (!buf)
> + return NVME_SC_INTERNAL;
> +
> + if (!recv) {
> + status = nvmet_copy_from_sgl(req, 0, buf, req->transfer_len);
> + if (status)
> + goto free;
> + }
> +
> + c.common = req->cmd->common;
> + c.common.nsid = cpu_to_le32(req->ns->fdp.backend_nsid);
> + ret = nvme_submit_sync_cmd(req->ns->fdp.backend_q, &c, buf,
> + req->transfer_len);
> + if (ret) {
> + status = ret > 0 ? ret : NVME_SC_INTERNAL;
> + goto free;
> + }
> +
> + if (recv)
> + status = nvmet_copy_to_sgl(req, 0, buf, req->transfer_len);
> +free:
> + kfree(buf);
> + return status;
> +}
> +
> +void nvmet_execute_io_mgmt_recv(struct nvmet_req *req)
> +{
> + u16 status;
> +
> + status = nvmet_fdp_io_mgmt_check(req);
> + if (status)
> + goto out;
> +
> + if (req->cmd->imr.mo != NVME_IO_MGMT_RECV_MO_RUHS) {
> + status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
> + goto out;
> + }
> +
> + status = nvmet_fdp_io_mgmt_forward(req, true);
> +out:
> + nvmet_req_complete(req, status);
> +}
> +
> +void nvmet_execute_io_mgmt_send(struct nvmet_req *req)
> +{
> + u16 status;
> +
> + status = nvmet_fdp_io_mgmt_check(req);
> + if (status)
> + goto out;
> +
> + if (req->cmd->ims.mo != NVME_IO_MGMT_SEND_MO_RUH_UPDATE) {
> + status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
> + goto out;
> + }
> +
> + status = nvmet_fdp_io_mgmt_forward(req, false);
> +out:
> + nvmet_req_complete(req, status);
> +}
[Severity: Critical]
Should nvmet_execute_io_mgmt_recv() and nvmet_execute_io_mgmt_send()
validate req->transfer_len with nvmet_check_transfer_len() before calling
nvmet_fdp_io_mgmt_forward(), and construct the forwarded command explicitly
rather than copying req->cmd->common?
On Fabrics transports (nvmet-tcp, nvmet-rdma, nvmet-fc, nvme-loop),
req->transfer_len comes from the transport SGL descriptor or FCP data_len,
while nvmet_req_transfer_len() -> nvmet_io_cmd_transfer_len() is only used
by nvmet-pci-epf.
Without nvmet_check_transfer_len(), nvmet_fdp_io_mgmt_forward() allocates
buf with req->transfer_len bytes, copies the raw 64-byte SQE via
c.common = req->cmd->common (including dptr, metadata, imr.numd, and
ims.mos), and passes both to nvme_submit_sync_cmd():
1. If a Fabrics host sends req->transfer_len == 0 with a non-zero imr.numd
or ims.mos, kzalloc(0, GFP_KERNEL) returns ZERO_SIZE_PTR, which passes
if (!buf). In __nvme_submit_sync_cmd(), nvme_init_request() clears
NVME_CMD_SGL_ALL in c.common.flags (selecting PRP mode) and skips
blk_rq_map_kern() because bufflen == 0. In nvme_prep_rq(),
blk_rq_nr_phys_segments(req) is 0, so nvme_map_data() is skipped and
iod->cmd.common.dptr.prp1/prp2 retain the raw host-supplied dptr bytes,
causing the backing PCIe NVMe controller to DMA directly to or from the
host-specified physical/IOVA address.
2. If a Fabrics host sends a non-zero req->transfer_len that is smaller
than the byte length encoded in c.imr.numd or c.ims.mos, kzalloc()
allocates the smaller slab buffer and blk_rq_map_kern() maps
req->transfer_len bytes. Because PRP1/PRP2 do not encode the buffer
length, the backing PCIe NVMe controller DMAs the full byte count from
c.imr.numd or c.ims.mos into or out of the smaller slab object,
overflowing the kernel heap buffer on Receive or reading out-of-bounds
kernel memory on Send.
[ ... ]
> +static int nvmet_fdp_init_backend(struct nvmet_ns *ns)
> +{
> + struct nvme_fdp_ruh_status *st;
> + struct nvme_id_ns *id;
> + int ret, i;
> +
> + id = kzalloc_obj(*id, GFP_KERNEL);
> + if (!id)
> + return -ENOMEM;
> +
> + ret = nvmet_fdp_backend_identify(ns, id);
> + if (ret) {
> + pr_err("the ns:%u backing identify failed: %d\n",
> + ns->nsid, ret);
> + goto free_id;
> + }
> +
> + ns->fdp.backend_endgid = le16_to_cpu(id->endgid);
> +
> + st = kzalloc(struct_size(st, ruhsd, ns->fdp.nruh), GFP_KERNEL);
> + if (!st) {
> + ret = -ENOMEM;
> + goto free_id;
> + }
> +
> + ret = nvmet_fdp_backend_ruhs(ns, st);
> + if (ret) {
> + pr_err("the ns:%u backing reclaim unit handle status failed: %d\n",
> + ns->nsid, ret);
> + goto free_st;
> + }
[Severity: Medium]
What happens if nvmet_fdp_backend_identify() or nvmet_fdp_backend_ruhs()
returns a positive NVMe status code from nvme_submit_sync_cmd(), or if
nvmet_fdp_init_backend() fails after nvmet_fdp_init_ns() has already set
ns->fdp.backend_q and ns->fdp.backend_ctrl?
nvme_submit_sync_cmd() returns a positive NVMe status code when the
backing controller completes the command with an error. Propagating that
positive value through nvmet_fdp_init_ns() -> nvmet_ns_enable() ->
nvmet_ns_enable_store() causes configfs to treat the positive return value
as the number of bytes written rather than a failure.
Also, when nvmet_fdp_init_ns() fails in nvmet_ns_enable():
ret = nvmet_fdp_init_ns(ns);
if (ret)
goto out_pr_exit;
error recovery jumps to out_pr_exit (skipping nvmet_fdp_exit_ns()) and
closes the block device in nvmet_ns_dev_disable(ns), leaving dangling
pointers in ns->fdp.backend_q and ns->fdp.backend_ctrl.
> +
> + if (le16_to_cpu(st->nruhsd) < ns->fdp.nruh) {
> + pr_err("the ns:%u backing reports fewer handles than write streams\n",
> + ns->nsid);
> + ret = -EINVAL;
> + goto free_st;
> + }
> +
> + for (i = 0; i < ns->fdp.nruh; i++) {
> + if (le16_to_cpu(st->ruhsd[i].pid) != i) {
> + pr_err("the ns:%u backing placement identifiers are not identity mapped\n",
> + ns->nsid);
> + ret = -EINVAL;
> + goto free_st;
> + }
> + }
> +
> +free_st:
> + kfree(st);
> +free_id:
> + kfree(id);
> + return ret;
> +}
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261019033624.402337-1-kanie@linux.alibaba.com?part=8
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 08/11] nvmet: forward the FDP I/O management commands to the backing device
2026-10-19 3:57 ` sashiko-bot
@ 2026-10-19 9:44 ` Guixin Liu
0 siblings, 0 replies; 22+ messages in thread
From: Guixin Liu @ 2026-10-19 9:44 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-nvme
在 2026/10/19 11:57, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Critical] nvmet: missing transfer_len check and numd overflow in FDP io_mgmt
> - [High] nvmet: FDP io_mgmt bypasses PR checks and fails recv on readonly ns
> - [Medium] nvmet: nvmet_fdp_init_ns() returns positive status and leaks ptrs
> --
>
> commit 7240be6ef505919551b3b5eef502431a1b23ca3e
> Author: Guixin Liu <kanie@linux.alibaba.com>
>
> nvmet: forward the FDP I/O management commands to the backing device
>
> This commit learns the backing namespace FDP geometry when enabling an
> FDP namespace and forwards I/O Management Receive and Send commands to
> the backing NVMe device via a linear kernel buffer.
>
>> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
>> index 14f55631cf502..cdf5a4c183edd 100644
>> --- a/drivers/nvme/target/core.c
>> +++ b/drivers/nvme/target/core.c
>> @@ -1110,9 +1110,22 @@ static u32 nvmet_io_cmd_transfer_len(struct nvmet_req *req)
>> return nvmet_dsm_len(req);
>> case nvme_cmd_zone_mgmt_recv:
>> return (le32_to_cpu(req->cmd->zmr.numd) + 1) << 2;
>> + case nvme_cmd_io_mgmt_recv:
>> + if (IS_ENABLED(CONFIG_NVME_TARGET_FDP))
>> + return (le32_to_cpu(req->cmd->imr.numd) + 1) << 2;
>> + break;
>> + case nvme_cmd_io_mgmt_send:
>> + if (IS_ENABLED(CONFIG_NVME_TARGET_FDP)) {
>> + u32 npid = le16_to_cpu(req->cmd->ims.mos);
>> +
>> + return (npid + 1) * sizeof(__le16);
>> + }
>> + break;
> [Severity: Critical]
> In nvmet_io_cmd_transfer_len(), can the 32-bit unsigned calculation
> (le32_to_cpu(req->cmd->imr.numd) + 1) << 2 overflow and wrap to 0 or a
> small truncated value when req->cmd->imr.numd is >= 0x3fffffff? Also, for
> nvme_cmd_io_mgmt_send, does computing (npid + 1) * sizeof(__le16) assume
> req->cmd->ims.mo is NVME_IO_MGMT_SEND_MO_RUH_UPDATE before checking mo?
Well, this is not possible, but I can add a defence.
>> default:
>> - return 0;
>> + break;
>> }
>> +
>> + return 0;
>> }
>>
>> static u16 nvmet_parse_io_cmd(struct nvmet_req *req)
>> @@ -1154,6 +1167,17 @@ static u16 nvmet_parse_io_cmd(struct nvmet_req *req)
>> return ret;
>> }
>>
>> + if (IS_ENABLED(CONFIG_NVME_TARGET_FDP)) {
>> + switch (req->cmd->common.opcode) {
>> + case nvme_cmd_io_mgmt_recv:
>> + req->execute = nvmet_execute_io_mgmt_recv;
>> + return 0;
>> + case nvme_cmd_io_mgmt_send:
>> + req->execute = nvmet_execute_io_mgmt_send;
>> + return 0;
>> + }
>> + }
>> +
> [Severity: High]
> Does returning 0 early here in nvmet_parse_io_cmd() bypass the persistent
> reservation checks at the end of the function:
>
> if (req->ns->pr.enable) {
> ret = nvmet_pr_check_cmd_access(req);
> if (ret)
> return ret;
>
> ret = nvmet_pr_get_ns_pc_ref(req);
> }
>
> In addition, neither nvme_cmd_io_mgmt_recv nor nvme_cmd_io_mgmt_send is
> handled in nvmet_is_req_write_cmd_group() or nvmet_is_req_read_cmd_group()
> in drivers/nvme/target/pr.c, so unauthorized hosts can issue Reclaim Unit
> Handle Updates and reads despite an active reservation, and
> nvmet_pr_do_abort() will not wait for in-flight I/O management commands.
io mgmt is not controled by reservation, so no need fix.
> Separately, earlier in nvmet_parse_io_cmd(), nvmet_io_cmd_check_access()
> only allows nvme_cmd_read and nvme_cmd_flush when req->ns->readonly is
> set:
>
> if (unlikely(req->ns->readonly)) {
> switch (req->cmd->common.opcode) {
> case nvme_cmd_read:
> case nvme_cmd_flush:
> break;
> default:
> return NVME_SC_NS_WRITE_PROTECTED;
> }
> }
>
> Should nvme_cmd_io_mgmt_recv also be permitted on a read-only namespace
> instead of failing with NVME_SC_NS_WRITE_PROTECTED?
This is right, fixed in v2.
>
> [ ... ]
>
>> diff --git a/drivers/nvme/target/fdp.c b/drivers/nvme/target/fdp.c
>> index 0327970a9cb9d..6458efadbcb8e 100644
>> --- a/drivers/nvme/target/fdp.c
>> +++ b/drivers/nvme/target/fdp.c
>> @@ -34,6 +34,8 @@ bool nvmet_fdp_subsys_configured(struct nvmet_subsys *subsys)
>> return ret;
>> }
>>
>> +static int nvmet_fdp_init_backend(struct nvmet_ns *ns);
>> +
>> int nvmet_fdp_init_ns(struct nvmet_ns *ns)
>> {
>> struct block_device *bdev = ns->bdev;
>> @@ -78,8 +80,9 @@ int nvmet_fdp_init_ns(struct nvmet_ns *ns)
>> backend_ns = bdev->bd_disk->private_data;
>> ns->fdp.backend_q = backend_ns->queue;
>> ns->fdp.backend_ctrl = backend_ns->ctrl;
>> + ns->fdp.backend_nsid = backend_ns->head->ns_id;
>>
>> - return 0;
>> + return nvmet_fdp_init_backend(ns);
>> }
> [ ... ]
>
>> +/*
>> + * The backend submission path takes a linear kernel buffer, so the
>> + * request data is staged through one.
>> + */
>> +static u16 nvmet_fdp_io_mgmt_forward(struct nvmet_req *req, bool recv)
>> +{
>> + struct nvme_command c = {};
>> + u16 status = 0;
>> + void *buf;
>> + int ret;
>> +
>> + buf = kzalloc(req->transfer_len, GFP_KERNEL);
>> + if (!buf)
>> + return NVME_SC_INTERNAL;
>> +
>> + if (!recv) {
>> + status = nvmet_copy_from_sgl(req, 0, buf, req->transfer_len);
>> + if (status)
>> + goto free;
>> + }
>> +
>> + c.common = req->cmd->common;
>> + c.common.nsid = cpu_to_le32(req->ns->fdp.backend_nsid);
>> + ret = nvme_submit_sync_cmd(req->ns->fdp.backend_q, &c, buf,
>> + req->transfer_len);
>> + if (ret) {
>> + status = ret > 0 ? ret : NVME_SC_INTERNAL;
>> + goto free;
>> + }
>> +
>> + if (recv)
>> + status = nvmet_copy_to_sgl(req, 0, buf, req->transfer_len);
>> +free:
>> + kfree(buf);
>> + return status;
>> +}
>> +
>> +void nvmet_execute_io_mgmt_recv(struct nvmet_req *req)
>> +{
>> + u16 status;
>> +
>> + status = nvmet_fdp_io_mgmt_check(req);
>> + if (status)
>> + goto out;
>> +
>> + if (req->cmd->imr.mo != NVME_IO_MGMT_RECV_MO_RUHS) {
>> + status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
>> + goto out;
>> + }
>> +
>> + status = nvmet_fdp_io_mgmt_forward(req, true);
>> +out:
>> + nvmet_req_complete(req, status);
>> +}
>> +
>> +void nvmet_execute_io_mgmt_send(struct nvmet_req *req)
>> +{
>> + u16 status;
>> +
>> + status = nvmet_fdp_io_mgmt_check(req);
>> + if (status)
>> + goto out;
>> +
>> + if (req->cmd->ims.mo != NVME_IO_MGMT_SEND_MO_RUH_UPDATE) {
>> + status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
>> + goto out;
>> + }
>> +
>> + status = nvmet_fdp_io_mgmt_forward(req, false);
>> +out:
>> + nvmet_req_complete(req, status);
>> +}
> [Severity: Critical]
> Should nvmet_execute_io_mgmt_recv() and nvmet_execute_io_mgmt_send()
> validate req->transfer_len with nvmet_check_transfer_len() before calling
> nvmet_fdp_io_mgmt_forward(), and construct the forwarded command explicitly
> rather than copying req->cmd->common?
>
> On Fabrics transports (nvmet-tcp, nvmet-rdma, nvmet-fc, nvme-loop),
> req->transfer_len comes from the transport SGL descriptor or FCP data_len,
> while nvmet_req_transfer_len() -> nvmet_io_cmd_transfer_len() is only used
> by nvmet-pci-epf.
>
> Without nvmet_check_transfer_len(), nvmet_fdp_io_mgmt_forward() allocates
> buf with req->transfer_len bytes, copies the raw 64-byte SQE via
> c.common = req->cmd->common (including dptr, metadata, imr.numd, and
> ims.mos), and passes both to nvme_submit_sync_cmd():
>
> 1. If a Fabrics host sends req->transfer_len == 0 with a non-zero imr.numd
> or ims.mos, kzalloc(0, GFP_KERNEL) returns ZERO_SIZE_PTR, which passes
> if (!buf). In __nvme_submit_sync_cmd(), nvme_init_request() clears
> NVME_CMD_SGL_ALL in c.common.flags (selecting PRP mode) and skips
> blk_rq_map_kern() because bufflen == 0. In nvme_prep_rq(),
> blk_rq_nr_phys_segments(req) is 0, so nvme_map_data() is skipped and
> iod->cmd.common.dptr.prp1/prp2 retain the raw host-supplied dptr bytes,
> causing the backing PCIe NVMe controller to DMA directly to or from the
> host-specified physical/IOVA address.
>
> 2. If a Fabrics host sends a non-zero req->transfer_len that is smaller
> than the byte length encoded in c.imr.numd or c.ims.mos, kzalloc()
> allocates the smaller slab buffer and blk_rq_map_kern() maps
> req->transfer_len bytes. Because PRP1/PRP2 do not encode the buffer
> length, the backing PCIe NVMe controller DMAs the full byte count from
> c.imr.numd or c.ims.mos into or out of the smaller slab object,
> overflowing the kernel heap buffer on Receive or reading out-of-bounds
> kernel memory on Send.
Sashiko is right, fixed in v2.
>
> [ ... ]
>
>> +static int nvmet_fdp_init_backend(struct nvmet_ns *ns)
>> +{
>> + struct nvme_fdp_ruh_status *st;
>> + struct nvme_id_ns *id;
>> + int ret, i;
>> +
>> + id = kzalloc_obj(*id, GFP_KERNEL);
>> + if (!id)
>> + return -ENOMEM;
>> +
>> + ret = nvmet_fdp_backend_identify(ns, id);
>> + if (ret) {
>> + pr_err("the ns:%u backing identify failed: %d\n",
>> + ns->nsid, ret);
>> + goto free_id;
>> + }
>> +
>> + ns->fdp.backend_endgid = le16_to_cpu(id->endgid);
>> +
>> + st = kzalloc(struct_size(st, ruhsd, ns->fdp.nruh), GFP_KERNEL);
>> + if (!st) {
>> + ret = -ENOMEM;
>> + goto free_id;
>> + }
>> +
>> + ret = nvmet_fdp_backend_ruhs(ns, st);
>> + if (ret) {
>> + pr_err("the ns:%u backing reclaim unit handle status failed: %d\n",
>> + ns->nsid, ret);
>> + goto free_st;
>> + }
> [Severity: Medium]
> What happens if nvmet_fdp_backend_identify() or nvmet_fdp_backend_ruhs()
> returns a positive NVMe status code from nvme_submit_sync_cmd(), or if
> nvmet_fdp_init_backend() fails after nvmet_fdp_init_ns() has already set
> ns->fdp.backend_q and ns->fdp.backend_ctrl?
>
> nvme_submit_sync_cmd() returns a positive NVMe status code when the
> backing controller completes the command with an error. Propagating that
> positive value through nvmet_fdp_init_ns() -> nvmet_ns_enable() ->
> nvmet_ns_enable_store() causes configfs to treat the positive return value
> as the number of bytes written rather than a failure.
>
> Also, when nvmet_fdp_init_ns() fails in nvmet_ns_enable():
>
> ret = nvmet_fdp_init_ns(ns);
> if (ret)
> goto out_pr_exit;
>
> error recovery jumps to out_pr_exit (skipping nvmet_fdp_exit_ns()) and
> closes the block device in nvmet_ns_dev_disable(ns), leaving dangling
> pointers in ns->fdp.backend_q and ns->fdp.backend_ctrl.
Sure, fixed in v2.
>
>> +
>> + if (le16_to_cpu(st->nruhsd) < ns->fdp.nruh) {
>> + pr_err("the ns:%u backing reports fewer handles than write streams\n",
>> + ns->nsid);
>> + ret = -EINVAL;
>> + goto free_st;
>> + }
>> +
>> + for (i = 0; i < ns->fdp.nruh; i++) {
>> + if (le16_to_cpu(st->ruhsd[i].pid) != i) {
>> + pr_err("the ns:%u backing placement identifiers are not identity mapped\n",
>> + ns->nsid);
>> + ret = -EINVAL;
>> + goto free_st;
>> + }
>> + }
>> +
>> +free_st:
>> + kfree(st);
>> +free_id:
>> + kfree(id);
>> + return ret;
>> +}
> [ ... ]
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 09/11] nvmet: serve the FDP log pages
2026-10-19 3:36 [PATCH RESEND 0/11] nvmet: FDP bridge for NVMe-oF targets Guixin Liu
` (7 preceding siblings ...)
2026-10-19 3:36 ` [PATCH 08/11] nvmet: forward the FDP I/O management commands to the backing device Guixin Liu
@ 2026-10-19 3:36 ` Guixin Liu
2026-10-19 4:07 ` sashiko-bot
2026-10-19 3:36 ` [PATCH 10/11] nvmet: translate placement identifiers of writes to write streams Guixin Liu
2026-10-19 3:36 ` [PATCH 11/11] nvmet: add an FDP write translation tracepoint Guixin Liu
10 siblings, 1 reply; 22+ messages in thread
From: Guixin Liu @ 2026-10-19 3:36 UTC (permalink / raw)
To: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg,
Nilay Shroff, Daniel Wagner, Hannes Reinecke
Cc: linux-nvme
The bridge advertises the FDP log pages 20h to 23h as supported;
serve them.
The FDP Configurations log page (LID 20h) is constructed from the
snapshot taken at namespace enable time: one valid configuration
with a single reclaim group, the backing number of reclaim unit
handles, all of the Initially Isolated type, and the backing reclaim
unit nominal size. The page is addressed by the Endurance Group
Identifier in the Log Specific Identifier field. Hosts first
request the 16 byte header to learn the page size and then the full
page; both request sizes are served from the same construction path.
The remaining pages are owned by the backing controller and are
forwarded to it over its admin queue, addressed by the Endurance
Group Identifier of the backing endurance group, so hosts read the
real reclaim unit handle usage, counters and events of the backing
device.
Assisted-by: LLM
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
drivers/nvme/target/admin-cmd.c | 16 ++++++
drivers/nvme/target/fdp.c | 98 ++++++++++++++++++++++++++++++++-
drivers/nvme/target/nvmet.h | 14 +++++
3 files changed, 127 insertions(+), 1 deletion(-)
diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 34492cf02875a..08bec1259dd94 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -220,6 +220,16 @@ static void nvmet_execute_get_supported_log_pages(struct nvmet_req *req)
logs->lids[NVME_LOG_FEATURES] = cpu_to_le32(NVME_LIDS_LSUPP);
logs->lids[NVME_LOG_RMI] = cpu_to_le32(NVME_LIDS_LSUPP);
logs->lids[NVME_LOG_RESERVATION] = cpu_to_le32(NVME_LIDS_LSUPP);
+ if (nvmet_fdp_subsys_configured(nvmet_req_subsys(req))) {
+ logs->lids[NVME_LOG_FDP_CONFIGS] =
+ cpu_to_le32(NVME_LIDS_LSUPP);
+ logs->lids[NVME_LOG_FDP_RUH_USAGE] =
+ cpu_to_le32(NVME_LIDS_LSUPP);
+ logs->lids[NVME_LOG_FDP_STATS] =
+ cpu_to_le32(NVME_LIDS_LSUPP);
+ logs->lids[NVME_LOG_FDP_EVENTS] =
+ cpu_to_le32(NVME_LIDS_LSUPP);
+ }
status = nvmet_copy_to_sgl(req, 0, logs, sizeof(*logs));
kfree(logs);
@@ -648,6 +658,12 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
return nvmet_execute_get_log_page_rmi(req);
case NVME_LOG_RESERVATION:
return nvmet_execute_get_log_page_resv(req);
+ case NVME_LOG_FDP_CONFIGS:
+ return nvmet_execute_get_log_page_fdp_configs(req);
+ case NVME_LOG_FDP_RUH_USAGE:
+ case NVME_LOG_FDP_STATS:
+ case NVME_LOG_FDP_EVENTS:
+ return nvmet_fdp_log_forward(req);
}
pr_debug("unhandled lid %d on qid %d\n",
req->cmd->get_log_page.lid, req->sq->qid);
diff --git a/drivers/nvme/target/fdp.c b/drivers/nvme/target/fdp.c
index 6458efadbcb8e..8c0bc297471ec 100644
--- a/drivers/nvme/target/fdp.c
+++ b/drivers/nvme/target/fdp.c
@@ -162,7 +162,6 @@ u16 nvmet_get_feat_fdp(struct nvmet_req *req)
return 0;
}
-
static u16 nvmet_fdp_io_mgmt_check(struct nvmet_req *req)
{
if (!nvmet_fdp_enabled(req->ns))
@@ -333,3 +332,100 @@ static int nvmet_fdp_init_backend(struct nvmet_ns *ns)
return ret;
}
+void nvmet_execute_get_log_page_fdp_configs(struct nvmet_req *req)
+{
+ u32 lsi = le16_to_cpu(req->cmd->get_log_page.lsi);
+ struct nvme_fdp_config_desc *desc;
+ struct nvme_fdp_config_log *log;
+ struct nvmet_ns *ns;
+ size_t dsze, total;
+ u16 status = 0;
+ int i;
+
+ ns = nvmet_fdp_lookup_ns(req, lsi);
+ if (!ns) {
+ status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
+ goto out;
+ }
+
+ dsze = ALIGN(struct_size(desc, ruhs, ns->fdp.nruh), 8);
+ total = sizeof(*log) + dsze;
+
+ log = kzalloc(req->transfer_len, GFP_KERNEL);
+ if (!log) {
+ status = NVME_SC_INTERNAL;
+ goto out_put;
+ }
+
+ if (req->transfer_len >= sizeof(*log)) {
+ /* single configuration, 0's based */
+ log->numfdpc = 0;
+ log->ver = 0;
+ log->sze = cpu_to_le32(total);
+ }
+ if (req->transfer_len >= total) {
+ desc = (void *)(log + 1);
+ desc->dsze = cpu_to_le16(dsze);
+ /* FDPCV set, FDPVWC clear, RGIF 0 for the single RG */
+ desc->fdpa = 1 << 7;
+ desc->nrg = cpu_to_le32(1);
+ desc->nruh = cpu_to_le16(ns->fdp.nruh);
+ desc->maxpids = cpu_to_le16(ns->fdp.nruh - 1);
+ desc->nns = cpu_to_le32(1);
+ desc->runs = cpu_to_le64(ns->fdp.runs);
+ /* all handles Initially Isolated */
+ for (i = 0; i < ns->fdp.nruh; i++)
+ desc->ruhs[i].ruht = 0x1;
+ }
+
+ status = nvmet_copy_to_sgl(req, 0, log, req->transfer_len);
+ kfree(log);
+out_put:
+ percpu_ref_put(&ns->ref);
+out:
+ nvmet_req_complete(req, status);
+}
+
+/*
+ * Forward an FDP log page that the backing controller owns, addressed
+ * by the Endurance Group Identifier of the backing endurance group.
+ */
+void nvmet_fdp_log_forward(struct nvmet_req *req)
+{
+ struct nvme_command c = {};
+ struct nvmet_ns *ns;
+ u16 status = 0;
+ void *buf;
+ int ret;
+
+ ns = nvmet_fdp_lookup_ns(req,
+ le16_to_cpu(req->cmd->get_log_page.lsi));
+ if (!ns) {
+ status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
+ goto out;
+ }
+
+ buf = kzalloc(req->transfer_len, GFP_KERNEL);
+ if (!buf) {
+ status = NVME_SC_INTERNAL;
+ goto out_put;
+ }
+
+ c = *req->cmd;
+ c.get_log_page.nsid = cpu_to_le32(0);
+ c.get_log_page.lsi = cpu_to_le16(ns->fdp.backend_endgid);
+ ret = nvme_submit_sync_cmd(ns->fdp.backend_ctrl->admin_q, &c, buf,
+ req->transfer_len);
+ if (ret) {
+ status = ret > 0 ? ret : NVME_SC_INTERNAL;
+ goto free;
+ }
+
+ status = nvmet_copy_to_sgl(req, 0, buf, req->transfer_len);
+free:
+ kfree(buf);
+out_put:
+ percpu_ref_put(&ns->ref);
+out:
+ nvmet_req_complete(req, status);
+}
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 037c3862062ab..5bcc719973a57 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -1003,6 +1003,8 @@ u16 nvmet_set_feat_fdp(struct nvmet_req *req);
u16 nvmet_get_feat_fdp(struct nvmet_req *req);
void nvmet_execute_io_mgmt_recv(struct nvmet_req *req);
void nvmet_execute_io_mgmt_send(struct nvmet_req *req);
+void nvmet_execute_get_log_page_fdp_configs(struct nvmet_req *req);
+void nvmet_fdp_log_forward(struct nvmet_req *req);
#else
static inline bool nvmet_fdp_enabled(struct nvmet_ns *ns)
{
@@ -1035,6 +1037,18 @@ static inline void nvmet_execute_io_mgmt_send(struct nvmet_req *req)
{
nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_STATUS_DNR);
}
+static inline void nvmet_execute_get_log_page_fdp_configs(struct nvmet_req *req)
+{
+ nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_STATUS_DNR);
+}
+static inline void nvmet_fdp_log_forward(struct nvmet_req *req)
+{
+ nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_STATUS_DNR);
+}
+static inline u8 nvmet_fdp_write_stream(struct nvmet_req *req)
+{
+ return 0;
+}
#endif
/*
--
2.43.7
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 09/11] nvmet: serve the FDP log pages
2026-10-19 3:36 ` [PATCH 09/11] nvmet: serve the FDP log pages Guixin Liu
@ 2026-10-19 4:07 ` sashiko-bot
0 siblings, 0 replies; 22+ messages in thread
From: sashiko-bot @ 2026-10-19 4:07 UTC (permalink / raw)
To: Guixin Liu; +Cc: linux-nvme
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] nvmet: unvalidated numd and raw cmd copy in nvmet_fdp_log_forward()
- [High] nvmet: race and UAF in nvmet_fdp_lookup_ns() during log page lookup
- [Medium] nvmet: wrong buffer sizing and missing lpo handling in FDP log handlers
--
commit ed3a3e1b914df9ead808e45ef64cb2a7ed4fbd7a
Author: Guixin Liu <kanie@linux.alibaba.com>
nvmet: serve the FDP log pages
This commit adds support for serving the FDP log pages (20h to 23h) in the
NVMe target by constructing the FDP Configurations log page locally and
forwarding the remaining FDP log pages to the backing controller.
> diff --git a/drivers/nvme/target/fdp.c b/drivers/nvme/target/fdp.c
> index 6458efadbcb8e..8c0bc297471ec 100644
> --- a/drivers/nvme/target/fdp.c
> +++ b/drivers/nvme/target/fdp.c
[ ... ]
> @@ -333,3 +332,100 @@ static int nvmet_fdp_init_backend(struct nvmet_ns *ns)
> return ret;
> }
>
> +void nvmet_execute_get_log_page_fdp_configs(struct nvmet_req *req)
> +{
> + u32 lsi = le16_to_cpu(req->cmd->get_log_page.lsi);
> + struct nvme_fdp_config_desc *desc;
> + struct nvme_fdp_config_log *log;
> + struct nvmet_ns *ns;
> + size_t dsze, total;
> + u16 status = 0;
> + int i;
> +
> + ns = nvmet_fdp_lookup_ns(req, lsi);
> + if (!ns) {
> + status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
> + goto out;
> + }
[Severity: High]
Can calling nvmet_fdp_lookup_ns() here and in nvmet_fdp_log_forward() race
with nvmet_ns_disable() and nvmet_ns_free(), leading to a use-after-free on
ns or ns->ref.data, or a NULL pointer dereference on
ns->fdp.backend_ctrl->admin_q?
In drivers/nvme/target/fdp.c, nvmet_fdp_lookup_ns() looks up the namespace
without holding rcu_read_lock() or checking NVMET_NS_IO_LIVE:
drivers/nvme/target/fdp.c:nvmet_fdp_lookup_ns() {
ns = xa_load(&subsys->namespaces, endgid);
if (!ns || !ns->enabled || !nvmet_fdp_enabled(ns))
return NULL;
if (!percpu_ref_tryget_live(&ns->ref))
return NULL;
return ns;
}
Whereas in drivers/nvme/target/core.c, nvmet_ns_disable() clears
NVMET_NS_IO_LIVE, waits for an RCU grace period, exits ns->ref, and tears
down FDP state before setting ns->enabled to false:
drivers/nvme/target/core.c:nvmet_ns_disable() {
clear_bit(NVMET_NS_IO_LIVE, &ns->flags);
mutex_unlock(&subsys->lock);
percpu_ref_kill(&ns->ref);
synchronize_rcu();
wait_for_completion(&ns->disable_done);
percpu_ref_exit(&ns->ref);
...
nvmet_fdp_exit_ns(ns);
mutex_lock(&subsys->lock);
nvmet_ns_changed(subsys, ns->nsid);
nvmet_ns_dev_disable(ns);
ns->enabled = false;
mutex_unlock(&subsys->lock);
}
Because nvmet_fdp_lookup_ns() does not hold rcu_read_lock() across
xa_load() and percpu_ref_tryget_live_rcu(), synchronize_rcu() in
nvmet_ns_disable() will not wait for a thread inside nvmet_fdp_lookup_ns(),
allowing percpu_ref_exit(&ns->ref) and kfree(ns) in nvmet_ns_free() to run
while nvmet_fdp_lookup_ns() is still accessing ns and ns->ref.
Additionally, because ns->enabled stays true while percpu_ref_exit() and
nvmet_fdp_exit_ns() run, if the namespace is disabled, reconfigured with
FDP disabled, and re-enabled concurrently, percpu_ref_tryget_live(&ns->ref)
can succeed on the re-initialized reference after nvmet_fdp_exit_ns() has
cleared ns->fdp.backend_ctrl to NULL.
Should nvmet_fdp_lookup_ns() wrap the lookup in rcu_read_lock() /
rcu_read_unlock(), test NVMET_NS_IO_LIVE, and use
percpu_ref_tryget_live_rcu() like nvmet_req_find_ns() does, while re-checking
nvmet_fdp_enabled(ns) after acquiring the reference?
> +
> + dsze = ALIGN(struct_size(desc, ruhs, ns->fdp.nruh), 8);
> + total = sizeof(*log) + dsze;
> +
> + log = kzalloc(req->transfer_len, GFP_KERNEL);
> + if (!log) {
> + status = NVME_SC_INTERNAL;
> + goto out_put;
> + }
> +
> + if (req->transfer_len >= sizeof(*log)) {
> + /* single configuration, 0's based */
> + log->numfdpc = 0;
> + log->ver = 0;
> + log->sze = cpu_to_le32(total);
> + }
> + if (req->transfer_len >= total) {
> + desc = (void *)(log + 1);
> + desc->dsze = cpu_to_le16(dsze);
> + /* FDPCV set, FDPVWC clear, RGIF 0 for the single RG */
> + desc->fdpa = 1 << 7;
> + desc->nrg = cpu_to_le32(1);
> + desc->nruh = cpu_to_le16(ns->fdp.nruh);
> + desc->maxpids = cpu_to_le16(ns->fdp.nruh - 1);
> + desc->nns = cpu_to_le32(1);
> + desc->runs = cpu_to_le64(ns->fdp.runs);
> + /* all handles Initially Isolated */
> + for (i = 0; i < ns->fdp.nruh; i++)
> + desc->ruhs[i].ruht = 0x1;
> + }
> +
> + status = nvmet_copy_to_sgl(req, 0, log, req->transfer_len);
> + kfree(log);
[Severity: Medium]
What happens if a host issues a partial read with req->transfer_len between
sizeof(*log) and total, or uses a non-zero Log Page Offset (lpo), or passes
a very large req->transfer_len?
If a host requests a partial transfer where sizeof(*log) < req->transfer_len
< total (for example, reading the header plus the fixed portion of
nvme_fdp_config_desc), the req->transfer_len >= total check is false, so
desc is left completely zeroed in the returned buffer. Similarly, if a host
reads the header at lpo = 0 and then reads the descriptor at lpo = 16
(since nvmet_execute_identify_ctrl() advertises NVME_CTRL_LPA_LPOS in
id->lpa), nvmet_execute_get_log_page_fdp_configs() ignores
nvmet_get_log_page_offset(req->cmd) and returns the header again at offset 0.
Also, both nvmet_execute_get_log_page_fdp_configs() and
nvmet_fdp_log_forward() pass the host-controlled req->transfer_len directly
to kzalloc(..., GFP_KERNEL) without size bounds or __GFP_NOWARN, which can
trigger a page allocator warning when req->transfer_len > KMALLOC_MAX_SIZE.
Since total is at most 1104 bytes (given ns->fdp.nruh <= U8_MAX), could
nvmet_execute_get_log_page_fdp_configs() allocate and populate the fixed
total-sized buffer unconditionally, and then copy the requested slice taking
nvmet_get_log_page_offset(req->cmd) into account?
> +out_put:
> + percpu_ref_put(&ns->ref);
> +out:
> + nvmet_req_complete(req, status);
> +}
> +
> +/*
> + * Forward an FDP log page that the backing controller owns, addressed
> + * by the Endurance Group Identifier of the backing endurance group.
> + */
> +void nvmet_fdp_log_forward(struct nvmet_req *req)
> +{
> + struct nvme_command c = {};
> + struct nvmet_ns *ns;
> + u16 status = 0;
> + void *buf;
> + int ret;
> +
> + ns = nvmet_fdp_lookup_ns(req,
> + le16_to_cpu(req->cmd->get_log_page.lsi));
> + if (!ns) {
> + status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
> + goto out;
> + }
> +
> + buf = kzalloc(req->transfer_len, GFP_KERNEL);
> + if (!buf) {
> + status = NVME_SC_INTERNAL;
> + goto out_put;
> + }
> +
> + c = *req->cmd;
> + c.get_log_page.nsid = cpu_to_le32(0);
> + c.get_log_page.lsi = cpu_to_le16(ns->fdp.backend_endgid);
> + ret = nvme_submit_sync_cmd(ns->fdp.backend_ctrl->admin_q, &c, buf,
> + req->transfer_len);
[Severity: Critical]
Can a host wrap the 32-bit length calculation in nvmet_get_log_page_len()
and cause nvmet_fdp_log_forward() to submit an unsanitized command with a
mismatched buffer size or untrusted dptr to the backing controller's admin
queue?
In drivers/nvme/target/admin-cmd.c, nvmet_execute_get_log_page() checks
req->transfer_len against nvmet_get_log_page_len(req->cmd):
drivers/nvme/target/admin-cmd.c:nvmet_get_log_page_len() {
u32 len = le16_to_cpu(cmd->get_log_page.numdu);
len <<= 16;
len += le16_to_cpu(cmd->get_log_page.numdl);
/* NUMD is a 0's based value */
len += 1;
len *= sizeof(u32);
return len;
}
When (numdu << 16) + numdl >= 0x3fffffff, the 32-bit calculation in
nvmet_get_log_page_len() wraps around:
1. If a host sends NUMD = 0x3fffffff (or 0xffffffff) with a 0-byte SGL
(req->transfer_len == 0), nvmet_get_log_page_len() wraps to 0 and
nvmet_check_transfer_len() passes. In nvmet_fdp_log_forward(),
kzalloc(0, GFP_KERNEL) returns ZERO_SIZE_PTR, and c = *req->cmd copies the
host's raw command including req->cmd->common.dptr and the large
numdu/numdl. In __nvme_submit_sync_cmd():
drivers/nvme/host/core.c:__nvme_submit_sync_cmd() {
nvme_init_request(req, cmd);
if (flags & NVME_SUBMIT_RETRY)
req->cmd_flags &= ~REQ_FAILFAST_DRIVER;
if (buffer && bufflen) {
ret = blk_rq_map_kern(req, buffer, bufflen, GFP_KERNEL);
if (ret)
goto out;
}
}
Because bufflen is 0, blk_rq_map_kern() and subsequent DMA mapping are
skipped after nvme_init_request() has cleared NVME_CMD_SGL_ALL, so the
backing controller receives a Get Log Page command in PRP mode with a
multi-gigabyte NUMD and the host's raw dptr.prp1/prp2 values.
2. If a host sends NUMD = 0x40000000 with a 4-byte SGL (req->transfer_len
== 4), nvmet_get_log_page_len() wraps to 4. nvmet_fdp_log_forward()
allocates a 4-byte slab buffer, but forwards c with numdu = 0x4000 to the
PCIe admin queue (which uses PRPs without a buffer length field), causing
the backing controller to DMA the full log page into the 4-byte slab buffer
and overflow adjacent kernel heap memory.
Instead of copying *req->cmd raw, should nvmet_fdp_log_forward() construct
a clean struct nvme_command from validated fields (or use a helper like
nvme_get_log_ext()) and reject zero or oversized transfer lengths?
> + if (ret) {
> + status = ret > 0 ? ret : NVME_SC_INTERNAL;
> + goto free;
> + }
> +
> + status = nvmet_copy_to_sgl(req, 0, buf, req->transfer_len);
> +free:
> + kfree(buf);
> +out_put:
> + percpu_ref_put(&ns->ref);
> +out:
> + nvmet_req_complete(req, status);
> +}
[ ... ]
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index c86fb2c19c152..8f48227323fea 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261019033624.402337-1-kanie@linux.alibaba.com?part=9
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 10/11] nvmet: translate placement identifiers of writes to write streams
2026-10-19 3:36 [PATCH RESEND 0/11] nvmet: FDP bridge for NVMe-oF targets Guixin Liu
` (8 preceding siblings ...)
2026-10-19 3:36 ` [PATCH 09/11] nvmet: serve the FDP log pages Guixin Liu
@ 2026-10-19 3:36 ` Guixin Liu
2026-10-19 3:44 ` sashiko-bot
2026-10-19 3:36 ` [PATCH 11/11] nvmet: add an FDP write translation tracepoint Guixin Liu
10 siblings, 1 reply; 22+ messages in thread
From: Guixin Liu @ 2026-10-19 3:36 UTC (permalink / raw)
To: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg,
Nilay Shroff, Daniel Wagner, Hannes Reinecke
Cc: linux-nvme
A write command that carries the Data Placement Directive names a
placement identifier in the Directive Specific field. Both sides of
the bridge use identity mapped handles: the write streams of the
backing device map to the placement handles one by one, in the order
the reclaim unit handle status reports them. Translate the placement
identifier to the matching backing write stream on the write path, so
the data lands in the reclaim unit of the backing device that the
host selected. The enable of the namespace has verified that the
backing placement identifiers are identity mapped.
Writes without the directive, and writes with a placement identifier
that is out of range, keep the default stream 0: the spec lets the
controller place such data itself.
Assisted-by: LLM
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
drivers/nvme/target/io-cmd-bdev.c | 4 ++++
drivers/nvme/target/nvmet.h | 20 ++++++++++++++++++++
2 files changed, 24 insertions(+)
diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
index f2d9e8901df4e..ff480f9ab9906 100644
--- a/drivers/nvme/target/io-cmd-bdev.c
+++ b/drivers/nvme/target/io-cmd-bdev.c
@@ -300,6 +300,10 @@ static void nvmet_bdev_execute_rw(struct nvmet_req *req)
bio->bi_iter.bi_sector = sector;
bio->bi_private = req;
bio->bi_end_io = nvmet_bio_done;
+ if (req->cmd->rw.opcode == nvme_cmd_write &&
+ nvmet_fdp_enabled(req->ns)) {
+ bio->bi_write_stream = nvmet_fdp_write_stream(req);
+ }
blk_start_plug(&plug);
if (req->metadata_len)
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 5bcc719973a57..af77591053226 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -1005,6 +1005,26 @@ void nvmet_execute_io_mgmt_recv(struct nvmet_req *req);
void nvmet_execute_io_mgmt_send(struct nvmet_req *req);
void nvmet_execute_get_log_page_fdp_configs(struct nvmet_req *req);
void nvmet_fdp_log_forward(struct nvmet_req *req);
+
+/*
+ * Both sides of the bridge use identity mapped handles, so placement
+ * identifier i routes to backing write stream i+1; stream 0, the
+ * default, places the data without a directive.
+ */
+static inline u8 nvmet_fdp_write_stream(struct nvmet_req *req)
+{
+ u16 control = le16_to_cpu(req->cmd->rw.control);
+ u16 pid;
+
+ if ((control & 0xf0) != (NVME_RW_DTYPE_DPLCMT & 0xf0))
+ return 0;
+
+ pid = le32_to_cpu(req->cmd->rw.dsmgmt) >> 16;
+ if (pid >= req->ns->fdp.nruh)
+ return 0;
+
+ return pid + 1;
+}
#else
static inline bool nvmet_fdp_enabled(struct nvmet_ns *ns)
{
--
2.43.7
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 10/11] nvmet: translate placement identifiers of writes to write streams
2026-10-19 3:36 ` [PATCH 10/11] nvmet: translate placement identifiers of writes to write streams Guixin Liu
@ 2026-10-19 3:44 ` sashiko-bot
2026-10-19 8:27 ` Guixin Liu
0 siblings, 1 reply; 22+ messages in thread
From: sashiko-bot @ 2026-10-19 3:44 UTC (permalink / raw)
To: Guixin Liu; +Cc: linux-nvme
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] nvmet: bi_write_stream lost on chained bios in nvmet_bdev_execute_rw()
--
commit ee4faa4396ddf960be1a3f58157a725fb3691c6f
Author: Guixin Liu <kanie@linux.alibaba.com>
nvmet: translate placement identifiers of writes to write streams
This commit translates NVMe Data Placement Directive placement identifiers
on write commands into block layer write streams on the backing block
device when FDP is enabled on the namespace.
> diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
> index f2d9e8901df4e..ff480f9ab9906 100644
> --- a/drivers/nvme/target/io-cmd-bdev.c
> +++ b/drivers/nvme/target/io-cmd-bdev.c
> @@ -300,6 +300,10 @@ static void nvmet_bdev_execute_rw(struct nvmet_req *req)
> bio->bi_iter.bi_sector = sector;
> bio->bi_private = req;
> bio->bi_end_io = nvmet_bio_done;
> + if (req->cmd->rw.opcode == nvme_cmd_write &&
> + nvmet_fdp_enabled(req->ns)) {
> + bio->bi_write_stream = nvmet_fdp_write_stream(req);
> + }
>
> blk_start_plug(&plug);
> if (req->metadata_len)
[Severity: Medium]
What happens when a write command spans multiple bios in
nvmet_bdev_execute_rw()?
When req->sg_cnt exceeds BIO_MAX_VECS (or bio_add_page() otherwise fills
the current bio), the for_each_sg() loop allocates a new chained bio:
for_each_sg(req->sg, sg, req->sg_cnt, i) {
while (bio_add_page(bio, sg_page(sg), sg->length, sg->offset)
!= sg->length) {
struct bio *prev = bio;
...
bio = bio_alloc(req->ns->bdev, bio_max_segs(sg_cnt),
opf, GFP_KERNEL);
bio->bi_iter.bi_sector = sector;
bio_chain(bio, prev);
submit_bio(prev);
}
Since bio_alloc() initializes bi_write_stream to 0 and bio_chain() does
not copy bi_write_stream from prev, won't all subsequent chained bios for
the write command be submitted with bi_write_stream set to 0 instead of
the stream returned by nvmet_fdp_write_stream(req), preventing them from
merging with the initial bio and routing the tail of the write to the
default stream instead of the host-selected reclaim unit?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261019033624.402337-1-kanie@linux.alibaba.com?part=10
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 10/11] nvmet: translate placement identifiers of writes to write streams
2026-10-19 3:44 ` sashiko-bot
@ 2026-10-19 8:27 ` Guixin Liu
0 siblings, 0 replies; 22+ messages in thread
From: Guixin Liu @ 2026-10-19 8:27 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-nvme
在 2026/10/19 11:44, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] nvmet: bi_write_stream lost on chained bios in nvmet_bdev_execute_rw()
> --
>
> commit ee4faa4396ddf960be1a3f58157a725fb3691c6f
> Author: Guixin Liu <kanie@linux.alibaba.com>
>
> nvmet: translate placement identifiers of writes to write streams
>
> This commit translates NVMe Data Placement Directive placement identifiers
> on write commands into block layer write streams on the backing block
> device when FDP is enabled on the namespace.
>
>> diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
>> index f2d9e8901df4e..ff480f9ab9906 100644
>> --- a/drivers/nvme/target/io-cmd-bdev.c
>> +++ b/drivers/nvme/target/io-cmd-bdev.c
>> @@ -300,6 +300,10 @@ static void nvmet_bdev_execute_rw(struct nvmet_req *req)
>> bio->bi_iter.bi_sector = sector;
>> bio->bi_private = req;
>> bio->bi_end_io = nvmet_bio_done;
>> + if (req->cmd->rw.opcode == nvme_cmd_write &&
>> + nvmet_fdp_enabled(req->ns)) {
>> + bio->bi_write_stream = nvmet_fdp_write_stream(req);
>> + }
>>
>> blk_start_plug(&plug);
>> if (req->metadata_len)
> [Severity: Medium]
> What happens when a write command spans multiple bios in
> nvmet_bdev_execute_rw()?
>
> When req->sg_cnt exceeds BIO_MAX_VECS (or bio_add_page() otherwise fills
> the current bio), the for_each_sg() loop allocates a new chained bio:
>
> for_each_sg(req->sg, sg, req->sg_cnt, i) {
> while (bio_add_page(bio, sg_page(sg), sg->length, sg->offset)
> != sg->length) {
> struct bio *prev = bio;
> ...
> bio = bio_alloc(req->ns->bdev, bio_max_segs(sg_cnt),
> opf, GFP_KERNEL);
> bio->bi_iter.bi_sector = sector;
>
> bio_chain(bio, prev);
> submit_bio(prev);
> }
>
> Since bio_alloc() initializes bi_write_stream to 0 and bio_chain() does
> not copy bi_write_stream from prev, won't all subsequent chained bios for
> the write command be submitted with bi_write_stream set to 0 instead of
> the stream returned by nvmet_fdp_write_stream(req), preventing them from
> merging with the initial bio and routing the tail of the write to the
> default stream instead of the host-selected reclaim unit?
>
> [ ... ]
Sashiko is right, fixed in v2.
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 11/11] nvmet: add an FDP write translation tracepoint
2026-10-19 3:36 [PATCH RESEND 0/11] nvmet: FDP bridge for NVMe-oF targets Guixin Liu
` (9 preceding siblings ...)
2026-10-19 3:36 ` [PATCH 10/11] nvmet: translate placement identifiers of writes to write streams Guixin Liu
@ 2026-10-19 3:36 ` Guixin Liu
2026-10-19 3:48 ` sashiko-bot
10 siblings, 1 reply; 22+ messages in thread
From: Guixin Liu @ 2026-10-19 3:36 UTC (permalink / raw)
To: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg,
Nilay Shroff, Daniel Wagner, Hannes Reinecke
Cc: linux-nvme
Add a tracepoint to the write path that reports the placement
identifier carried by a write command with the Data Placement
Directive and the backing write stream it is translated to.
Assisted-by: LLM
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
drivers/nvme/target/io-cmd-bdev.c | 4 ++++
drivers/nvme/target/trace.h | 23 +++++++++++++++++++++++
2 files changed, 27 insertions(+)
diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
index ff480f9ab9906..139be9edad969 100644
--- a/drivers/nvme/target/io-cmd-bdev.c
+++ b/drivers/nvme/target/io-cmd-bdev.c
@@ -9,6 +9,7 @@
#include <linux/memremap.h>
#include <linux/module.h>
#include "nvmet.h"
+#include "trace.h"
void nvmet_bdev_set_limits(struct block_device *bdev, struct nvme_id_ns *id)
{
@@ -302,7 +303,10 @@ static void nvmet_bdev_execute_rw(struct nvmet_req *req)
bio->bi_end_io = nvmet_bio_done;
if (req->cmd->rw.opcode == nvme_cmd_write &&
nvmet_fdp_enabled(req->ns)) {
+ u16 pid = le32_to_cpu(req->cmd->rw.dsmgmt) >> 16;
+
bio->bi_write_stream = nvmet_fdp_write_stream(req);
+ trace_nvmet_fdp_write_translate(req, pid, bio->bi_write_stream);
}
blk_start_plug(&plug);
diff --git a/drivers/nvme/target/trace.h b/drivers/nvme/target/trace.h
index 7f7ebf9558e50..0ab7252960790 100644
--- a/drivers/nvme/target/trace.h
+++ b/drivers/nvme/target/trace.h
@@ -131,6 +131,29 @@ TRACE_EVENT(nvmet_req_complete,
);
+TRACE_EVENT(nvmet_fdp_write_translate,
+ TP_PROTO(struct nvmet_req *req, u16 pid, u8 stream),
+ TP_ARGS(req, pid, stream),
+ TP_STRUCT__entry(
+ __field(u16, ctrl_id)
+ __array(char, disk, DISK_NAME_LEN)
+ __field(u32, nsid)
+ __field(u16, pid)
+ __field(u8, stream)
+ ),
+ TP_fast_assign(
+ __entry->ctrl_id = nvmet_req_to_ctrl_id(req);
+ __assign_req_name(__entry->disk, req);
+ __entry->nsid = req->ns->nsid;
+ __entry->pid = pid;
+ __entry->stream = stream;
+ ),
+ TP_printk("nvmet%s: %snsid=%u, pid=%u, write_stream=%u",
+ __print_ctrl_id(__entry->ctrl_id),
+ __print_disk_name(__entry->disk),
+ __entry->nsid, __entry->pid, __entry->stream)
+);
+
#define aer_name(aer) { aer, #aer }
TRACE_EVENT(nvmet_async_event,
--
2.43.7
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 11/11] nvmet: add an FDP write translation tracepoint
2026-10-19 3:36 ` [PATCH 11/11] nvmet: add an FDP write translation tracepoint Guixin Liu
@ 2026-10-19 3:48 ` sashiko-bot
2026-10-19 8:32 ` Guixin Liu
0 siblings, 1 reply; 22+ messages in thread
From: sashiko-bot @ 2026-10-19 3:48 UTC (permalink / raw)
To: Guixin Liu; +Cc: linux-nvme
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] nvmet: trace_nvmet_fdp_write_translate() called on non-directive writes
- [Medium] nvmet: missing bi_write_stream copy on chained bios in nvmet_bdev_execute_rw()
--
commit 910a51c080606805fab754e08158c76023d49e00
Author: Guixin Liu <kanie@linux.alibaba.com>
nvmet: add an FDP write translation tracepoint
This commit adds the nvmet_fdp_write_translate tracepoint to the bdev write
path to report the placement identifier from a write command and the backing
write stream it is translated to.
> diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
> index ff480f9ab9906..139be9edad969 100644
> --- a/drivers/nvme/target/io-cmd-bdev.c
> +++ b/drivers/nvme/target/io-cmd-bdev.c
[ ... ]
> @@ -302,7 +303,10 @@ static void nvmet_bdev_execute_rw(struct nvmet_req *req)
> bio->bi_end_io = nvmet_bio_done;
> if (req->cmd->rw.opcode == nvme_cmd_write &&
> nvmet_fdp_enabled(req->ns)) {
> + u16 pid = le32_to_cpu(req->cmd->rw.dsmgmt) >> 16;
> +
> bio->bi_write_stream = nvmet_fdp_write_stream(req);
> + trace_nvmet_fdp_write_translate(req, pid, bio->bi_write_stream);
> }
[Severity: Medium]
Should trace_nvmet_fdp_write_translate() only be called when the write
command actually carries the Data Placement Directive?
The commit message notes that the tracepoint "reports the placement
identifier carried by a write command with the Data Placement Directive
and the backing write stream it is translated to."
In drivers/nvme/target/nvmet.h, nvmet_fdp_write_stream() checks the
directive type before treating the upper 16 bits of req->cmd->rw.dsmgmt
as a placement identifier:
static inline u8 nvmet_fdp_write_stream(struct nvmet_req *req)
{
u16 control = le16_to_cpu(req->cmd->rw.control);
u16 pid;
if ((control & 0xf0) != (NVME_RW_DTYPE_DPLCMT & 0xf0))
return 0;
pid = le32_to_cpu(req->cmd->rw.dsmgmt) >> 16;
In nvmet_bdev_execute_rw(), however, trace_nvmet_fdp_write_translate() is
invoked for every nvme_cmd_write to an FDP-enabled namespace without
checking whether req->cmd->rw.control specifies NVME_RW_DTYPE_DPLCMT.
When a host issues an ordinary non-directive write (or a write with a
different directive type) to an FDP-enabled namespace, won't this still
fire the tracepoint and report the upper 16 bits of req->cmd->rw.dsmgmt
as pid alongside write_stream=0, making non-directive writes
indistinguishable from directive writes whose placement identifier failed
translation?
[Severity: Medium]
Also in nvmet_bdev_execute_rw(), what happens when a write with the Data
Placement Directive spans more than BIO_MAX_VECS (256) scatterlist
segments and has to be split across chained bios?
Later in nvmet_bdev_execute_rw(), if bio_add_page() cannot fit all
segments into the initial bio, a continuation bio is allocated and
chained:
for_each_sg(req->sg, sg, req->sg_cnt, i) {
while (bio_add_page(bio, sg_page(sg), sg->length, sg->offset)
!= sg->length) {
struct bio *prev = bio;
...
bio = bio_alloc(req->ns->bdev, bio_max_segs(sg_cnt),
opf, GFP_KERNEL);
bio->bi_iter.bi_sector = sector;
bio_chain(bio, prev);
submit_bio(prev);
}
Because bio_alloc() initializes bi_write_stream to 0 and bio_chain() does
not copy prev->bi_write_stream, the continuation bios keep
bi_write_stream == 0 while trace_nvmet_fdp_write_translate() reports that
the request was translated to the initial bio's stream.
As a result, blk_rq_merge_ok() will reject merging the continuation bios
with the initial bio due to the mismatched bi_write_stream, and
nvme_setup_rw() will submit the remainder of the write to the backing
device without NVME_RW_DTYPE_DPLCMT (placing it into default stream 0).
Should prev->bi_write_stream be copied to each chained bio in
nvmet_bdev_execute_rw()?
>
> blk_start_plug(&plug);
[ ... ]
>
>
>
[]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261019033624.402337-1-kanie@linux.alibaba.com?part=11
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 11/11] nvmet: add an FDP write translation tracepoint
2026-10-19 3:48 ` sashiko-bot
@ 2026-10-19 8:32 ` Guixin Liu
0 siblings, 0 replies; 22+ messages in thread
From: Guixin Liu @ 2026-10-19 8:32 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-nvme
在 2026/10/19 11:48, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] nvmet: trace_nvmet_fdp_write_translate() called on non-directive writes
> - [Medium] nvmet: missing bi_write_stream copy on chained bios in nvmet_bdev_execute_rw()
> --
>
> commit 910a51c080606805fab754e08158c76023d49e00
> Author: Guixin Liu <kanie@linux.alibaba.com>
>
> nvmet: add an FDP write translation tracepoint
>
> This commit adds the nvmet_fdp_write_translate tracepoint to the bdev write
> path to report the placement identifier from a write command and the backing
> write stream it is translated to.
>
>> diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
>> index ff480f9ab9906..139be9edad969 100644
>> --- a/drivers/nvme/target/io-cmd-bdev.c
>> +++ b/drivers/nvme/target/io-cmd-bdev.c
> [ ... ]
>
>> @@ -302,7 +303,10 @@ static void nvmet_bdev_execute_rw(struct nvmet_req *req)
>> bio->bi_end_io = nvmet_bio_done;
>> if (req->cmd->rw.opcode == nvme_cmd_write &&
>> nvmet_fdp_enabled(req->ns)) {
>> + u16 pid = le32_to_cpu(req->cmd->rw.dsmgmt) >> 16;
>> +
>> bio->bi_write_stream = nvmet_fdp_write_stream(req);
>> + trace_nvmet_fdp_write_translate(req, pid, bio->bi_write_stream);
>> }
> [Severity: Medium]
> Should trace_nvmet_fdp_write_translate() only be called when the write
> command actually carries the Data Placement Directive?
>
> The commit message notes that the tracepoint "reports the placement
> identifier carried by a write command with the Data Placement Directive
> and the backing write stream it is translated to."
>
> In drivers/nvme/target/nvmet.h, nvmet_fdp_write_stream() checks the
> directive type before treating the upper 16 bits of req->cmd->rw.dsmgmt
> as a placement identifier:
>
> static inline u8 nvmet_fdp_write_stream(struct nvmet_req *req)
> {
> u16 control = le16_to_cpu(req->cmd->rw.control);
> u16 pid;
>
> if ((control & 0xf0) != (NVME_RW_DTYPE_DPLCMT & 0xf0))
> return 0;
>
> pid = le32_to_cpu(req->cmd->rw.dsmgmt) >> 16;
>
> In nvmet_bdev_execute_rw(), however, trace_nvmet_fdp_write_translate() is
> invoked for every nvme_cmd_write to an FDP-enabled namespace without
> checking whether req->cmd->rw.control specifies NVME_RW_DTYPE_DPLCMT.
>
> When a host issues an ordinary non-directive write (or a write with a
> different directive type) to an FDP-enabled namespace, won't this still
> fire the tracepoint and report the upper 16 bits of req->cmd->rw.dsmgmt
> as pid alongside write_stream=0, making non-directive writes
> indistinguishable from directive writes whose placement identifier failed
> translation?
Well, yes, fixed in v2.
>
> [Severity: Medium]
> Also in nvmet_bdev_execute_rw(), what happens when a write with the Data
> Placement Directive spans more than BIO_MAX_VECS (256) scatterlist
> segments and has to be split across chained bios?
>
> Later in nvmet_bdev_execute_rw(), if bio_add_page() cannot fit all
> segments into the initial bio, a continuation bio is allocated and
> chained:
>
> for_each_sg(req->sg, sg, req->sg_cnt, i) {
> while (bio_add_page(bio, sg_page(sg), sg->length, sg->offset)
> != sg->length) {
> struct bio *prev = bio;
> ...
> bio = bio_alloc(req->ns->bdev, bio_max_segs(sg_cnt),
> opf, GFP_KERNEL);
> bio->bi_iter.bi_sector = sector;
>
> bio_chain(bio, prev);
> submit_bio(prev);
> }
>
> Because bio_alloc() initializes bi_write_stream to 0 and bio_chain() does
> not copy prev->bi_write_stream, the continuation bios keep
> bi_write_stream == 0 while trace_nvmet_fdp_write_translate() reports that
> the request was translated to the initial bio's stream.
>
> As a result, blk_rq_merge_ok() will reject merging the continuation bios
> with the initial bio due to the mismatched bi_write_stream, and
> nvme_setup_rw() will submit the remainder of the write to the backing
> device without NVME_RW_DTYPE_DPLCMT (placing it into default stream 0).
> Should prev->bi_write_stream be copied to each chained bio in
> nvmet_bdev_execute_rw()?
Same as before, fixed in v2.
>>
>> blk_start_plug(&plug);
> [ ... ]
>
>>
>>
> []
>
^ permalink raw reply [flat|nested] 22+ messages in thread