Re: [PATCH] nvme: stop asynchronous event work before ANA teardown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] nvme: inaccurate comment on async_event_work in nvme_uninit_ctrl()
--

commit ce7eff9e841922523de71205d5552ff0e0d5e58d
Author: Yaohan Li <shunsheng1218@gmail.com>

nvme: stop asynchronous event work before ANA teardown

This commit cancels async_event_work in nvme_uninit_ctrl() and ana_work in
nvme_mpath_uninit() to prevent use-after-free during controller teardown.

> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index a7a4d09644c13..017aae1189de1 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -5182,6 +5182,15 @@ EXPORT_SYMBOL_GPL(nvme_start_ctrl);
>  void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
>  {
>  	nvme_stop_keep_alive(ctrl);
> +
> +	/*
> +	 * Stop the async event worker before the final put: it processes
> +	 * ANA change events by re-queueing ctrl->ana_work, which is
> +	 * embedded in ctrl and cancelled in nvme_free_ctrl(); a worker
> +	 * running past that point would re-queue it onto freed memory.
> +	 */
> +	cancel_work_sync(&ctrl->async_event_work);

[Severity: Low]
This isn't a bug in the code change itself, but is the explanation in this
comment and in the commit message accurate about how ctrl->ana_work gets
requeued?

Looking at drivers/nvme/host/core.c, ctrl->async_event_work runs
nvme_async_event_work(), which only sends a uevent and resubmits an async
event request if the controller is live:

static void nvme_async_event_work(struct work_struct *work)
{
	struct nvme_ctrl *ctrl =
		container_of(work, struct nvme_ctrl, async_event_work);

	nvme_aen_uevent(ctrl);

	if (nvme_ctrl_state(ctrl) == NVME_CTRL_LIVE)
		ctrl->ops->submit_async_event(ctrl);
}

Instead, ctrl->ana_work is queued synchronously from the AEN completion path
nvme_complete_async_event() -> nvme_handle_aen_notice() before
nvme_complete_async_event() queues ctrl->async_event_work:

	case NVME_AER_NOTICE_ANA:
		if (!ctrl->ana_log_buf)
			break;
		queue_work(nvme_wq, &ctrl->ana_work);
		break;

In addition, nvme_scan_work() (flushed in nvme_remove_namespaces() after
nvme_stop_ctrl()) and nvme_failover_req() can also queue ctrl->ana_work
after nvme_stop_ctrl() has run.

Cancelling ctrl->async_event_work in nvme_uninit_ctrl() is still needed
because nvme_complete_async_event() queues ctrl->async_event_work and the
work item is embedded in struct nvme_ctrl (and accesses ctrl->aen_result,
ctrl->device->kobj, and ctrl->state), rather than because
nvme_async_event_work() requeues ctrl->ana_work.

Could the comment in nvme_uninit_ctrl() and the commit message be updated to
reflect the actual paths that queue ctrl->async_event_work and
ctrl->ana_work?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179378587000.73780.18152328009023418065@gmail.com?part=1