Re: [PATCH] scsi: target: core: annotate LUN hlist traversals

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Traversing an RCU list using `hlist_for_each_entry_rcu()` without `rcu_read_lock()` while concurrently removing and freeing elements leads to a Use-After-Free.
--

commit 0808004d8e16208c69c8bd1b2b9d1e2400a32c51
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

scsi: target: core: annotate LUN hlist traversals

This commit annotates LUN hlist traversals with lockdep conditions where
a mutex is held instead of an RCU read lock. This silences
"RCU-list traversed in non-reader section!!" warnings from
CONFIG_PROVE_RCU_LIST without changing semantics.

> diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> index 9db2201aa553f..82aa831c69c9b 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -237,7 +237,8 @@ void core_free_device_list_for_node(
>  	struct se_dev_entry *deve;
>  
>  	mutex_lock(&nacl->lun_entry_mutex);
> -	hlist_for_each_entry_rcu(deve, &nacl->lun_entry_hlist, link)
> +	hlist_for_each_entry_rcu(deve, &nacl->lun_entry_hlist, link,
> +				 lockdep_is_held(&nacl->lun_entry_mutex))
>  		core_disable_device_list_for_node(deve->se_lun, deve, nacl, tpg);

[Severity: High]
This isn't a bug introduced by this patch, but does this loop contain a
potential use-after-free when advancing the iterator?

Inside the loop, core_disable_device_list_for_node() removes the current
element from the list and queues it for deferred freeing:

core_disable_device_list_for_node() {
	...
	hlist_del_rcu(&orig->link);
	...
	call_rcu(&orig->rcu_head, target_free_dev_entry);
	...
}

Because the traversal is protected only by lun_entry_mutex and not by
rcu_read_lock(), the thread is fully preemptible. If preemption occurs
after call_rcu() returns but before the iterator macro evaluates
deve->link.next to advance to the next element, a full RCU grace period
can expire.

This would allow the RCU callback to execute and free the current element,
causing a use-after-free when the thread resumes and dereferences the
freed memory's next pointer.

[ ... ]

> @@ -483,7 +484,8 @@ void core_clear_lun_from_tpg(struct se_lun *lun, struct se_portal_group *tpg)
>  	list_for_each_entry(nacl, &tpg->acl_node_list, acl_list) {
>  
>  		mutex_lock(&nacl->lun_entry_mutex);
> -		hlist_for_each_entry_rcu(deve, &nacl->lun_entry_hlist, link) {
> +		hlist_for_each_entry_rcu(deve, &nacl->lun_entry_hlist, link,
> +					 lockdep_is_held(&nacl->lun_entry_mutex)) {
>  			if (lun != deve->se_lun)
>  				continue;
>  
>  			core_disable_device_list_for_node(lun, deve, nacl, tpg);

[Severity: High]
This is a pre-existing issue, but does this traversal have the same potential
use-after-free as above?

It calls core_disable_device_list_for_node() which queues the element for
freeing via call_rcu() while we are not in an RCU read-side critical section,
meaning the memory could be freed before we read the next pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625113807.574788-1-runyu.xiao@seu.edu.cn?part=1