Re: [PATCH v3 1/2] nvme-multipath: apply delayed_removal_secs changes to a pending removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] nvme-multipath: race in delayed_removal_secs_store() re-arms
  remove_work
--

commit 92bcf5100d1d5ed726ca5da1431d5f936dbea0e9
Author: Qiliang Yuan <qiliang.yuan@linux.dev>

nvme-multipath: apply delayed_removal_secs changes to a pending removal

This commit cancels and requeues any pending head->remove_work when
delayed_removal_secs is written via sysfs so the updated delay takes
effect immediately.

> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index ae8059366b57d..35278b9c4de60 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -1242,6 +1242,9 @@ static ssize_t delayed_removal_secs_store(struct device *dev,
>  	mutex_lock(&head->subsys->lock);
>  	head->delayed_removal_secs = sec;
>  	assign_bit(NVME_NSHEAD_QUEUE_IF_NO_PATH, &head->flags, sec);
> +	/* Apply the new delay to a removal that is already pending */
> +	if (cancel_delayed_work(&head->remove_work))
> +		queue_delayed_work(nvme_wq, &head->remove_work, sec * HZ);

[Severity: Medium]
Can this race with nvme_init_ns_head() and re-arm head->remove_work after
a path has reconnected?

drivers/nvme/host/core.c:nvme_init_ns_head() {
	...
	list_add_tail_rcu(&ns->siblings, &head->list);
	ns->head = head;
	mutex_unlock(&ctrl->subsys->lock);

#ifdef CONFIG_NVME_MULTIPATH
	if (cancel_delayed_work(&head->remove_work))
		module_put(THIS_MODULE);
#endif
}

nvme_init_ns_head() adds the new namespace to head->list under
subsys->lock, but cancels head->remove_work after dropping the lock.  If
delayed_removal_secs_store() runs in between, it clears the pending bit on
head->remove_work before nvme_init_ns_head() checks it, and then requeues
the work:

CPU0                              CPU1
nvme_alloc_ns()
  nvme_init_ns_head()
    mutex_lock(&subsys->lock)
    list_add_tail_rcu(..., &head->list)
    mutex_unlock(&subsys->lock)
                                  delayed_removal_secs_store()
                                    mutex_lock(&subsys->lock)
                                    cancel_delayed_work(&head->remove_work)
                                      (returns true)
    cancel_delayed_work(&head->remove_work)
      (returns false)
                                    queue_delayed_work(..., &head->remove_work, ...)
                                    mutex_unlock(&subsys->lock)
  ...
  out_unlink_ns:
    nvme_put_ns_head(ns->head)    <-- frees head with remove_work queued

This leaves head->remove_work pending while a path is on head->list.  If
nvme_alloc_ns() then fails and takes its out_unlink_ns error path, it
drops both references and frees head while head->remove_work is still
queued on nvme_wq, causing a use-after-free when the work runs.

Checking list_empty(&head->list) under head->subsys->lock before
cancelling and requeuing head->remove_work (or moving the cancel in
nvme_init_ns_head() inside subsys->lock) would avoid this race.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-bug-nvme-mpath-set-live-srcu-v3-0-d84651de6334@linux.dev?part=1