Re: [PATCH 3/3] nvme: Expose link rate switching tunables via sysfs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] nvme: Use-After-Free in timer teardown due to missing timer_shutdown_sync()
- [High] nvme: Use-After-Free on per-CPU stats due to lack of synchronization during teardown
- [Medium] nvme: sysfs teardown race allows resurrecting feature state and causing UAF
- [Medium] nvme: torn read-modify-write data race on per-CPU I/O statistics
--

commit 923c7df43c12b9f8956632abc24a17d7d85b2d2f
Author: Liao Xuan <liaoxuan@open-hieco.net>

nvme: Expose link rate switching tunables via sysfs

This commit adds a speed attribute group under /sys/class/nvme/nvmeX/ to
expose adaptive link rate switching parameters for runtime tuning. The
attributes are read/write and are removed when the controller is torn down.

> diff --git a/drivers/nvme/host/speed_switch.c b/drivers/nvme/host/speed_switch.c
> index 22f180c8119b2..fce4268459a0c 100644
> --- a/drivers/nvme/host/speed_switch.c
> +++ b/drivers/nvme/host/speed_switch.c
[ ... ]
> @@ -405,6 +417,7 @@ void nvme_speed_switch_exit(struct nvme_ctrl *ctrl)
>  	WRITE_ONCE(sw->enabled, false);
>  	timer_delete_sync(&sw->timer);

[Severity: High]
Is it safe to use timer_delete_sync() for the teardown path?

If a concurrent I/O thread in nvme_update_io_stats() is preempted, could it
wait for timer_delete_sync() to complete and then re-arm the timer after
the cancellation completes?

nvme_update_io_stats()
    mod_timer(&sw->timer, jiffies + msecs_to_jiffies(READ_ONCE(sw->monitor_interval)));

When the parent nvme_ctrl is later freed, the timer will execute and access
freed memory. Should this use timer_shutdown_sync() instead?

>  	cancel_work_sync(&sw->work);
> +	sysfs_remove_group(&ctrl->device->kobj, &nvme_speed_attr_group);

[Severity: Medium]
Is this sysfs group removed too late, leaving a race window during teardown?

Since WRITE_ONCE(sw->enabled, false) and timer_delete_sync() are called
before sysfs_remove_group(), could a concurrent sysfs write to the enable
attribute execute WRITE_ONCE(sw->enabled, val) and set it back to true?

Because sysfs_remove_group() blocks until active writes finish, it will
proceed to free sw->stats while sw->enabled remains true, guaranteeing that
subsequent I/Os will bypass the guard and access freed memory.

>  
>  	free_percpu(sw->stats);

[Severity: High]
Can this cause a use-after-free when dereferencing sw->stats?

Since there is no synchronization (such as an RCU grace period or queue
freeze) before freeing the memory, could a concurrent I/O thread executing
nvme_update_io_stats() that has already passed the lockless sw->enabled
check resume and call get_cpu_ptr(sw->stats)?

nvme_update_io_stats()
    stat = get_cpu_ptr(sw->stats);

This would dereference the per-CPU pointer which may have just been freed by
this concurrent teardown.

[Severity: Medium]
Can this also cause a torn read-modify-write data race on the per-CPU I/O
statistics?

If nvme_check_io_and_decide_speed() iterates over all CPUs and clears their
counters with a remote, non-atomic write:

    stat->read_bytes = 0;
    stat->write_bytes = 0;

And concurrently, the I/O submission path nvme_update_io_stats() executes a
non-atomic read-modify-write on its local CPU:

    stat->read_bytes += bytes;

Could the remote write of 0 occur between the local load and store, causing
the 0 to be overwritten by the local CPU's accumulated total, thereby double
counting the transferred bytes and artificially inflating throughput?

>  	sw->stats = NULL;
>  }
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index abf8edaae371b..3918d9c3d3306 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
[ ... ]
> @@ -1310,3 +1310,146 @@ const struct attribute_group *nvme_subsys_attrs_groups[] = {
[ ... ]
> +static ssize_t nvme_speed_attr_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
[ ... ]
> +	switch (sa->id) {
> +	case NVME_SPEED_ATTR_ENABLE:
> +		if (val > 1)
> +			return -EINVAL;
> +		if (!val && READ_ONCE(sw->enabled)) {
> +			WRITE_ONCE(sw->enabled, false);
> +			timer_delete_sync(&sw->timer);
> +			cancel_work_sync(&sw->work);
> +			atomic_set(&sw->timer_active, NVME_SPEED_TIMER_INACTIVE);
> +		} else
> +			WRITE_ONCE(sw->enabled, val);

[Severity: Medium]
Can this overwrite the teardown flag without a lock?

If this executes concurrently with nvme_speed_switch_exit() due to late
sysfs_remove_group() calling, it could silently reactivate lockless access
to memory that is about to be freed.

> +		break;
> +	case NVME_SPEED_ATTR_MONITOR_INTERVAL:

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