Re: [PATCH 18/23] nvmet: fix nvmet_ns::device_path 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_req_find_ns() [1]
--

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

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

This commit acquires subsys->lock in nvmet_ns_device_path_show() and moves
kfree(ns->device_path) under subsys->lock in nvmet_ns_free() to satisfy
Clang thread-safety analysis for nvmet_ns::device_path.

> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 805b5f70eb066..6b11beb2e2b20 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -730,13 +730,13 @@ void nvmet_ns_free(struct nvmet_ns *ns)
>  		subsys->max_nsid = nvmet_max_nsid(subsys);
>  
>  	subsys->nr_namespaces--;
> +	kfree(ns->device_path);
>  	mutex_unlock(&subsys->lock);
>  
>  	down_write(&nvmet_ana_sem);
>  	nvmet_ana_group_enabled[ns->anagrpid]--;
>  	up_write(&nvmet_ana_sem);
>  
> -	kfree(ns->device_path);
>  	kfree(ns);
>  }

[Severity: Critical]
This problem wasn't introduced by this patch, but can freeing ns immediately
after xa_erase() in nvmet_ns_free() cause a use-after-free in
nvmet_req_find_ns() [1]?

nvmet_ns_disable() waits for the RCU grace period (via percpu_ref_kill())
while ns is still published in subsys->namespaces.  nvmet_ns_free() then
erases the XArray entry and synchronously calls kfree(ns) without waiting
for another RCU grace period:

CPU0                                    CPU1
nvmet_ns_free()
  nvmet_ns_disable()
    clear_bit(NVMET_NS_IO_LIVE, ...)
    percpu_ref_kill(&ns->ref)
    wait_for_completion(...)
                                        nvmet_req_find_ns()
                                          rcu_read_lock()
                                          req->ns = xa_load(...)
  xa_erase(&subsys->namespaces, ...)
  ...
  kfree(ns)
                                          test_bit(NVMET_NS_IO_LIVE,
                                                   &req->ns->flags)
                                            <-- use-after-free

drivers/nvme/target/core.c:nvmet_req_find_ns() {
	rcu_read_lock();
	req->ns = xa_load(&subsys->namespaces, nsid);
	if (unlikely(!req->ns) ||
	    !test_bit(NVMET_NS_IO_LIVE, &req->ns->flags) ||
	    !percpu_ref_tryget_live_rcu(&req->ns->ref)) {
    ...
}

Because ns remains visible to xa_load() until xa_erase() runs in
nvmet_ns_free(), a reader entering nvmet_req_find_ns() after
nvmet_ns_disable() finishes (or when the namespace was never enabled) can
load req->ns just before xa_erase() and dereference it after kfree(ns).
Freeing ns with kfree_rcu() or waiting for an RCU grace period after
xa_erase() would prevent this race.

[1] https://sashiko.dev/#/bug/linux-a674b331-ed9a-4029-b6d0-2a1abec4aaf0

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