Re: [PATCH 09/11] nvmet: serve the FDP log pages

[Guixin Liu <kanie@linux.alibaba.com>]

在 2026/10/23 16:37, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Critical] nvmet: unsanitized cmd and truncated transfer_len in nvmet_fdp_log_forward()
> - [High] nvmet: broken partial/offset reads and unbounded kzalloc() in FDP log handlers
> - [High] nvmet: check NVMET_NS_IO_LIVE instead of ns->enabled in nvmet_fdp_lookup_ns()
> --
>
> commit 6fe9b61853a62f18f9ae3bb5a0ad4dcfebf1822f
> Author: Guixin Liu <kanie@linux.alibaba.com>
>
> nvmet: serve the FDP log pages
>
> This commit adds support for serving the FDP Configurations log page
> from the target's namespace configuration snapshot and forwarding the
> remaining FDP log pages to the backing controller over its admin queue.
>
>> diff --git a/drivers/nvme/target/fdp.c b/drivers/nvme/target/fdp.c
>> index 807f54c73c4e1..011f022e857aa 100644
>> --- a/drivers/nvme/target/fdp.c
>> +++ b/drivers/nvme/target/fdp.c
> [ ... ]
>
>> @@ -355,3 +354,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()?
>
> In drivers/nvme/target/fdp.c, nvmet_fdp_lookup_ns() checks ns->enabled
> under rcu_read_lock() instead of test_bit(NVMET_NS_IO_LIVE, &ns->flags):
>
> drivers/nvme/target/fdp.c:nvmet_fdp_lookup_ns() {
> 	rcu_read_lock();
> 	ns = xa_load(&subsys->namespaces, endgid);
> 	if (!ns || !ns->enabled || !nvmet_fdp_enabled(ns))
> 		goto out_unlock;
>
> 	if (!percpu_ref_tryget_live(&ns->ref))
> 		goto out_unlock;
> }
>
> However, in drivers/nvme/target/core.c, nvmet_ns_disable() clears
> NVMET_NS_IO_LIVE, kills the percpu_ref, and runs synchronize_rcu() and
> nvmet_fdp_exit_ns() before setting ns->enabled = false:
>
> drivers/nvme/target/core.c:nvmet_ns_disable() {
> 	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);
> 	...
> 	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);
> }
>
> When nvmet_ns_free() calls nvmet_ns_disable(), it immediately erases the
> namespace from subsys->namespaces and calls kfree(ns) without another RCU
> grace period after ns->enabled is cleared. An RCU reader in
> nvmet_fdp_lookup_ns() entering after synchronize_rcu() can still see
> ns->enabled == true and call percpu_ref_tryget_live(&ns->ref) on freed
> memory.
>
> Additionally, if a reader in nvmet_fdp_lookup_ns() passes the ns->enabled
> and nvmet_fdp_enabled(ns) checks while the namespace is being disabled, and
> the namespace is then reconfigured with FDP disabled and re-enabled via
> nvmet_ns_enable() (which re-initializes ns->ref),
> percpu_ref_tryget_live(&ns->ref) can succeed after nvmet_fdp_exit_ns() has
> set ns->fdp.backend_ctrl = NULL, leading to a NULL pointer dereference of
> ns->fdp.backend_ctrl->admin_q in nvmet_fdp_log_forward().
>
> Should nvmet_fdp_lookup_ns() check test_bit(NVMET_NS_IO_LIVE, &ns->flags)
> just like nvmet_req_find_ns() does?
Sashiko is right, same as patch07, changed in v3.
>> +
>> +	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;
>> +	}
> [Severity: High]
> What happens here when a host performs a partial read with
> sizeof(*log) < req->transfer_len < total (or req->transfer_len < sizeof(*log)),
> or specifies a non-zero Log Page Offset (req->cmd->get_log_page.lpo)?
>
> Because desc is only populated when req->transfer_len >= total, any partial
> read between sizeof(*log) and total (such as reading the header plus the
> fixed portion of struct nvme_fdp_config_desc, or reading the descriptor at
> a non-zero lpo with transfer_len == dsze) will copy all-zeroes for the
> descriptor, even though nvmet_execute_identify_ctrl() advertises LPO support
> in id->lpa.
>
> Also, passing the initiator-controlled req->transfer_len directly to
> kzalloc(req->transfer_len, GFP_KERNEL) here and in nvmet_fdp_log_forward()
> without bounding the length or passing __GFP_NOWARN allows a large SGL
> transfer_len (for example on RDMA) to exceed KMALLOC_MAX_SIZE and trigger
> WARN_ON_ONCE_GFP(order > MAX_PAGE_ORDER, gfp) in the page allocator.
>
> Could nvmet_execute_get_log_page_fdp_configs() instead allocate total bytes
> (which is bounded because ns->fdp.nruh <= U8_MAX), populate the log header
> and descriptor unconditionally, and copy the requested slice based on
> nvmet_get_log_page_offset(req->cmd) and req->transfer_len (similar to
> nvmet_execute_disc_get_log_page() in drivers/nvme/target/discovery.c)?
Don't make any difference, so no change in v3.
>> +
>> +	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);
> [Severity: Critical]
> Can forwarding a raw copy of *req->cmd with the 32-bit result of
> nvmet_get_log_page_len() cause the backing NVMe controller to DMA into
> arbitrary host addresses or overflow buf?
>
> In drivers/nvme/target/admin-cmd.c, nvmet_execute_get_log_page() checks
> req->transfer_len against nvmet_get_log_page_len(req->cmd), which computes
> the byte length in 32-bit u32 arithmetic:
>
> 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 the 32-bit NUMD value ((numdu << 16) | numdl) is >= 0x3fffffff,
> (NUMD + 1) * sizeof(u32) overflows u32 and wraps modulo 2^32. Meanwhile,
> nvmet_fdp_log_forward() copies the raw command (c = *req->cmd) without
> clearing c.common.dptr or c.get_log_page.rsvd2, and passes the untruncated
> numdu/numdl to the backing controller:
>
> 1. If NUMD wraps len to 0 (e.g. NUMD == 0x3fffffff with a 0-length SGL so
> req->transfer_len == 0), nvmet_check_transfer_len(req, 0) succeeds and
> kzalloc(0, GFP_KERNEL) returns ZERO_SIZE_PTR. In __nvme_submit_sync_cmd(),
> nvme_init_request() clears NVME_CMD_SGL_ALL from c.common.flags (switching
> the command to PRP mode) and skips blk_rq_map_kern() because bufflen is 0:
>
> 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;
> 	}
> }
>
> In nvme_prep_rq(), blk_rq_nr_phys_segments(req) is 0, so nvme_map_data() is
> skipped and the initiator's raw c.common.dptr (prp1/prp2) remains in the
> admin SQE with NUMD >= 0x3fffffff, instructing the backing PCIe controller
> to DMA the log page into initiator-chosen physical/IOVA addresses.
>
> 2. If NUMD wraps len to a small non-zero value (e.g. numdu == 0x4000,
> numdl == 0x0000 so req->transfer_len == 4), kzalloc(4, GFP_KERNEL) allocates
> an 8-byte slab buffer and blk_rq_map_kern() maps 4 bytes, while numdu/numdl
> in the forwarded admin SQE requests 4 GiB + 4 bytes. Since PCIe admin
> queues use PRPs (which do not encode buffer length), the backing controller
> DMAs the full FDP log page past the end of buf.
>
> Should nvmet_fdp_log_forward() construct a clean struct nvme_command (or
> zero c.common.dptr and rsvd2 as nvmet_fdp_io_mgmt_forward() does for dptr)
> and derive numdl/numdu from a bounded buffer length (or use a host helper
> like nvme_get_log_lsi()), and should nvmet_get_log_page_len() avoid 32-bit
> overflow?
Sashiko is right, changed in v3.
>