Re: [PATCH 07/11] nvmet: implement the FDP feature

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] nvmet: fdp: nvmet_fdp_lookup_ns() checks ns->enabled instead of NVMET_NS_IO_LIVE
- [Medium] nvmet: nvmet_get_feat_fdp() fails on non-FDP namespaces in mixed subsystems
--

commit 306dfd499967efa07a30c35673d8c81562928443
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) and advertises it in the Supported Features log
page when FDP is configured on the subsystem.

> diff --git a/drivers/nvme/target/fdp.c b/drivers/nvme/target/fdp.c
> index 17949e3665293..5008294984885 100644
> --- a/drivers/nvme/target/fdp.c
> +++ b/drivers/nvme/target/fdp.c

[ ... ]

> @@ -83,3 +87,82 @@ 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;
> +
> +	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;

[Severity: High]
Can checking ns->enabled instead of test_bit(NVMET_NS_IO_LIVE, &ns->flags)
here race with nvmet_ns_disable() and nvmet_ns_enable()?

After commits 42b0f5196dc2 ("nvmet: defer setting ns->enabled to false in
nvmet_ns_disable()") and 3fdd281e0a04 ("nvmet: don't allow I/O admission
after percpu ns reference is killed"), nvmet_ns_disable() clears
NVMET_NS_IO_LIVE and tears down ns->ref before setting ns->enabled to 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);
	...
	mutex_lock(&subsys->lock);
	nvmet_ns_changed(subsys, ns->nsid);
	nvmet_ns_dev_disable(ns);
	ns->enabled = false;
	mutex_unlock(&subsys->lock);
}

Because ns->enabled remains true across percpu_ref_kill(&ns->ref),
synchronize_rcu(), and percpu_ref_exit(&ns->ref), a concurrent
nvmet_set_feat_fdp() or nvmet_get_feat_fdp() calling nvmet_fdp_lookup_ns()
can still observe ns->enabled == true and call percpu_ref_tryget_live(&ns->ref)
while or after percpu_ref_exit(&ns->ref) frees the per-CPU counter.

Similarly, in nvmet_ns_enable(), ns->enabled = true is a plain store with no
release barrier after percpu_ref_init(&ns->ref, ...), whereas NVMET_NS_IO_LIVE
is set after xa_set_mark():

drivers/nvme/target/core.c:nvmet_ns_enable() {
	ret = percpu_ref_init(&ns->ref, nvmet_destroy_namespace, 0, GFP_KERNEL);
	if (ret)
		goto out_fdp_exit;

	nvmet_ns_changed(subsys, ns->nsid);
	ns->enabled = true;
	xa_set_mark(&subsys->namespaces, ns->nsid, NVMET_NS_ENABLED);
	nvmet_debugfs_ns_setup(ns);
	set_bit(NVMET_NS_IO_LIVE, &ns->flags);
}

Should nvmet_fdp_lookup_ns() check test_bit(NVMET_NS_IO_LIVE, &ns->flags) and
use percpu_ref_tryget_live_rcu(&ns->ref), matching nvmet_req_find_ns()?

> +
> +	rcu_read_unlock();
> +	return ns;
> +
> +out_unlock:
> +	rcu_read_unlock();
> +	return NULL;
> +}

[ ... ]

> +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;

[Severity: Medium]
What happens in a subsystem that contains both FDP-enabled and non-FDP
namespaces?

When at least one namespace in the subsystem has FDP enabled,
nvmet_fdp_subsys_configured() returns true, so the controller advertises
NVME_CTRL_ATTR_FDPS in ctratt and NVME_FEAT_FDP in the Supported Features log
page. Every enabled namespace is exposed as its own endurance group
(endgid = nsid).

When the Linux NVMe host scans namespaces on a controller with
NVME_CTRL_ATTR_FDPS set, nvme_alloc_ns_head() -> nvme_query_fdp_info() issues
Get Features (NVME_FEAT_FDP) for each namespace's endgid and checks whether
FDPCFG_FDPE is set in the result:

drivers/nvme/host/core.c:nvme_query_fdp_info() {
	ret = nvme_get_features(ctrl, NVME_FEAT_FDP, info->endgid, NULL, 0,
				&fdp);
	if (ret) {
		dev_warn(ctrl->device, "FDP get feature status:0x%x\n", ret);
		return ret;
	}

	if (!(fdp.flags & FDPCFG_FDPE))
		return 0;
}

Because nvmet_get_feat_fdp() uses nvmet_fdp_lookup_ns(), which rejects
namespaces where !nvmet_fdp_enabled(ns), won't Get Features fail with
NVME_SC_INVALID_FIELD | NVME_STATUS_DNR on valid non-FDP namespaces instead
of succeeding with result 0 (FDPE cleared) when nvmet_fdp_subsys_configured()
is true?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023080251.2606443-1-kanie@linux.alibaba.com?part=7