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

[sashiko-bot@kernel.org]

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