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

[Hannes Reinecke <hare@suse.de>]

On 11/4/26 10:51 AM, Yaohan Li wrote:
> 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;

Why can we schedule AENs when a controller teardown is active?
Shouldn't we rather disallow that instead of fixing the fallout?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich