nvme: fix uninitialized variable in get_feature_id_changed()

[fhassan-ibm]

The get_feature_id_changed() function retrieves and compares NVMe feature values. The local @Result variable was left uninitialized before being passed to get_feature_id().

If get_feature_id() fails early, @Result may be used without ever being assigned a valid value, leading to undefined behavior.

Initialize @Result to -EINVAL to indicate no feature value has been retrieved and prevent undefined behavior.

[igaw]

Hmm, this might be a tricky one. Let me write down my though process:

The idea here is, that the result variable should only be updated when get_feature_id has been successfully executed by the kernel (err >= 0). Thus result should only be accessed on success.

If we follow the code path, we end up in the wrapper nvme_get_features which does:

	err = libnvme_exec_admin_passthru(hdl, &cmd);
	if (result)
		*result = cmd.result;

and the kernel is doing:

static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
		struct nvme_passthru_cmd __user *ucmd, unsigned int flags,
		bool open_for_write)
{
[...]
	status = nvme_submit_user_cmd(ns ? ns->queue : ctrl->admin_q, &c,
			cmd.addr, cmd.data_len, nvme_to_user_ptr(cmd.metadata),
			cmd.metadata_len, &result, timeout, 0);

	if (status >= 0) {
		if (put_user(result, &ucmd->result))
			return -EFAULT;
	}
[...]

First, I moved these wrappers from libnvme to nvme-cli to reduce the API size. My plan was to merge them into the code. Second, it should not blindly update result, only on success.

This brings us back to get_feature_id_changed:

	if (err || !cfg.changed || err_def || result != result_def ||
	    (buf && buf_def && !strcmp(buf, buf_def)))
		get_feature_id_print(cfg, err, result, buf, flags);

and it looks awful. Not sure what it wants to achieve? Does want to print the features on success? But then why err can to be set?

I'd say the condition should be first untangled, initializing result just makes the checker happy but not me :)

IIRC, result is holding return values NVMe spec, so it should not contain errno values.

@copilot can you verify my reasoning?

[Copilot]

Pull request overview

This PR aims to prevent undefined behavior in get_feature_id_changed() by ensuring the local feature result value is safely initialized before being passed into get_feature_id().

Changes:

• Changes the initialization of the local result variable in get_feature_id_changed().

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.