Re: [PATCH 02/23] nvmet: fix nvmet_ns::bdev access using nvmet_subsys::lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] nvmet: use-after-free in nvmet_for_each_enabled_ns() [1]
--

commit c1fe0968cd1483b32634bdd9e21897a686ef88f2
Author: Nilay Shroff <nilay@linux.ibm.com>

nvmet: fix nvmet_ns::bdev access using nvmet_subsys::lock

This commit acquires ctrl->subsys->lock in nvmet_get_smart_log_all() while
iterating over enabled namespaces and reading ns->bdev statistics, and adds
an __assume_ctx_lock() annotation for the clang context analyzer.

> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 2d4e21b3ee7d1..517f8cc784734 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -289,7 +289,15 @@ static u16 nvmet_get_smart_log_all(struct nvmet_req *req,
>  	unsigned long idx;
>  
>  	ctrl = req->sq->ctrl;
> +	mutex_lock(&ctrl->subsys->lock);
>  	nvmet_for_each_enabled_ns(&ctrl->subsys->namespaces, idx, ns) {

[Severity: Critical]
This problem wasn't introduced by this patch, but can the remaining
unlocked nvmet_for_each_enabled_ns() loops [1] in
nvmet_format_ana_group(), nvmet_execute_identify_endgrp_list(),
nvmet_execute_identify_nslist(), and nvmet_set_feat_resv_notif_mask()
still hit a use-after-free on ns when racing with namespace removal?

nvmet_for_each_enabled_ns() expands to xa_for_each_marked(), which drops
rcu_read_lock() before yielding each entry to the caller:

drivers/nvme/target/admin-cmd.c:nvmet_execute_identify_nslist() {
    ...
	nvmet_for_each_enabled_ns(&ctrl->subsys->namespaces, idx, ns) {
		if (ns->nsid <= min_nsid)
			continue;
    ...
}

Because those callers hold neither subsys->lock nor rcu_read_lock() across
the loop body, synchronize_rcu() in nvmet_ns_disable() does not wait for
them before nvmet_ns_free() frees ns:

CPU0                                  CPU1
nvmet_execute_identify_nslist()
  nvmet_for_each_enabled_ns()
    xa_for_each_marked()
      // drops rcu_read_lock()
                                      nvmet_ns_free()
                                        nvmet_ns_disable()
                                          synchronize_rcu()
                                        kfree(ns)
    if (ns->nsid <= min_nsid)  <-- use-after-free

Holding rcu_read_lock() (or ctrl->subsys->lock) around those loops would
prevent ns from being freed mid-iteration.

[1] https://sashiko.dev/#/bug/linux-dd6d8d28-fcd8-41b8-9469-54cca852794f

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109033102.982191-25-nilay@linux.ibm.com?part=2