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

[PATCH v2] 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+0xc44/0xde0
  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-gbe448252d3e7-dirty #4 PREEMPT(lazy)
  Tainted: [N]=TEST
  Workqueue: efct-kunit-wq efct_lio_setup_session
  Call Trace:
   <TASK>
   print_report+0xcd/0x620
   ? efct_lio_setup_session+0xc44/0xde0
   ? __virt_addr_valid+0x208/0x3f0
   ? efct_lio_setup_session+0xc44/0xde0
   kasan_report+0xfb/0x130
   ? efct_lio_setup_session+0xc44/0xde0
   efct_lio_setup_session+0xc44/0xde0
   ? __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+0xa4a/0xde0
   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. 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 reports completion instead of
-EIO, and the setup work unwinds the session it built instead of
publishing it. Once the session is registered the existing removal path
runs unchanged, and because the state transition and the lookup store
share one critical section, a store can no longer follow an erase.

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 reports completion rather than building
it.

An earlier version gated the work on node->nport and node->req_free
instead, which is not enough, because req_free is only set well after
efct_scsi_del_initiator() has already run and returned.

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 | 90 +++++++++++++++++++++++++++++---
 drivers/scsi/elx/efct/efct_lio.h |  9 ++++
 drivers/scsi/elx/libefc/efclib.h |  2 +
 3 files changed, 94 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/elx/efct/efct_lio.c b/drivers/scsi/elx/efct/efct_lio.c
index 67d686dd6fb33..634200c1a61b5 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,26 @@ 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;
 	int watermark;
 	int ini_count;
 	u64 id;
 
+	spin_lock_irqsave(&efc->lock, flags);
+	if (!efct_tgt_sess_may_register(node)) {
+		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 +1204,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 +1216,43 @@ 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);
+		spin_lock_irqsave(&efc->lock, flags);
+		if (!efct_tgt_sess_may_register(node)) {
+			node->tgt_sess_state = EFCT_TGT_SESS_NONE;
+			spin_unlock_irqrestore(&efc->lock, flags);
+			goto done;
+		}
+		node->tgt_sess_state = EFCT_TGT_SESS_NONE;
+		spin_unlock_irqrestore(&efc->lock, flags);
 		efc_scsi_sess_reg_complete(node, -EIO);
-		return;
+		goto done;
+	}
+
+	spin_lock_irqsave(&efc->lock, flags);
+	if (!efct_tgt_sess_may_register(node)) {
+		tgt_node = node->tgt_node;
+		node->tgt_node = NULL;
+		node->tgt_sess_state = EFCT_TGT_SESS_NONE;
+		spin_unlock_irqrestore(&efc->lock, flags);
+		/*
+		 * No kref_put(), because efct_scsi_del_initiator() already
+		 * reported completion. tgt_node never reached efct->lookup
+		 * and node->tgt_node is cleared above, so nothing else can
+		 * reach it and its count is the one kref_init() gave it.
+		 */
+		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 +1261,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);
 
@@ -1215,7 +1276,9 @@ static void efct_lio_setup_session(struct work_struct *work)
 			efct->tgt_efct.watermark_min : watermark;
 	atomic_set(&efct->tgt_efct.io_high_watermark, watermark);
 
+done:
 	kfree(wq_data);
+	kref_put(&node->ref, node->release);
 }
 
 int efct_scsi_new_initiator(struct efc *efc, struct efc_node *node)
@@ -1223,6 +1286,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 must not build a second session for one node */
+	if (node->tgt_sess_state != EFCT_TGT_SESS_NONE)
+		return EFC_SCSI_CALL_COMPLETE;
+
 	/*
 	 * Since LIO only supports initiator validation at thread level,
 	 * we are open minded and accept all callers.
@@ -1231,6 +1298,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 +1358,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) {
+		/* hand the session being built back to the setup work */
+		node->tgt_sess_state = EFCT_TGT_SESS_DELETE_PENDING;
+		return EFC_SCSI_CALL_COMPLETE;
 	}
 
+	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 v2] 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: premature PRLI completion in efct_scsi_new_initiator()
- [High] scsi: elx: efct: synchronous completion race in efct_scsi_del_initiator()
- [High] scsi: elx: efct: lock-drop race before efc_scsi_sess_reg_complete()
--

commit a9f642ffcfa8c9202c9520625d658419af962d69
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
efct_lio_setup_session() and introduces a tgt_sess_state state machine
under efc->lock to synchronize LIO target session setup with node
teardown.

> diff --git a/drivers/scsi/elx/efct/efct_lio.c b/drivers/scsi/elx/efct/efct_lio.c
> index 67d686dd6fb33..634200c1a61b5 100644
> --- a/drivers/scsi/elx/efct/efct_lio.c
> +++ b/drivers/scsi/elx/efct/efct_lio.c

[ ... ]

> @@ -1186,14 +1216,43 @@ 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);
> +		spin_lock_irqsave(&efc->lock, flags);
> +		if (!efct_tgt_sess_may_register(node)) {
> +			node->tgt_sess_state = EFCT_TGT_SESS_NONE;
> +			spin_unlock_irqrestore(&efc->lock, flags);
> +			goto done;
> +		}
> +		node->tgt_sess_state = EFCT_TGT_SESS_NONE;
> +		spin_unlock_irqrestore(&efc->lock, flags);
>  		efc_scsi_sess_reg_complete(node, -EIO);
> -		return;
> +		goto done;
> +	}
> +
> +	spin_lock_irqsave(&efc->lock, flags);
> +	if (!efct_tgt_sess_may_register(node)) {
> +		tgt_node = node->tgt_node;
> +		node->tgt_node = NULL;
> +		node->tgt_sess_state = EFCT_TGT_SESS_NONE;
> +		spin_unlock_irqrestore(&efc->lock, flags);
> +		/*
> +		 * No kref_put(), because efct_scsi_del_initiator() already
> +		 * reported completion. tgt_node never reached efct->lookup
> +		 * and node->tgt_node is cleared above, so nothing else can
> +		 * reach it and its count is the one kref_init() gave it.
> +		 */
> +		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 +1261,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]
Does dropping efc->lock in efct_lio_setup_session() before calling
efc_scsi_sess_reg_complete() leave a race window with concurrent node
teardown?

On the target_setup_session() error path, efct_lio_setup_session()
checks efct_tgt_sess_may_register(node), sets node->tgt_sess_state to
EFCT_TGT_SESS_NONE, and releases efc->lock before calling
efc_scsi_sess_reg_complete(node, -EIO), which then re-acquires
efc->lock to post EFC_EVT_NODE_SESS_REG_FAIL:

drivers/scsi/elx/libefc/efc_node.c:efc_scsi_sess_reg_complete() {
	...
	spin_lock_irqsave(&efc->lock, flags);
	/* Notify the node to resume */
	efc_node_post_event(node, evt, NULL);
	spin_unlock_irqrestore(&efc->lock, flags);
}

If __efc_d_initiate_shutdown() -> efct_scsi_del_initiator() runs on
another CPU during that unlocked window, efct_scsi_del_initiator() sees
node->tgt_sess_state == EFCT_TGT_SESS_NONE and returns
EFC_SCSI_CALL_COMPLETE immediately. The node can then finish
__efc_node_shutdown() and run efc_node_free(node) (which sets
node->nport = NULL while leaving node->req_free = true), or be reused
in place on an implicit or explicit logout, before
efc_scsi_sess_reg_complete() re-acquires efc->lock.

When efc_scsi_sess_reg_complete() then calls efc_node_post_event() on
an already-freed node, efc_node_post_event() sees:

drivers/scsi/elx/libefc/efc_node.c:efc_node_post_event() {
	...
	if (node->evtdepth == 0 && node->req_free)
		free_node = true;

	if (free_node)
		efc_node_free(node);
}

Calling efc_node_free() a second time when node->refound is set will
dereference the NULL node->nport inside efc_node_find(node->nport,
FC_FID_DIR_SERV). A similar lock drop occurs on the success path
between setting node->tgt_sess_state = EFCT_TGT_SESS_REGISTERED and
calling efc_scsi_sess_reg_complete(node, 0). Should the registration
completion event be posted before releasing efc->lock?

[ ... ]

> @@ -1223,6 +1286,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 must not build a second session for one node */
> +	if (node->tgt_sess_state != EFCT_TGT_SESS_NONE)
> +		return EFC_SCSI_CALL_COMPLETE;

[Severity: High]
What happens if a remote initiator sends a second PRLI while the first
efct_lio_setup_session() work item is still in flight (with
node->tgt_sess_state == EFCT_TGT_SESS_SETUP)?

In drivers/scsi/elx/libefc/efc_device.c, efc_d_send_prli_rsp() treats
EFC_SCSI_CALL_COMPLETE as synchronous session registration completion
and immediately posts EFC_EVT_NODE_SESS_REG_OK:

drivers/scsi/elx/libefc/efc_device.c:efc_d_send_prli_rsp() {
	...
	if (rc == EFC_SCSI_CALL_COMPLETE)
		efc_node_post_event(node, EFC_EVT_NODE_SESS_REG_OK, NULL);
}

In __efc_d_port_logged_in(), handling EFC_EVT_NODE_SESS_REG_OK sends
the PRLI LS_ACC and moves the node to __efc_d_device_ready:

drivers/scsi/elx/libefc/efc_device.c:__efc_d_port_logged_in() {
	...
	case EFC_EVT_NODE_SESS_REG_OK:
		if (node->send_ls_acc == EFC_NODE_SEND_LS_ACC_PRLI)
			efc_send_prli_acc(node, node->ls_acc_oxid);

		node->send_ls_acc = EFC_NODE_SEND_LS_ACC_NONE;
		efc_node_transition(node, __efc_d_device_ready, NULL);
		break;
}

Because efct_lio_setup_session() has not finished target_setup_session()
yet, the PRLI LS_ACC is sent and the node enters __efc_d_device_ready
before ConfigFS NodeACLs are checked in
core_tpg_check_initiator_node_acl() and before tgt_node is stored in
efct->lookup. Any FCP commands arriving before xa_store(&efct->lookup,
...) will be dropped by efct_dispatch_frame(), and if
target_setup_session() later fails (for example with -EACCES or
-ENOMEM), the PRLI LS_ACC has already been sent instead of LS_RJT and
__efc_d_device_ready ignores the subsequent EFC_EVT_NODE_SESS_REG_FAIL
event, leaving the node ready with no LIO session.

Should efct_scsi_new_initiator() return EFC_SCSI_CALL_ASYNC when
node->tgt_sess_state == EFCT_TGT_SESS_SETUP so the node waits for the
in-flight setup work to finish?

[ ... ]

> @@ -1287,17 +1358,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) {
> +		/* hand the session being built back to the setup work */
> +		node->tgt_sess_state = EFCT_TGT_SESS_DELETE_PENDING;
> +		return EFC_SCSI_CALL_COMPLETE;
>  	}

[Severity: High]
Could returning EFC_SCSI_CALL_COMPLETE here while
efct_lio_setup_session() is still running on lio_wq cause races with
node re-login or teardown?

When __efc_d_initiate_shutdown() calls efct_scsi_del_initiator() and
receives EFC_SCSI_CALL_COMPLETE, it immediately posts
EFC_EVT_NODE_DEL_INI_COMPLETE and advances the node to
__efc_node_shutdown() while efct_lio_setup_session() is still executing:

1. On an implicit logout (EFC_NODE_SHUTDOWN_IMPLICIT_LOGO, such as a
new PLOGI) or an explicit logout with pending frames,
efc_node_handle_implicit_logo() or efc_node_handle_explicit_logo() in
drivers/scsi/elx/libefc/efc_node.c sets node->req_free = false and
re-attaches the same struct efc_node in place:

drivers/scsi/elx/libefc/efc_node.c:efc_node_handle_implicit_logo() {
	...
	/* Re-attach node with the same HW node resources */
	node->req_free = false;
	rc = efc_node_attach(node);
	efc_node_transition(node, __efc_d_wait_node_attach, NULL);
	...
}

If the new PRLI arrives before the previous efct_lio_setup_session()
work item has unwound EFCT_TGT_SESS_DELETE_PENDING,
efct_scsi_new_initiator() sees node->tgt_sess_state !=
EFCT_TGT_SESS_NONE and returns EFC_SCSI_CALL_COMPLETE without queueing
a new session setup. The node transitions to __efc_d_device_ready, and
the old work item then unwinds and resets node->tgt_node = NULL and
node->tgt_sess_state = EFCT_TGT_SESS_NONE, leaving the ready node
without a LIO session.

2. On normal node teardown, __efc_node_shutdown() sets
node->req_free = true and efc_node_post_event() calls
efc_node_free(node) immediately while efct_lio_setup_session() may
still be inside target_setup_session(). After efct_session_cb() drops
efc->lock, transport_register_session() briefly publishes se_sess
(with se_sess->fabric_sess_ptr = node) onto se_nacl->acl_sess_list
before efct_lio_setup_session() removes it. If a concurrent LIO
session teardown (such as target_shutdown_sessions() ->
efct_lio_close_session()) runs during that window, it calls
efc_node_post_shutdown(node, NULL) -> efc_node_post_event() on the
already-freed node, which sees node->evtdepth == 0 && node->req_free
and calls efc_node_free(node) a second time, dereferencing NULL
node->nport in efc_node_find(node->nport, FC_FID_DIR_SERV) when
node->refound is set:

drivers/scsi/elx/libefc/efc_node.c:efc_node_free() {
	...
	if (node->refound) {
		ns = efc_node_find(node->nport, FC_FID_DIR_SERV);
	}

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

Would it be safer for efct_scsi_del_initiator() to return
EFC_SCSI_CALL_ASYNC when setting EFCT_TGT_SESS_DELETE_PENDING and have
efct_lio_setup_session() call efc_scsi_del_initiator_complete() once
the abort and unwind have completed?

[ ... ]

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

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

[Yehyeong Lee]

Thanks for the review. Two of the three points are right, and v3 fixes
them.

Both came from the backend reporting a synchronous completion while the
setup work was still in flight. efct_scsi_new_initiator() returned
EFC_SCSI_CALL_COMPLETE for a duplicate PRLI, and efct_scsi_del_initiator()
returned it when it handed the in-flight setup to teardown, so
efc_d_send_prli_rsp() and __efc_d_initiate_shutdown() advanced the node
before the work had registered or unwound the session. v3 returns
EFC_SCSI_CALL_ASYNC in both cases and lets the setup work post the one
completion it owes, through efc_scsi_del_initiator_complete() or
efc_scsi_sess_reg_complete().

The third, the window around efc_scsi_sess_reg_complete(), stays memory
safe because the work now holds a node reference across it. The second
efc_node_free() it describes is a pre-existing path in efc_node_free()
that this patch does not change.

v3 is at
https://lore.kernel.org/r/20261021142818.2782104-1-yhlee@isslab.korea.ac.kr