[PATCH 08/11] nvmet: forward the FDP I/O management commands to the backing device
Guixin Liu
kanie at linux.alibaba.com
Mon Oct 19 02:44:45 PDT 2026
在 2026/10/19 11:57, sashiko-bot at 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 at 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;
>> +}
> [ ... ]
>
More information about the Linux-nvme
mailing list