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

[sashiko-bot@kernel.org]

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