Re: [PATCH v2] 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: 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