Re: [PATCH] scsi: target: tcm_fc: use kref_get_unless_zero() in ft_sess_get()

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

Pre-existing issues:
- [High] Use-after-free in ft_sess_delete_all() due to unsafe RCU list iteration while blocking
--

--- Patch [1]: [PATCH] scsi: target: tcm_fc: use kref_get_unless_zero() in ft_sess_get() ---
commit 7c29402c5a3237eba08c6ba5beba9b0842deb5e7
Author: Yifei Gao <[email protected]>

scsi: target: tcm_fc: use kref_get_unless_zero() in ft_sess_get()

ft_sess_get() walks the RCU-protected session hash under rcu_read_lock()
and takes a plain kref_get() on a matching session. Session teardown does
hlist_del_rcu() and then drops ft_lport_lock before the final
ft_sess_put() -> kfree_rcu(). A reader that is preempted between the
port_id comparison and the kref_get() can therefore revive a session whose
refcount has already dropped to zero and is pending free, leading to a
use-after-free and a double target_remove_session().

> diff --git a/drivers/target/tcm_fc/tfc_sess.c b/drivers/target/tcm_fc/tfc_sess.c
> index 797be06ab71b8..1bc7d2dcbea36 100644
> --- a/drivers/target/tcm_fc/tfc_sess.c
> +++ b/drivers/target/tcm_fc/tfc_sess.c
> @@ -172,7 +172,8 @@ static struct ft_sess *ft_sess_get(struct fc_lport *lport, u32 port_id)
>  	head = &tport->hash[ft_sess_hash(port_id)];
>  	hlist_for_each_entry_rcu(sess, head, hash) {
>  		if (sess->port_id == port_id) {
> -			kref_get(&sess->kref);
> +			if (!kref_get_unless_zero(&sess->kref))
> +				break;
>  			rcu_read_unlock();
>  			TFC_SESS_DBG(lport, "port_id %x found %p\n",
>  				     port_id, sess);

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the use of
RCU list iteration in this file, I noticed a pre-existing issue in
ft_sess_delete_all().

During port teardown, the code iterates over the session hash table:

drivers/target/tcm_fc/tfc_sess.c:ft_sess_delete_all() {
    ...
		hlist_for_each_entry_rcu(sess, head, hash) {
			ft_sess_unhash(sess);
			ft_close_sess(sess);	/* release from table */
		}
    ...
}

The iteration uses hlist_for_each_entry_rcu(), but ft_close_sess() calls
target_wait_for_sess_cmds() which blocks, and then drops the final table
reference triggering kfree_rcu(sess).

Because the thread sleeps, an RCU grace period can easily expire before the
loop macro evaluates the next element (sess->hash.next), resulting in a read
of freed memory.

Does this loop need to use a safe iterator that doesn't rely on RCU
while blocking, or restart the iteration after closing a session?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1