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

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

[Yaohan Li]

nvme_mpath_stop() cancels ana_work, but the asynchronous event worker
can enqueue ANA processing again while controller teardown is in
progress. An ANA work item can therefore survive the final
controller release and access freed memory.

Cancel async_event_work during controller uninitialization, after the
controller has stopped accepting new events, and cancel ana_work
again before releasing its ANA resources. Draining only ana_work is
not sufficient while the event worker can requeue it.

The race was reproduced with the in-tree NVMe loop target and
scheduling injection. Cancelling ANA work alone left reports; draining
the event source and the ANA work together removed the reported
use-after-free under the same test conditions.

Fixes: 0d0b660f214dc ("nvme: add ANA support")
Assisted-by: ZCode:GLM-4
Signed-off-by: Yaohan Li <shunsheng1218@gmail.com>
---
 drivers/nvme/host/core.c      | 9 +++++++++
 drivers/nvme/host/multipath.c | 7 +++++++
 2 files changed, 16 insertions(+)

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);
+
 	nvme_hwmon_exit(ctrl);
 	nvme_fault_inject_fini(&ctrl->fault_inject);
 	dev_pm_qos_hide_latency_tolerance(ctrl->device);
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 9e0a0fd77c56e..44bd1a1ad5940 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -1610,6 +1610,13 @@ int nvme_mpath_init_identify(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 
 void nvme_mpath_uninit(struct nvme_ctrl *ctrl)
 {
+	/*
+	 * nvme_mpath_stop() already cancelled ana_work, but an ANA async
+	 * event completing after nvme_stop_ctrl() can have re-queued it.
+	 * It is embedded in ctrl, which is freed after uninit.
+	 */
+	cancel_work_sync(&ctrl->ana_work);
+
 	kvfree(ctrl->ana_log_buf);
 	ctrl->ana_log_buf = NULL;
 	ctrl->ana_log_size = 0;
-- 
2.34.1

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

[sashiko-bot]

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

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

[Christoph Hellwig]

The fix looks good, but it would be nice to get the Sashiko nit about the
comment fixed.