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

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

[Runyu Xiao]

Some target-core writer-side helpers walk RCU hlist state while already
holding the mutex that serializes the operation:

core_free_device_list_for_node() holds nacl->lun_entry_mutex
core_clear_lun_from_tpg() holds nacl->lun_entry_mutex
core_tpg_add_node_to_devs() holds tpg->tpg_lun_mutex

The code already uses the same lockdep condition for the
rcu_dereference_check() of lun->lun_se_dev in
core_tpg_add_node_to_devs(). Pass the corresponding mutex condition to
the RCU hlist iterators as well so CONFIG_PROVE_RCU_LIST can see the
non-RCU protection.

This was found by our static analysis tool and then manually reviewed
against the current tree. In the reviewed CONFIG_PROVE_RCU_LIST triage
runs, the LUN hlist walkers produced the expected "RCU-list traversed
in non-reader section!!" warning with stacks matching
core_free_device_list_for_node(), core_clear_lun_from_tpg(), and
core_tpg_add_node_to_devs(). The change is limited to documenting the
existing protection contract.

This is a lockdep annotation cleanup. It does not change LUN list
lifetime or serialization.

Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/target/target_core_device.c | 6 ++++--
 drivers/target/target_core_tpg.c    | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 9db2201aa553..82aa831c69c9 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);
 	mutex_unlock(&nacl->lun_entry_mutex);
 }
@@ -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;
 
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index ccff9f6e4826..74bfcaa11188 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -111,7 +111,8 @@ void core_tpg_add_node_to_devs(
 	struct se_device *dev;
 
 	mutex_lock(&tpg->tpg_lun_mutex);
-	hlist_for_each_entry_rcu(lun, &tpg->tpg_lun_hlist, link) {
+	hlist_for_each_entry_rcu(lun, &tpg->tpg_lun_hlist, link,
+				 lockdep_is_held(&tpg->tpg_lun_mutex)) {
 		if (lun_orig && lun != lun_orig)
 			continue;
 
-- 
2.34.1

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

[Runyu Xiao]

Hi,

Sorry, this patch was sent twice by mistake. Please ignore this duplicate copy.

Thanks,
Runyu

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

[sashiko-bot]

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