[PATCH] scsi: target: tcm_fc: annotate session hash traversals

[PATCH] scsi: target: tcm_fc: annotate session hash traversals

[Runyu Xiao]

The TCM FC session hash is updated with RCU hlist primitives. Several
writer-side helpers traverse the hash with hlist_for_each_entry_rcu()
while the file-level comments state that the caller holds ft_lport_lock:

ft_sess_create()
ft_sess_delete()
ft_sess_delete_all()

Other tcm_fc paths already use lockdep_is_held(&ft_lport_lock) when
dereferencing RCU-protected local-port state. Pass the same lockdep
condition to these session hash iterators so CONFIG_PROVE_RCU_LIST can
distinguish the ft_lport_lock-protected writer paths from ordinary RCU
readers.

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 session hash walkers produced the expected "RCU-list traversed
in non-reader section!!" warning with stacks matching ft_sess_create()/
ft_prli_locked(), ft_sess_delete()/ft_prlo(), and ft_sess_delete_all()/
ft_lport_del(). The change is limited to documenting the existing
protection contract.

This is a lockdep annotation cleanup and does not change the session
hash lifetime model.

Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/target/tcm_fc/tfc_sess.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/target/tcm_fc/tfc_sess.c b/drivers/target/tcm_fc/tfc_sess.c
index 797be06ab71b..84a6f29a1b5f 100644
--- a/drivers/target/tcm_fc/tfc_sess.c
+++ b/drivers/target/tcm_fc/tfc_sess.c
@@ -215,7 +215,8 @@ static struct ft_sess *ft_sess_create(struct ft_tport *tport, u32 port_id,
 	ft_format_wwn(&initiatorname[0], TRANSPORT_IQN_LEN, rdata->ids.port_name);
 
 	head = &tport->hash[ft_sess_hash(port_id)];
-	hlist_for_each_entry_rcu(sess, head, hash)
+	hlist_for_each_entry_rcu(sess, head, hash,
+				 lockdep_is_held(&ft_lport_lock))
 		if (sess->port_id == port_id)
 			return sess;
 
@@ -264,7 +265,8 @@ static struct ft_sess *ft_sess_delete(struct ft_tport *tport, u32 port_id)
 	struct ft_sess *sess;
 
 	head = &tport->hash[ft_sess_hash(port_id)];
-	hlist_for_each_entry_rcu(sess, head, hash) {
+	hlist_for_each_entry_rcu(sess, head, hash,
+				 lockdep_is_held(&ft_lport_lock)) {
 		if (sess->port_id == port_id) {
 			ft_sess_unhash(sess);
 			return sess;
@@ -291,7 +293,8 @@ static void ft_sess_delete_all(struct ft_tport *tport)
 
 	for (head = tport->hash;
 	     head < &tport->hash[FT_SESS_HASH_SIZE]; head++) {
-		hlist_for_each_entry_rcu(sess, head, hash) {
+		hlist_for_each_entry_rcu(sess, head, hash,
+					 lockdep_is_held(&ft_lport_lock)) {
 			ft_sess_unhash(sess);
 			ft_close_sess(sess);	/* release from table */
 		}
-- 
2.34.1

Re: [PATCH] scsi: target: tcm_fc: annotate session hash traversals

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Unsafe RCU list traversal in `ft_sess_delete_all` causing Iterator Use-After-Free.
- [Critical] `ft_sess_get` uses `kref_get` instead of `kref_get_unless_zero` during RCU list traversal, causing refcount corruption.
--

commit 3175abadc099d1a62c16806e23dfcc68463481a8
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

scsi: target: tcm_fc: annotate session hash traversals

This commit adds lockdep condition annotations to several RCU hlist
traversals in the TCM FC session hash to document the writer-side
protection under ft_lport_lock.

> diff --git a/drivers/target/tcm_fc/tfc_sess.c b/drivers/target/tcm_fc/tfc_sess.c
> index 797be06ab71b8..84a6f29a1b5f8 100644
> --- a/drivers/target/tcm_fc/tfc_sess.c
> +++ b/drivers/target/tcm_fc/tfc_sess.c

[ ... ]

> @@ -291,7 +293,8 @@ static void ft_sess_delete_all(struct ft_tport *tport)
>  
>  	for (head = tport->hash;
>  	     head < &tport->hash[FT_SESS_HASH_SIZE]; head++) {
> -		hlist_for_each_entry_rcu(sess, head, hash) {
> +		hlist_for_each_entry_rcu(sess, head, hash,
> +					 lockdep_is_held(&ft_lport_lock)) {
>  			ft_sess_unhash(sess);
>  			ft_close_sess(sess);	/* release from table */
>  		}

[Severity: Critical]
This isn't a bug introduced by this patch, but is it safe to use the standard
hlist_for_each_entry_rcu macro here while the elements are being destroyed?

During ft_sess_delete_all(), the code unlinks the session and calls
ft_close_sess(). This drops the final reference and queues the object for RCU
freeing. 

Because the iteration does not hold rcu_read_lock(), the thread can be
preempted after ft_close_sess() returns. If the RCU grace period expires
during this preemption, the session object is freed.

When the thread resumes, the iterator macro will evaluate the next pointer
using the freed object, resulting in a use-after-free:

    rcu_dereference_raw(hlist_next_rcu(&(sess)->hash))
    
Should this use a safe iteration mechanism such as a while loop checking
!hlist_empty(head) instead?


While looking at the session hash logic, I noticed another pre-existing
issue in the reader path:

drivers/target/tcm_fc/tfc_sess.c:ft_sess_get() {
        ...
        hlist_for_each_entry_rcu(sess, head, hash) {
                if (sess->port_id == port_id) {
                        kref_get(&sess->kref);
                        rcu_read_unlock();
                        return sess;
                }
        }
        ...
}

[Severity: Critical]
This is a pre-existing issue, but does this kref_get() risk reviving a
session that has already dropped its reference count to zero?

If a remote port sends a data frame concurrently with a process logout
request, the reader thread in ft_sess_get() could find the target session
but get preempted before executing kref_get().

Concurrently, the writer thread handling the logout in ft_sess_delete()
removes the session from the list and drops the final kref to 0, queuing
it for freeing.

When the reader thread resumes, it unconditionally increments the zero-ref
kref, resulting in accessing an object queued for destruction.

Does the reader need to use kref_get_unless_zero() here and handle the
failure case appropriately?

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