[PATCH v3] scsi: elx: efct: fix target session lifetime against node teardown

[PATCH v3] scsi: elx: efct: fix target session lifetime against node teardown

[Yehyeong Lee]

efct_scsi_new_initiator() hands a bare struct efc_node pointer to lio_wq
and returns EFC_SCSI_CALL_ASYNC, so the node state machine keeps running
while efct_lio_setup_session() is still queued. Three things go wrong
over that window.

The node itself can be freed under the work item. efc_node_alloc() takes
a single reference and efc_node_free() drops it as the last step of a
node shutdown, which a peer LOGO, an FC link down, a domain or fabric
change or target teardown can all start. efct_get_vport_tpg() reads
node->nport before target_setup_session(), efct_session_cb() reads it
from inside that call, and node->tgt_node is read after it returns. The
node pool is created with curr_nr == min_nr, so mempool_free() calls
kfree() rather than refilling the reserve and the work item reads freed
slab memory:

  BUG: KASAN: slab-use-after-free in efct_lio_setup_session+0xd63/0xe20
  Read of size 8 at addr ffff888002d538a0 by task kworker/u4:0/12

  CPU: 0 UID: 0 PID: 12 Comm: kworker/u4:0 Tainted: G                 N  7.3.0-00343-g88d83015fdb1-dirty #5 PREEMPT(lazy)
  Tainted: [N]=TEST
  Workqueue: efct-kunit-wq efct_lio_setup_session
  Call Trace:
   <TASK>
   print_report+0xcd/0x620
   ? efct_lio_setup_session+0xd63/0xe20
   ? __virt_addr_valid+0x208/0x3f0
   ? efct_lio_setup_session+0xd63/0xe20
   kasan_report+0xfb/0x130
   ? efct_lio_setup_session+0xd63/0xe20
   efct_lio_setup_session+0xd63/0xe20
   ? __pfx_efct_lio_setup_session+0x10/0x10
   ? lock_acquire+0x100/0x2d0
   ? process_one_work+0x835/0x1c40
   ? process_one_work+0x735/0x1c40
   ? lock_release+0xc3/0x250
   process_one_work+0x88b/0x1c40
   ? __pfx_process_one_work+0x10/0x10
   ? lock_acquire+0x100/0x2d0
   ? worker_thread+0x2c7/0x11b0
   ? lock_is_held_type+0x84/0xf0
   ? __pfx_efct_lio_setup_session+0x10/0x10
   worker_thread+0x649/0x11b0
   ? __pfx_worker_thread+0x10/0x10
   kthread+0x3aa/0x550
   ? _raw_spin_unlock_irq+0x23/0x40
   ? __pfx_kthread+0x10/0x10
   ret_from_fork+0x493/0x940
   ? __pfx_ret_from_fork+0x10/0x10
   ? __switch_to+0x22/0xdc0
   ? __pfx_kthread+0x10/0x10
   ret_from_fork_asm+0x19/0x30
   </TASK>

  Allocated by task 33:
   kasan_save_stack+0x33/0x60
   kasan_save_track+0x14/0x30
   __kasan_kmalloc+0x8f/0xa0
   __kmalloc_noprof+0x23b/0x580
   mempool_alloc_noprof+0x143/0x230
   efct_kt_node_alloc.isra.0+0x38/0x4b0
   efct_kt_uaf_after_cb+0x7a/0x370
   kunit_try_run_case+0x1ca/0x4c0
   kunit_generic_run_threadfn_adapter+0x7b/0xe0
   kthread+0x3aa/0x550
   ret_from_fork+0x493/0x940
   ret_from_fork_asm+0x19/0x30

  Freed by task 12:
   kasan_save_stack+0x33/0x60
   kasan_save_track+0x14/0x30
   kasan_save_free_info+0x3b/0x60
   __kasan_slab_free+0x43/0x70
   kfree+0x119/0x490
   mempool_free+0xbc/0x100
   efc_node_free+0x311/0x4a0
   efct_lio_setup_session+0xa57/0xe20
   process_one_work+0x88b/0x1c40
   worker_thread+0x649/0x11b0
   kthread+0x3aa/0x550
   ret_from_fork+0x493/0x940
   ret_from_fork_asm+0x19/0x30

  The buggy address belongs to the object at ffff888002d53800
   which belongs to the cache kmalloc-1k of size 1024
  The buggy address is located 160 bytes inside of
   freed 1024-byte region [ffff888002d53800, ffff888002d53c00)

Teardown can also run while the session does not exist yet.
__efc_d_initiate_shutdown() moves the node to __efc_d_wait_del_node and
only then calls efct_scsi_del_initiator(), which finds node->tgt_node
still NULL and returns -EIO without queueing the removal work. The node
waits there with node->nport set and node->req_free still clear, since
req_free is only set later in __efc_node_shutdown(). The setup work
therefore sees nothing wrong, registers a session, and nothing ever
removes it: the se_session and the struct efct_node leak, and
se_session->fabric_sess_ptr keeps pointing at a node that is freed
moments later.

Or teardown can run just after the session became visible.
efct_session_cb() publishes node->tgt_node from inside
target_setup_session(). If efct_scsi_del_initiator() runs between that
and the end of the setup work it erases the efct->lookup entry and
queues the removal work, and the tail of the setup work then puts the
entry back. efct_lio_remove_session() frees the struct efct_node without
erasing the key a second time, so efct->lookup keeps a freed pointer
that efct_node_find() returns, and takes a reference on, for every
inbound FCP or BLS frame. That is the case the comment above
efct_node_find() states cannot happen.

Give the node a reference for the life of the work item, and give the
backend an explicit session state, read and written under efc->lock,
which is the lock the node state machine holds when it calls in. The
work claims EFCT_TGT_SESS_SETUP before it is queued and reaches
EFCT_TGT_SESS_REGISTERED in the same critical section that stores the
lookup entry, so a store can no longer follow an erase.

efct_scsi_del_initiator() now decides from that state rather than from
node->tgt_node. While a setup is in flight it marks the node
EFCT_TGT_SESS_DELETE_PENDING and returns EFC_SCSI_CALL_ASYNC, so the
node waits in __efc_d_wait_del_node instead of running ahead of the work
that still holds the session. The work unwinds the session it built and
then calls efc_scsi_del_initiator_complete(), which posts the
EFC_EVT_NODE_DEL_INI_COMPLETE the node is parked on. Once the session is
registered the existing removal path runs unchanged. Every exit from the
work now reports exactly one completion, either that one or a session
registration result, so no path leaves the node waiting.

The same state closes a smaller hole next door. An initiator that
repeats PRLI while registration is pending used to get a second session
for one node, which left the first one unreachable.
efct_scsi_new_initiator() now returns EFC_SCSI_CALL_ASYNC for the
repeat, which is what the first PRLI already made the node wait for.

Two earlier versions got the handshake wrong. v1 gated the work on
node->nport and node->req_free, which is not enough, because req_free is
only set well after efct_scsi_del_initiator() has run and returned. v2
replaced that with the state above but reported the two races with
EFC_SCSI_CALL_COMPLETE, which let efc_d_send_prli_rsp() and
__efc_d_initiate_shutdown() advance the node while the work was still in
flight, so this version reports them asynchronously instead.

Fixes: 692e5d73a811 ("scsi: elx: efct: LIO backend interface routines")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/elx/efct/efct_lio.c | 98 +++++++++++++++++++++++++++++---
 drivers/scsi/elx/efct/efct_lio.h |  9 +++
 drivers/scsi/elx/libefc/efclib.h |  2 +
 3 files changed, 101 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/elx/efct/efct_lio.c b/drivers/scsi/elx/efct/efct_lio.c
index 67d686dd6fb33..9ddab907556da 100644
--- a/drivers/scsi/elx/efct/efct_lio.c
+++ b/drivers/scsi/elx/efct/efct_lio.c
@@ -1062,17 +1062,33 @@ _efct_tgt_node_free(struct kref *arg)
 	kfree(tgt_node);
 }
 
+/* True only while a setup is in flight. The caller holds efc->lock. */
+static bool efct_tgt_sess_may_register(struct efc_node *node)
+{
+	return node->tgt_sess_state == EFCT_TGT_SESS_SETUP &&
+	       node->nport && !node->req_free;
+}
+
 static int efct_session_cb(struct se_portal_group *se_tpg,
 			   struct se_session *se_sess, void *private)
 {
 	struct efc_node *node = private;
 	struct efct_node *tgt_node;
 	struct efct *efct = node->efc->base;
+	unsigned long flags;
 
 	tgt_node = kzalloc_obj(*tgt_node);
 	if (!tgt_node)
 		return -ENOMEM;
 
+	/* target_setup_session() sleeps, so the node may be in teardown now */
+	spin_lock_irqsave(&node->efc->lock, flags);
+	if (!efct_tgt_sess_may_register(node)) {
+		spin_unlock_irqrestore(&node->efc->lock, flags);
+		kfree(tgt_node);
+		return -ENODEV;
+	}
+
 	kref_init(&tgt_node->ref);
 	tgt_node->release = _efct_tgt_node_free;
 
@@ -1086,6 +1102,7 @@ static int efct_session_cb(struct se_portal_group *se_tpg,
 	tgt_node->port_fc_id = node->nport->fc_id;
 	tgt_node->vpi = node->nport->indicator;
 	tgt_node->rpi = node->rnode.indicator;
+	spin_unlock_irqrestore(&node->efc->lock, flags);
 
 	spin_lock_init(&tgt_node->active_ios_lock);
 	INIT_LIST_HEAD(&tgt_node->active_ios);
@@ -1157,15 +1174,28 @@ static void efct_lio_setup_session(struct work_struct *work)
 		container_of(work, struct efct_lio_wq_data, work);
 	struct efct *efct = wq_data->efct;
 	struct efc_node *node = wq_data->ptr;
+	struct efc *efc = node->efc;
 	char wwpn[WWN_NAME_LEN];
 	struct efct_lio_tpg *tpg;
 	struct efct_node *tgt_node;
 	struct se_portal_group *se_tpg;
 	struct se_session *se_sess;
+	unsigned long flags;
+	bool deleted = false;
 	int watermark;
 	int ini_count;
 	u64 id;
 
+	spin_lock_irqsave(&efc->lock, flags);
+	if (!efct_tgt_sess_may_register(node)) {
+		deleted = node->tgt_sess_state == EFCT_TGT_SESS_DELETE_PENDING;
+		node->tgt_sess_state = EFCT_TGT_SESS_NONE;
+		spin_unlock_irqrestore(&efc->lock, flags);
+		efc_log_debug(efct, "node %p torn down before session setup\n",
+			      node);
+		goto done;
+	}
+
 	/* Check to see if it's belongs to vport,
 	 * if not get physical port
 	 */
@@ -1176,8 +1206,10 @@ static void efct_lio_setup_session(struct work_struct *work)
 		tpg = efct->tgt_efct.tpg;
 		se_tpg = &tpg->tpg;
 	} else {
+		node->tgt_sess_state = EFCT_TGT_SESS_NONE;
+		spin_unlock_irqrestore(&efc->lock, flags);
 		efc_log_err(efct, "failed to init session\n");
-		return;
+		goto done;
 	}
 
 	/*
@@ -1186,14 +1218,38 @@ static void efct_lio_setup_session(struct work_struct *work)
 	 * ConfigFS NodeACLs.
 	 */
 	efct_format_wwn(wwpn, sizeof(wwpn), "",	efc_node_get_wwpn(node));
+	spin_unlock_irqrestore(&efc->lock, flags);
 
 	se_sess = target_setup_session(se_tpg, 0, 0, TARGET_PROT_NORMAL, wwpn,
 				       node, efct_session_cb);
 	if (IS_ERR(se_sess)) {
 		efc_log_err(efct, "failed to setup session\n");
-		kfree(wq_data);
-		efc_scsi_sess_reg_complete(node, -EIO);
-		return;
+		spin_lock_irqsave(&efc->lock, flags);
+		deleted = node->tgt_sess_state == EFCT_TGT_SESS_DELETE_PENDING;
+		node->tgt_sess_state = EFCT_TGT_SESS_NONE;
+		spin_unlock_irqrestore(&efc->lock, flags);
+		goto done;
+	}
+
+	spin_lock_irqsave(&efc->lock, flags);
+	if (!efct_tgt_sess_may_register(node)) {
+		deleted = node->tgt_sess_state == EFCT_TGT_SESS_DELETE_PENDING;
+		tgt_node = node->tgt_node;
+		node->tgt_node = NULL;
+		node->tgt_sess_state = EFCT_TGT_SESS_NONE;
+		spin_unlock_irqrestore(&efc->lock, flags);
+		/*
+		 * kfree() and not kref_put(), because the release would post a
+		 * second completion. Nothing else can reach tgt_node, it never
+		 * entered efct->lookup and node->tgt_node is clear.
+		 */
+		efc_log_debug(efct, "node %p torn down during session setup\n",
+			      node);
+		target_stop_session(se_sess);
+		target_wait_for_sess_cmds(se_sess);
+		target_remove_session(se_sess);
+		kfree(tgt_node);
+		goto done;
 	}
 
 	tgt_node = node->tgt_node;
@@ -1202,8 +1258,10 @@ static void efct_lio_setup_session(struct work_struct *work)
 	efc_log_debug(efct, "new initiator sess=%p node=%p id: %llx\n",
 		      se_sess, node, id);
 
-	if (xa_err(xa_store(&efct->lookup, id, tgt_node, GFP_KERNEL)))
+	if (xa_err(xa_store(&efct->lookup, id, tgt_node, GFP_ATOMIC)))
 		efc_log_err(efct, "Node lookup store failed\n");
+	node->tgt_sess_state = EFCT_TGT_SESS_REGISTERED;
+	spin_unlock_irqrestore(&efc->lock, flags);
 
 	efc_scsi_sess_reg_complete(node, 0);
 
@@ -1216,6 +1274,17 @@ static void efct_lio_setup_session(struct work_struct *work)
 	atomic_set(&efct->tgt_efct.io_high_watermark, watermark);
 
 	kfree(wq_data);
+	kref_put(&node->ref, node->release);
+	return;
+
+done:
+	/* every exit owes the node exactly one completion */
+	if (deleted)
+		efc_scsi_del_initiator_complete(efc, node);
+	else
+		efc_scsi_sess_reg_complete(node, -EIO);
+	kfree(wq_data);
+	kref_put(&node->ref, node->release);
 }
 
 int efct_scsi_new_initiator(struct efc *efc, struct efc_node *node)
@@ -1223,6 +1292,10 @@ int efct_scsi_new_initiator(struct efc *efc, struct efc_node *node)
 	struct efct *efct = node->efc->base;
 	struct efct_lio_wq_data *wq_data;
 
+	/* a repeated PRLI waits for the in-flight setup, it does not add one */
+	if (node->tgt_sess_state != EFCT_TGT_SESS_NONE)
+		return EFC_SCSI_CALL_ASYNC;
+
 	/*
 	 * Since LIO only supports initiator validation at thread level,
 	 * we are open minded and accept all callers.
@@ -1231,6 +1304,10 @@ int efct_scsi_new_initiator(struct efc *efc, struct efc_node *node)
 	if (!wq_data)
 		return -ENOMEM;
 
+	/* the work outlives this call and undoes both of these */
+	kref_get(&node->ref);
+	node->tgt_sess_state = EFCT_TGT_SESS_SETUP;
+
 	wq_data->ptr = node;
 	wq_data->efct = efct;
 	INIT_WORK(&wq_data->work, efct_lio_setup_session);
@@ -1287,17 +1364,22 @@ int efct_scsi_del_initiator(struct efc *efc, struct efc_node *node, int reason)
 	if (reason == EFCT_SCSI_INITIATOR_MISSING)
 		return EFC_SCSI_CALL_COMPLETE;
 
-	if (!tgt_node) {
-		efc_log_err(efct, "tgt_node is NULL\n");
-		return -EIO;
+	if (node->tgt_sess_state == EFCT_TGT_SESS_SETUP) {
+		/* the setup work unwinds it and completes the delete */
+		node->tgt_sess_state = EFCT_TGT_SESS_DELETE_PENDING;
+		return EFC_SCSI_CALL_ASYNC;
 	}
 
+	if (node->tgt_sess_state != EFCT_TGT_SESS_REGISTERED || !tgt_node)
+		return EFC_SCSI_CALL_COMPLETE;
+
 	wq_data = kzalloc_obj(*wq_data, GFP_ATOMIC);
 	if (!wq_data)
 		return -ENOMEM;
 
 	id = (u64) tgt_node->port_fc_id << 32 | tgt_node->node_fc_id;
 	xa_erase(&efct->lookup, id);
+	node->tgt_sess_state = EFCT_TGT_SESS_NONE;
 
 	wq_data->ptr = node;
 	wq_data->efct = efct;
diff --git a/drivers/scsi/elx/efct/efct_lio.h b/drivers/scsi/elx/efct/efct_lio.h
index 569a0d4b1894c..8f550ad0e349b 100644
--- a/drivers/scsi/elx/efct/efct_lio.h
+++ b/drivers/scsi/elx/efct/efct_lio.h
@@ -65,6 +65,15 @@ struct efct_scsi_tgt_nport {
 	struct efct_lio_nport	*lio_nport;
 };
 
+/* Target session state of a struct efc_node, kept under efc->lock. */
+enum efct_tgt_sess_state {
+	EFCT_TGT_SESS_NONE = 0,
+	EFCT_TGT_SESS_SETUP,
+	EFCT_TGT_SESS_REGISTERED,
+	/* teardown raced setup; the setup work must unwind it */
+	EFCT_TGT_SESS_DELETE_PENDING,
+};
+
 struct efct_node {
 	struct list_head	list_entry;
 	struct kref		ref;
diff --git a/drivers/scsi/elx/libefc/efclib.h b/drivers/scsi/elx/libefc/efclib.h
index 57e3386128127..7c00a3fd79ad6 100644
--- a/drivers/scsi/elx/libefc/efclib.h
+++ b/drivers/scsi/elx/libefc/efclib.h
@@ -336,6 +336,8 @@ struct efc_node {
 	struct list_head	els_ios_list;
 	void			*ini_node;
 	void			*tgt_node;
+	/* enum efct_tgt_sess_state, private to the target backend */
+	u8			tgt_sess_state;
 
 	struct efc_remote_node	rnode;
 	/* Declarations private to FC trannport */

base-commit: f47d223627f80cb7a1eb14c585fe6cb35de5e984
-- 
2.43.0

Re: [PATCH v3] scsi: elx: efct: fix target session lifetime against node teardown

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] scsi: elx: efct: race in efct_lio_setup_session() dropping lock before completion
- [High] scsi: elx: efct: second efc_node_free() call in efct_lio_setup_session()
- [Medium] scsi: elx: efct: efct_scsi_new_initiator() hangs node when session is registered
--

commit 0dc470d1061493230f83a79f4682d76a8f9318fc
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: elx: efct: fix target session lifetime against node teardown

This commit adds reference counting on struct efc_node across target session
setup work items and introduces an explicit target session state machine
protected by efc->lock to synchronize session creation with node teardown.

> diff --git a/drivers/scsi/elx/efct/efct_lio.c b/drivers/scsi/elx/efct/efct_lio.c
> index 67d686dd6fb33..9ddab907556da 100644
> --- a/drivers/scsi/elx/efct/efct_lio.c
> +++ b/drivers/scsi/elx/efct/efct_lio.c
> @@ -1062,17 +1062,33 @@ _efct_tgt_node_free(struct kref *arg)
>  	kfree(tgt_node);
>  }
>  
> +/* True only while a setup is in flight. The caller holds efc->lock. */
> +static bool efct_tgt_sess_may_register(struct efc_node *node)
> +{
> +	return node->tgt_sess_state == EFCT_TGT_SESS_SETUP &&
> +	       node->nport && !node->req_free;
> +}

[ ... ]

> @@ -1157,15 +1174,28 @@ static void efct_lio_setup_session(struct work_struct *work)
>  		container_of(work, struct efct_lio_wq_data, work);
>  	struct efct *efct = wq_data->efct;
>  	struct efc_node *node = wq_data->ptr;
> +	struct efc *efc = node->efc;
>  	char wwpn[WWN_NAME_LEN];
>  	struct efct_lio_tpg *tpg;
>  	struct efct_node *tgt_node;
>  	struct se_portal_group *se_tpg;
>  	struct se_session *se_sess;
> +	unsigned long flags;
> +	bool deleted = false;
>  	int watermark;
>  	int ini_count;
>  	u64 id;
>  
> +	spin_lock_irqsave(&efc->lock, flags);
> +	if (!efct_tgt_sess_may_register(node)) {
> +		deleted = node->tgt_sess_state == EFCT_TGT_SESS_DELETE_PENDING;
> +		node->tgt_sess_state = EFCT_TGT_SESS_NONE;
> +		spin_unlock_irqrestore(&efc->lock, flags);
> +		efc_log_debug(efct, "node %p torn down before session setup\n",
> +			      node);
> +		goto done;
> +	}
> +
>  	/* Check to see if it's belongs to vport,
>  	 * if not get physical port
>  	 */
> @@ -1176,8 +1206,10 @@ static void efct_lio_setup_session(struct work_struct *work)
>  		tpg = efct->tgt_efct.tpg;
>  		se_tpg = &tpg->tpg;
>  	} else {
> +		node->tgt_sess_state = EFCT_TGT_SESS_NONE;
> +		spin_unlock_irqrestore(&efc->lock, flags);
>  		efc_log_err(efct, "failed to init session\n");
> -		return;
> +		goto done;
>  	}
>  
>  	/*
> @@ -1186,14 +1218,38 @@ static void efct_lio_setup_session(struct work_struct *work)
>  	 * ConfigFS NodeACLs.
>  	 */
>  	efct_format_wwn(wwpn, sizeof(wwpn), "",	efc_node_get_wwpn(node));
> +	spin_unlock_irqrestore(&efc->lock, flags);
>  
>  	se_sess = target_setup_session(se_tpg, 0, 0, TARGET_PROT_NORMAL, wwpn,
>  				       node, efct_session_cb);
>  	if (IS_ERR(se_sess)) {
>  		efc_log_err(efct, "failed to setup session\n");
> -		kfree(wq_data);
> -		efc_scsi_sess_reg_complete(node, -EIO);
> -		return;
> +		spin_lock_irqsave(&efc->lock, flags);
> +		deleted = node->tgt_sess_state == EFCT_TGT_SESS_DELETE_PENDING;
> +		node->tgt_sess_state = EFCT_TGT_SESS_NONE;
> +		spin_unlock_irqrestore(&efc->lock, flags);
> +		goto done;
> +	}
> +
> +	spin_lock_irqsave(&efc->lock, flags);
> +	if (!efct_tgt_sess_may_register(node)) {
> +		deleted = node->tgt_sess_state == EFCT_TGT_SESS_DELETE_PENDING;
> +		tgt_node = node->tgt_node;
> +		node->tgt_node = NULL;
> +		node->tgt_sess_state = EFCT_TGT_SESS_NONE;
> +		spin_unlock_irqrestore(&efc->lock, flags);
> +		/*
> +		 * kfree() and not kref_put(), because the release would post a
> +		 * second completion. Nothing else can reach tgt_node, it never
> +		 * entered efct->lookup and node->tgt_node is clear.
> +		 */
> +		efc_log_debug(efct, "node %p torn down during session setup\n",
> +			      node);
> +		target_stop_session(se_sess);
> +		target_wait_for_sess_cmds(se_sess);
> +		target_remove_session(se_sess);
> +		kfree(tgt_node);
> +		goto done;
> +	}
>  
>  	tgt_node = node->tgt_node;
> @@ -1202,8 +1258,10 @@ static void efct_lio_setup_session(struct work_struct *work)
>  	efc_log_debug(efct, "new initiator sess=%p node=%p id: %llx\n",
>  		      se_sess, node, id);
>  
> -	if (xa_err(xa_store(&efct->lookup, id, tgt_node, GFP_KERNEL)))
> +	if (xa_err(xa_store(&efct->lookup, id, tgt_node, GFP_ATOMIC)))
>  		efc_log_err(efct, "Node lookup store failed\n");
> +	node->tgt_sess_state = EFCT_TGT_SESS_REGISTERED;
> +	spin_unlock_irqrestore(&efc->lock, flags);
>  
>  	efc_scsi_sess_reg_complete(node, 0);

[Severity: High]
In efct_lio_setup_session(), node->tgt_sess_state is updated and efc->lock is
released before the completion event is posted to the node state machine.
Could this race with concurrent node teardown or a repeated PRLI?

On the failure paths (when no TPG is found, or when target_setup_session()
returns an error while node->tgt_sess_state == EFCT_TGT_SESS_SETUP),
efct_lio_setup_session() sets node->tgt_sess_state = EFCT_TGT_SESS_NONE and
drops efc->lock before jumping to done:, where efc_scsi_sess_reg_complete()
re-acquires efc->lock to post EFC_EVT_NODE_SESS_REG_FAIL.

During that unlocked window, the node is still in __efc_d_port_logged_in,
which allows two races:

1. If __efc_d_initiate_shutdown() -> efct_scsi_del_initiator() runs under
efc->lock in this window, efct_scsi_del_initiator() observes
node->tgt_sess_state == EFCT_TGT_SESS_NONE instead of EFCT_TGT_SESS_SETUP and
returns EFC_SCSI_CALL_COMPLETE instead of EFC_SCSI_CALL_ASYNC.
__efc_d_initiate_shutdown() then immediately posts
EFC_EVT_NODE_DEL_INI_COMPLETE and advances the node to __efc_node_shutdown().
If the shutdown is EFC_NODE_SHUTDOWN_IMPLICIT_LOGO, the node re-initiates
login and then receives the stale EFC_EVT_NODE_SESS_REG_FAIL from done:.

2. If a repeated PRLI arrives in __efc_d_port_logged_in during this unlocked
window, efct_scsi_new_initiator() sees node->tgt_sess_state ==
EFCT_TGT_SESS_NONE, sets EFCT_TGT_SESS_SETUP, and queues a second
efct_lio_setup_session() work item. The first work item then runs done: and
posts EFC_EVT_NODE_SESS_REG_FAIL, sending LS_RJT for the second PRLI's ox_id
and clearing node->send_ls_acc while the second work item is still in flight.

Similarly, on the success path above, node->tgt_sess_state is set to
EFCT_TGT_SESS_REGISTERED and efc->lock is dropped before
efc_scsi_sess_reg_complete(node, 0) and before atomic_add_return() on
efct->tgt_efct.initiator_count. A concurrent efct_scsi_del_initiator() can
run in that window before EFC_EVT_NODE_SESS_REG_OK is posted and decrement
initiator_count before it has been incremented.

>  
> @@ -1216,6 +1274,17 @@ static void efct_lio_setup_session(struct work_struct *work)
>  	atomic_set(&efct->tgt_efct.io_high_watermark, watermark);
>  
>  	kfree(wq_data);
> +	kref_put(&node->ref, node->release);
> +	return;
> +
> +done:
> +	/* every exit owes the node exactly one completion */
> +	if (deleted)
> +		efc_scsi_del_initiator_complete(efc, node);
> +	else
> +		efc_scsi_sess_reg_complete(node, -EIO);
> +	kfree(wq_data);
> +	kref_put(&node->ref, node->release);
>  }

[Severity: High]
Can jumping to done: and calling efc_scsi_sess_reg_complete() or
efc_scsi_del_initiator_complete() trigger a second call to efc_node_free(node)
and dereference a NULL node->nport if the node has already reached
__efc_node_shutdown()?

In efct_tgt_sess_may_register(node), the check tests node->nport &&
!node->req_free. Those fields only become NULL and true once the node has
already completed __efc_node_shutdown() and efc_node_free() (for instance, if
a subsequent PRLI cleared node->init via efc_process_prli_payload() before
__efc_d_initiate_shutdown() ran so efct_scsi_del_initiator() was skipped, or
via the unlocked window between setting EFCT_TGT_SESS_NONE and reaching done:
when efct_scsi_del_initiator() returns EFC_SCSI_CALL_COMPLETE).

Once efc_node_free(node) has run, node->nport is NULL and node->req_free
remains true while efct_lio_setup_session() still holds its kref on node.
When done: then calls efc_scsi_sess_reg_complete() or
efc_scsi_del_initiator_complete(), efc_node_post_event() runs on the node:

drivers/scsi/elx/libefc/efc_node.c:efc_node_post_event() {
	...
	node->evtdepth--;

	/*
	 * Free the node object if so requested,
	 * and we're at an event call depth of zero
	 */
	if (node->evtdepth == 0 && node->req_free)
		free_node = true;

	if (free_node)
		efc_node_free(node);
}

In efc_node_free(), if node->refound is set, node->nport is passed to
efc_node_find() before the !node->nport check:

drivers/scsi/elx/libefc/efc_node.c:efc_node_free() {
	...
	if (node->refound) {
		/*
		 * Save the name server node. We will send fake RSCN event at
		 * the end to handle ignored RSCN event during node deletion
		 */
		ns = efc_node_find(node->nport, FC_FID_DIR_SERV);
	}

	if (!node->nport) {
		efc_log_err(efc, "Node already Freed\n");
		return;
	}
	...
}

Should done: avoid posting completion events when node->req_free is already
set (or !node->nport)?

>  
>  int efct_scsi_new_initiator(struct efc *efc, struct efc_node *node)
>  {
>  	struct efct *efct = node->efc->base;
>  	struct efct_lio_wq_data *wq_data;
>  
> +	/* a repeated PRLI waits for the in-flight setup, it does not add one */
> +	if (node->tgt_sess_state != EFCT_TGT_SESS_NONE)
> +		return EFC_SCSI_CALL_ASYNC;

[ ... ]

> @@ -1287,17 +1364,22 @@ int efct_scsi_del_initiator(struct efc *efc, struct efc_node *node, int reason)
>  	if (reason == EFCT_SCSI_INITIATOR_MISSING)
>  		return EFC_SCSI_CALL_COMPLETE;
>  
> -	if (!tgt_node) {
> -		efc_log_err(efct, "tgt_node is NULL\n");
> -		return -EIO;
> +	if (node->tgt_sess_state == EFCT_TGT_SESS_SETUP) {
> +		/* the setup work unwinds it and completes the delete */
> +		node->tgt_sess_state = EFCT_TGT_SESS_DELETE_PENDING;
> +		return EFC_SCSI_CALL_ASYNC;
>  	}
>  
> +	if (node->tgt_sess_state != EFCT_TGT_SESS_REGISTERED || !tgt_node)
> +		return EFC_SCSI_CALL_COMPLETE;
> +
>  	wq_data = kzalloc_obj(*wq_data, GFP_ATOMIC);
>  	if (!wq_data)
>  		return -ENOMEM;
>  
>  	id = (u64) tgt_node->port_fc_id << 32 | tgt_node->node_fc_id;
>  	xa_erase(&efct->lookup, id);
> +	node->tgt_sess_state = EFCT_TGT_SESS_NONE;

[Severity: Medium]
What happens in efct_scsi_new_initiator() if node->tgt_sess_state is still
EFCT_TGT_SESS_REGISTERED when a new PRLI arrives?

Because efct_scsi_new_initiator() checks:

	if (node->tgt_sess_state != EFCT_TGT_SESS_NONE)
		return EFC_SCSI_CALL_ASYNC;

returning EFC_SCSI_CALL_ASYNC when node->tgt_sess_state is
EFCT_TGT_SESS_REGISTERED (rather than EFCT_TGT_SESS_SETUP) leaves the node
waiting in __efc_d_port_logged_in for EFC_EVT_NODE_SESS_REG_OK or
EFC_EVT_NODE_SESS_REG_FAIL when no efct_lio_setup_session() work item is in
flight to post it.

It looks like a node can reach efct_scsi_new_initiator() with
node->tgt_sess_state == EFCT_TGT_SESS_REGISTERED in two ways:

1. An initiator logs in and reaches EFCT_TGT_SESS_REGISTERED, then sends a
PRLI with FCP_SPPF_INIT_FCN clear in __efc_d_device_ready or
__efc_d_port_logged_in, which causes efc_process_prli_payload() to set
node->init = false. If the peer then sends PLOGI (triggering
EFC_EVT_SHUTDOWN_IMPLICIT_LOGO) or LOGO with pending frames,
__efc_d_initiate_shutdown() skips scsi_del_node because node->init is false,
and the node re-attaches and returns to __efc_d_port_logged_in with
node->tgt_sess_state still set to EFCT_TGT_SESS_REGISTERED.

2. During an implicit logout (or explicit logout with pending frames),
efct_scsi_del_initiator() fails its GFP_ATOMIC allocation of wq_data and
returns -ENOMEM before clearing node->tgt_sess_state. In
__efc_d_initiate_shutdown():

drivers/scsi/elx/libefc/efc_device.c:__efc_d_initiate_shutdown() {
	...
			if (node->nport->enable_tgt)
				rc = efc->tt.scsi_del_node(efc, node,
					EFC_SCSI_INITIATOR_DELETED);

			if (rc == EFC_SCSI_CALL_COMPLETE || rc < 0)
				efc_node_post_event(node,
					EFC_EVT_NODE_DEL_INI_COMPLETE, NULL);
	...
}

Because rc < 0 still posts EFC_EVT_NODE_DEL_INI_COMPLETE, the node completes
re-attachment and enters __efc_d_port_logged_in with node->tgt_sess_state
still set to EFCT_TGT_SESS_REGISTERED, where the next PRLI will hang the node
in efct_scsi_new_initiator().

>  
>  	wq_data->ptr = node;
>  	wq_data->efct = efct;

[ ... ]

>
> -- 
> 2.51.2
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021142818.2782104-1-yhlee@isslab.korea.ac.kr?part=1

Re: [PATCH v3] scsi: elx: efct: fix target session lifetime against node teardown

[Yehyeong Lee]

Please drop this patch. The node use after free it describes is real, but I
no longer think a correct fix fits in efct_lio.c alone, and I would rather
withdraw it than have a partial fix carry a Fixes tag. What follows is what
I found, in case it is useful. Line numbers are against f47d223627f8.

efct_scsi_new_initiator() hands a bare struct efc_node to
efct_lio_setup_session() through lio_wq and returns EFC_SCSI_CALL_ASYNC, so
the node state machine keeps running while the work is queued. A peer LOGO,
an FC link down, a fabric change or target teardown during that window frees
the node under the work, which then reads it:

  BUG: KASAN: slab-use-after-free in efct_lio_setup_session+0x934/0xa30
  Read of size 8 at addr ffff888002d538a0 by task kworker/u4:0/12

  CPU: 0 UID: 0 PID: 12 Comm: kworker/u4:0 Tainted: G                 N  7.3.0-00342-g66b1ae16db8c-dirty #7 PREEMPT(lazy)
  Tainted: [N]=TEST
  Workqueue: efct-kunit-wq efct_lio_setup_session
  Call Trace:
   <TASK>
   print_report+0xcd/0x620
   ? efct_lio_setup_session+0x934/0xa30
   ? __virt_addr_valid+0x208/0x3f0
   ? efct_lio_setup_session+0x934/0xa30
   kasan_report+0xfb/0x130
   ? efct_lio_setup_session+0x934/0xa30
   efct_lio_setup_session+0x934/0xa30
   ? __pfx_efct_lio_setup_session+0x10/0x10
   ? lock_acquire+0x100/0x2d0
   ? process_one_work+0x835/0x1c40
   ? process_one_work+0x735/0x1c40
   ? lock_release+0xc3/0x250
   process_one_work+0x88b/0x1c40
   ? __pfx_process_one_work+0x10/0x10
   ? lock_acquire+0x100/0x2d0
   ? worker_thread+0x2c7/0x11b0
   ? lock_is_held_type+0x84/0xf0
   ? __pfx_efct_lio_setup_session+0x10/0x10
   worker_thread+0x649/0x11b0
   ? __pfx_worker_thread+0x10/0x10
   kthread+0x3aa/0x550
   ? _raw_spin_unlock_irq+0x23/0x40
   ? __pfx_kthread+0x10/0x10
   ret_from_fork+0x493/0x940
   ? __pfx_ret_from_fork+0x10/0x10
   ? __switch_to+0x22/0xdc0
   ? __pfx_kthread+0x10/0x10
   ret_from_fork_asm+0x19/0x30
   </TASK>

  Allocated by task 33:
   kasan_save_stack+0x33/0x60
   kasan_save_track+0x14/0x30
   __kasan_kmalloc+0x8f/0xa0
   __kmalloc_noprof+0x23b/0x580
   mempool_alloc_noprof+0x143/0x230
   efct_kt_node_alloc.isra.0+0x38/0x4b0
   efct_kt_uaf_after_cb+0x7a/0x370
   kunit_try_run_case+0x1ca/0x4c0
   kunit_generic_run_threadfn_adapter+0x7b/0xe0
   kthread+0x3aa/0x550
   ret_from_fork+0x493/0x940
   ret_from_fork_asm+0x19/0x30

  Freed by task 12:
   kasan_save_stack+0x33/0x60
   kasan_save_track+0x14/0x30
   kasan_save_free_info+0x3b/0x60
   __kasan_slab_free+0x43/0x70
   kfree+0x119/0x490
   mempool_free+0xbc/0x100
   efc_node_free+0x311/0x4a0
   efct_lio_setup_session+0x6f1/0xa30
   process_one_work+0x88b/0x1c40
   worker_thread+0x649/0x11b0
   kthread+0x3aa/0x550
   ret_from_fork+0x493/0x940
   ret_from_fork_asm+0x19/0x30

  The buggy address belongs to the object at ffff888002d53800
   which belongs to the cache kmalloc-1k of size 1024
  The buggy address is located 160 bytes inside of
   freed 1024-byte region [ffff888002d53800, ffff888002d53c00)

Taking a reference on the node for the life of the work stops the slab use
after free, but it does not stop the driver faulting on the same ordering.
efc_node_free() clears node->nport before it drops the reference, so
efct_get_vport_tpg() and efct_session_cb() still dereference a NULL nport
unless they check it under efc->lock. I measured that: with the reference
held and nothing else changed, the same case moves from a
slab-use-after-free to a NULL pointer dereference in efct_session_cb().

The reason I stopped is one step below that. libefc does not guarantee the
backend is told to delete a session before it frees the node that owns it.
efc_node_free() makes no backend upcall at all, and scsi_del_node is reached
only from __efc_d_initiate_shutdown(), efc_device.c:225, :241, :255 and
:264, and from __efc_d_device_gone(), efc_device.c:1451 to :1475. The first
of those picks a branch from node->init and node->targ, and when both are
false it falls through to efc_device.c:293 and goes to cleanup with no upcall
at all. A remote initiator can arrange that, because
efc_process_prli_payload() takes both flags straight from the PRLI service
parameters at efc_device.c:366, and __efc_d_device_ready() accepts a further
PRLI and re-runs that at efc_device.c:1355, after a session is registered.
__efc_node_paused() is a second such path, setting node->req_free on
EFC_EVT_SHUTDOWN at efc_node.c:916 with no upcall either. On both, a
registered se_session and its efct->lookup entry outlive the struct efc_node
they belong to, and efct_node_find(), efct_unsol.c:22, hands that entry out
and takes a reference on it at efct_unsol.c:34 for every inbound FCP or BLS
frame.

Three smaller things nearby, which I am not proposing to fix either. The
node->refound block in efc_node_free() calls
efc_node_find(node->nport, FC_FID_DIR_SERV) at efc_node.c:137, above the
"Node already Freed" guard at efc_node.c:140 that would have caught a NULL
nport. __efc_d_device_ready() discards the scsi_new_node() return value at
efc_device.c:1312, so an EFC_SCSI_CALL_ASYNC there is never completed. And
the target branch of __efc_d_initiate_shutdown() tests only
rc == EFC_SCSI_CALL_COMPLETE at efc_device.c:244, :258 and :267, while the
initiator branch at :228 also accepts a negative return, so a negative
return in the target branch leaves the node in __efc_d_wait_del_node,
efc_device.c:82, with nothing left to wake it.

What I think it wants is both halves together, a reference held on the node
for the life of the setup work, and an invariant in libefc that a node for
which scsi_new_node() succeeded gets exactly one scsi_del_node() before
efc_node_free(), independent of init and targ. The second half changes the
node state machine contract, including how the two delete completion events
pair up and what a re-attach does, so that is your call and not something I
should decide from the backend. I have a KUnit harness that reproduces the
use after free, the session leak and the stale lookup entry, and I am happy
to write patches or test whatever shape you prefer.