Re: [PATCH 1/3] nvme: Add adaptive PCIe link rate switching function

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] nvme: UAF on per-CPU stats and timer re-arming race during teardown
- [High] nvme: infinite link retrain loop on rollback success
- [High] nvme: TOCTOU data race on sw->target_speed causes link mismatch
- [High] nvme: speed switch timer runs during suspend causing deadlocks
- [High] nvme: missing memalloc_noio_save() around queue freeze
- [High] nvme: nvme_unfreeze clears NVME_CTRL_FROZEN prematurely
- [Medium] nvme: synchronous PCIe config space read on block I/O fast path
- [Medium] nvme: data race when zeroing remote per-CPU statistics
- [Medium] nvme: speed switch permanently disabled after controller reset
--

commit 3ad252ce0a89b0f36c40c41b569dd1f1382434b5
Author: Liao Xuan <liaoxuan@open-hieco.net>

nvme: Add adaptive PCIe link rate switching function

This commit introduces dynamic adjustment of the PCIe link rate for NVMe
devices based on I/O activity. The link speed is downgraded during periods
of low activity to save power and upgraded to maximum when I/O load crosses
a calculated threshold.

> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 1322c678f4eb8..254053ea15fcb 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
[ ... ]
> @@ -5089,6 +5090,7 @@ EXPORT_SYMBOL_GPL(nvme_remove_io_tag_set);
>  
>  void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
>  {
> +	nvme_speed_switch_exit(ctrl);
>  	nvme_mpath_stop(ctrl);
>  	nvme_auth_stop(ctrl);

[Severity: Medium]
Since nvme_add_ctrl() only initializes the speed switch once during device
probe, will calling nvme_speed_switch_exit() during nvme_stop_ctrl()
permanently disable the feature after the first controller reset? The reset
recovery path does not appear to call nvme_speed_switch_init() again.

[ ... ]
> diff --git a/drivers/nvme/host/speed_switch.c b/drivers/nvme/host/speed_switch.c
> new file mode 100644
> index 0000000000000..88c046fcdde89
> --- /dev/null
> +++ b/drivers/nvme/host/speed_switch.c
[ ... ]
> +static void nvme_speed_switch_work(struct work_struct *work)
> +{
> +	struct nvme_speed_switch *sw = container_of(work, struct nvme_speed_switch, work);
> +	struct nvme_ctrl *ctrl = container_of(sw, struct nvme_ctrl, speed_switch);
> +	struct pci_dev *pdev = to_pci_dev(ctrl->dev);
> +	struct pci_dev *bridge = pdev->bus->self;
> +	u8 cur_speed = READ_ONCE(sw->cur_speed);
> +	int ret;
> +
> +	if (nvme_ctrl_state(ctrl) != NVME_CTRL_LIVE)
> +		return;

[Severity: High]
Is this guard sufficient to prevent operations during host-managed suspend?
The controller state remains NVME_CTRL_LIVE during host-managed suspend,
which might allow the timer to fire, schedule this work, and attempt to
access the PCIe configuration space of a suspended device.

> +
> +	/* Freeze I/O to avoid timeouts during link retraining. */
> +	nvme_start_freeze(ctrl);
> +	nvme_wait_freeze(ctrl);
> +
> +	ret = pcie_set_target_speed(bridge, nvme_speed_to_bus_speed(sw->target_speed),
> +				    true);

[Severity: High]
Should this section be protected by memalloc_noio_save() and
memalloc_noio_restore()? Code executing while a block queue is frozen
typically needs NOIO protection. Otherwise, memory reclaim interrupting this
section could attempt to flush dirty pages to the frozen NVMe device,
leading to a system deadlock under memory pressure.

[Severity: High]
Could reading sw->target_speed here introduce a TOCTOU data race? This value
is read to program the hardware, but read again below to update sw->cur_speed.

> +	if (ret) {
> +		dev_warn(ctrl->device,
> +			 "failed to set target rate Gen%u (%d), trying rollback to Gen%u\n",
> +			 sw->target_speed, ret, cur_speed);
> +		ret = pcie_set_target_speed(bridge, nvme_speed_to_bus_speed(cur_speed),
> +					    true);
> +		if (ret) {
> +			dev_err(ctrl->device,
> +				"rollback to Gen%u failed (%d), disabling speed switch\n",
> +				cur_speed, ret);
> +			WRITE_ONCE(sw->enabled, false);
> +			nvme_unfreeze(ctrl);
> +			return;
> +		}

[Severity: High]
If the pcie_set_target_speed() rollback succeeds, the code omits disabling the
speed switch. Since the function returns while sw->enabled remains true, won't
the timer fire again 100ms later, see a target speed mismatch, and repeatedly
fail, freezing the queues continuously?

> +	} else {
> +		WRITE_ONCE(sw->cur_speed, sw->target_speed);
> +	}

[Severity: High]
If the timer concurrently updates sw->target_speed between the hardware
programming step and this write, sw->cur_speed will receive the new value
while the hardware was programmed to the old value. Will this cause the
hardware link speed to become permanently out-of-sync with the software state?

> +
> +	nvme_unfreeze(ctrl);

[Severity: High]
Is it safe to unconditionally call nvme_unfreeze() here? nvme_unfreeze()
explicitly clears the NVME_CTRL_FROZEN bit. If this work finishes while a
concurrent controller reset is ongoing, it clears the flag prematurely, which
could break the reset state machine and allow namespaces to be added
concurrently with the reset.

> +	dev_dbg(ctrl->device, "link rate changed to Gen%u\n",
> +		READ_ONCE(sw->cur_speed));
> +}
[ ... ]
> +/* Called on the I/O submission path to accumulate the transferred bytes. */
> +void nvme_update_io_stats(struct nvme_ctrl *ctrl, struct request *req)
> +{
> +	struct nvme_speed_switch *sw = &ctrl->speed_switch;
> +	struct nvme_speed_switch_stats *stat;
> +	enum req_op op = req_op(req);
> +	unsigned int bytes = blk_rq_bytes(req);
> +
> +	if (!READ_ONCE(sw->enabled))
> +		return;
> +
> +	if (op != REQ_OP_READ && op != REQ_OP_WRITE)
> +		return;
> +
> +	if (!sw->stats) {
> +		dev_dbg(ctrl->device, "I/O statistics not allocated, skip accounting\n");
> +		return;
> +	}
> +
> +	stat = get_cpu_ptr(sw->stats);

[Severity: High]
Could the I/O thread locklessly dereference sw->stats after it has been
freed? If the I/O thread is preempted right after checking sw->enabled, and a
concurrent controller reset calls nvme_speed_switch_exit() (which frees
sw->stats), the I/O thread would resume and access the freed memory here.

> +	if (op == REQ_OP_READ)
> +		stat->read_bytes += bytes;
> +	else
> +		stat->write_bytes += bytes;
> +	put_cpu_ptr(sw->stats);
> +
> +	if (atomic_cmpxchg(&sw->timer_active, NVME_SPEED_TIMER_INACTIVE,
> +			   NVME_SPEED_TIMER_ACTIVE) != NVME_SPEED_TIMER_INACTIVE)
> +		return;
> +
> +	/* First I/O of the activity period: compute the threshold and arm the timer. */
> +	WRITE_ONCE(sw->threshold, nvme_calc_speed_threshold(sw,
> +					pcie_get_width_cap(to_pci_dev(ctrl->dev))));

[Severity: Medium]
Does pcie_get_width_cap() perform a synchronous read of the PCIe configuration
space? Evaluating this on the first I/O of an activity period within the block
I/O submission fast path might introduce severe latency spikes for affected
requests.

> +	if (!READ_ONCE(sw->threshold)) {
> +		dev_warn(ctrl->device,
> +			 "failed to compute speed switch threshold, keeping current rate\n");
> +		atomic_set(&sw->timer_active, NVME_SPEED_TIMER_INACTIVE);
> +		return;
> +	}
> +	mod_timer(&sw->timer, jiffies + msecs_to_jiffies(READ_ONCE(sw->monitor_interval)));
> +}

[Severity: High]
Following up on the preemption scenario, if nvme_speed_switch_exit() completes
its teardown (including timer_delete_sync()), will this mod_timer() call
re-arm the timer after teardown has finished, leading to a delayed
use-after-free when the timer fires?

[ ... ]
> +static int nvme_check_io_and_decide_speed(struct nvme_speed_switch *sw)
> +{
> +	struct nvme_ctrl *ctrl = container_of(sw, struct nvme_ctrl, speed_switch);
> +	u64 read_bytes = 0, write_bytes = 0;
> +	unsigned long read_kb, write_kb;
> +	int cpu;
> +
> +	for_each_possible_cpu(cpu) {
> +		struct nvme_speed_switch_stats *stat = per_cpu_ptr(sw->stats, cpu);
> +
> +		read_bytes += stat->read_bytes;
> +		write_bytes += stat->write_bytes;
> +		stat->read_bytes = 0;
> +		stat->write_bytes = 0;
> +	}

[Severity: Medium]
Could zeroing these remote per-CPU statistics lead to a data race? The timer
callback iterates over all CPUs and assigns 0 to stat->read_bytes and
stat->write_bytes of remote CPUs concurrently while the I/O submission path
might be updating these variables. Without atomic operations or
synchronization, this could cause incorrect statistics and dropped increments.

[ ... ]
> +void nvme_speed_switch_exit(struct nvme_ctrl *ctrl)
> +{
> +	struct nvme_speed_switch *sw = &ctrl->speed_switch;
> +
> +	if (!sw->initialized)
> +		return;
> +
> +	sw->initialized = false;
> +	WRITE_ONCE(sw->enabled, false);
> +	timer_delete_sync(&sw->timer);
> +	cancel_work_sync(&sw->work);
> +
> +	free_percpu(sw->stats);
> +	sw->stats = NULL;
> +}

[Severity: High]
This teardown routine frees sw->stats and uses timer_delete_sync(), but does
this adequately prevent the timer from being asynchronously re-armed by an I/O
thread that had already passed the sw->enabled check?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790222172.git.liaoxuan@hygon.cn?part=1