Re: [PATCH] scsi: elx: efct: hold a node reference across asynchronous session setup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] scsi: elx: efct: incomplete shutdown check in efct_node_registrable()
- [Critical] scsi: elx: efct: xa_erase() race with xa_store() in efct_lio_setup_session()
- [High] scsi: elx: efct: session leak on post-setup bailout in efct_lio_setup_session()
- [High] scsi: elx: efct: TOCTOU race before efc_scsi_sess_reg_complete()
--

commit 9faa68f52085fb309e7aece4f85020881ca6843a
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: elx: efct: hold a node reference across asynchronous session setup

This commit takes a kref on struct efc_node while efct_lio_setup_session() is
queued on lio_wq and adds efct_node_registrable() checks under efc->lock to
bail out if the node has shut down during asynchronous session setup.

> diff --git a/drivers/scsi/elx/efct/efct_lio.c b/drivers/scsi/elx/efct/efct_lio.c
> index 67d686dd6fb33..add939366bc6a 100644
> --- a/drivers/scsi/elx/efct/efct_lio.c
> +++ b/drivers/scsi/elx/efct/efct_lio.c
> @@ -1062,17 +1062,42 @@ _efct_tgt_node_free(struct kref *arg)
>  	kfree(tgt_node);
>  }
>  
> +/*
> + * Report whether the node state machine may still have a session registered
> + * for @node. The caller must hold efc->lock, which is also held by the paths
> + * that shut a node down.
> + */
> +static bool efct_node_registrable(struct efc_node *node)
> +{
> +	return 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 have entered
> +	 * shutdown since it was called. Refuse the session in that case:
> +	 * efct_scsi_del_initiator() only queues the removal work once
> +	 * node->tgt_node is set, so either it sees the session below or
> +	 * there is no session to remove at all.
> +	 */
> +	spin_lock_irqsave(&node->efc->lock, flags);
> +	if (!efct_node_registrable(node)) {
> +		spin_unlock_irqrestore(&node->efc->lock, flags);
> +		kfree(tgt_node);
> +		return -ENODEV;
> +	}

[Severity: Critical]
Is checking node->nport and !node->req_free in efct_node_registrable()
sufficient to detect a node that has already entered shutdown?

When a remote initiator logs in and efc_d_send_prli_rsp() queues
efct_lio_setup_session(), a shutdown event arriving before efct_session_cb()
sets node->tgt_node runs __efc_d_initiate_shutdown() in
drivers/scsi/elx/libefc/efc_device.c:

    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 node->tgt_node is still NULL, efct_scsi_del_initiator() returns
-EIO without queuing efct_lio_remove_session().

In drivers/scsi/elx/libefc/efc_node.c, efc_node_initiate_cleanup() ->
efc_node_check_els_quiesced() then checks whether the hardware node is still
attached or ELS IOs are active:

    } else {
        /*
         * hw node detach hasn't completed,
         * transition and wait
         */
        node_printf(node, "HW node still attached\n");
        efc_node_transition(node, __efc_node_wait_node_free,
                            NULL);
    }

While the node waits in __efc_node_wait_node_free (or
__efc_node_wait_els_shutdown, or __efc_d_wait_del_node for an
initiator+target node), node->nport is still non-NULL (cleared only in
efc_node_free()) and node->req_free is still false (set only on entry to
__efc_node_shutdown()). Similarly, efc_node_handle_implicit_logo() and
efc_node_handle_explicit_logo() reset node->req_free = false while keeping
node->nport valid:

    /* 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);

In all of these states, efct_node_registrable(node) returns true even though
efct_scsi_del_initiator() has already run for that login. When the detach
later completes and the node reaches efc_node_free() without calling
efct_scsi_del_initiator() again, won't this leak se_sess and tgt_node and
leave dangling pointers to the freed struct efc_node in efct->lookup and
se_sess->fabric_sess_ptr?

> +
>  	kref_init(&tgt_node->ref);
>  	tgt_node->release = _efct_tgt_node_free;
>  
> @@ -1086,6 +1111,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 +1183,25 @@ 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_node_registrable(node)) {
> +		spin_unlock_irqrestore(&efc->lock, flags);
> +		efc_log_debug(efct, "node %p shut down before session setup\n",
> +			      node);
> +		goto done;
> +	}
> +
>  	/* Check to see if it's belongs to vport,
>  	 * if not get physical port
>  	 */
> @@ -1176,8 +1212,9 @@ static void efct_lio_setup_session(struct work_struct *work)
>  		tpg = efct->tgt_efct.tpg;
>  		se_tpg = &tpg->tpg;
>  	} else {
> +		spin_unlock_irqrestore(&efc->lock, flags);
>  		efc_log_err(efct, "failed to init session\n");
> -		return;
> +		goto done;
>  	}
>  
>  	/*
> @@ -1186,14 +1223,28 @@ 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_node_registrable(node)) {
> +			spin_unlock_irqrestore(&efc->lock, flags);
> +			goto done;
> +		}
> +		spin_unlock_irqrestore(&efc->lock, flags);
>  		efc_scsi_sess_reg_complete(node, -EIO);
> -		return;
> +		goto done;
>  	}

[Severity: High]
Can dropping efc->lock before calling efc_scsi_sess_reg_complete() here (and
before efc_scsi_sess_reg_complete(node, 0) below) race with node teardown?

In drivers/scsi/elx/libefc/efc_node.c, efc_scsi_sess_reg_complete()
re-acquires efc->lock and posts an event to the node:

    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 the node finishes shutting down on another CPU in the window after
efct_lio_setup_session() releases efc->lock and before
efc_scsi_sess_reg_complete() re-acquires it, __efc_node_shutdown() sets
node->req_free = true and efc_node_post_event() calls efc_node_free(node),
which clears node->nport = NULL while leaving node->req_free set to true.

When efc_scsi_sess_reg_complete() then calls efc_node_post_event(),
node->evtdepth returns to 0 with node->req_free still true:

    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, efc_node_find(node->nport,
FC_FID_DIR_SERV) runs before the !node->nport check:

    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;
    }

Wouldn't calling efc_node_free() a second time dereference a NULL
node->nport inside efc_node_find()?

> +
> +	spin_lock_irqsave(&efc->lock, flags);
> +	if (!efct_node_registrable(node)) {
> +		spin_unlock_irqrestore(&efc->lock, flags);
> +		efc_log_debug(efct, "node %p shut down during session setup\n",
> +			      node);
> +		goto done;
> +	}

[Severity: High]
What cleans up se_sess and tgt_node when efct_lio_setup_session() takes this
goto done path after target_setup_session() has already succeeded?

By the time target_setup_session() returns a valid se_sess,
efct_session_cb() has allocated tgt_node and set node->tgt_node = tgt_node,
and target_setup_session() has registered se_sess with
se_sess->fabric_sess_ptr = node.

If efct_node_registrable(node) is false here (for instance, if the node
completed __efc_node_shutdown() and efc_node_free() while
transport_register_session() was running, or if efct_scsi_del_initiator()
ran after efct_session_cb() unlocked efc->lock and failed its GFP_ATOMIC
allocation of wq_data), efct_lio_remove_session() will not be queued for
this session.

Jumping directly to done frees wq_data and drops node->ref without calling
target_remove_session(se_sess) or freeing tgt_node. Doesn't this leak
se_sess and tgt_node, leave se_sess->fabric_sess_ptr pointing to a freed
struct efc_node, and leave efct->tgt_efct.initiator_count unbalanced if
efct_scsi_del_initiator() already decremented it?

>  
>  	tgt_node = node->tgt_node;
>  	id = (u64) tgt_node->port_fc_id << 32 | tgt_node->node_fc_id;
>  
>  	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");
> +	spin_unlock_irqrestore(&efc->lock, flags);

[Severity: Critical]
Could xa_store() here insert tgt_node into efct->lookup after
efct_scsi_del_initiator() has already called xa_erase()?

Inside target_setup_session(), efct_session_cb() sets
node->tgt_node = tgt_node and releases node->efc->lock before
target_setup_session() returns to efct_lio_setup_session().

If the node enters shutdown during that unlocked window,
__efc_d_initiate_shutdown() -> efct_scsi_del_initiator() sees
node->tgt_node != NULL and erases id from efct->lookup before queuing
efct_lio_remove_session():

    id = (u64) tgt_node->port_fc_id << 32 | tgt_node->node_fc_id;
    xa_erase(&efct->lookup, id);

    wq_data->ptr = node;
    wq_data->efct = efct;
    INIT_WORK(&wq_data->work, efct_lio_remove_session);
    queue_work(lio_wq, &wq_data->work);

Because efct_scsi_del_initiator() returns EFC_SCSI_CALL_ASYNC, the node
waits in __efc_d_wait_del_node for efct_lio_remove_session() to run on
lio_wq behind efct_lio_setup_session(). In __efc_d_wait_del_node,
node->nport is non-NULL and node->req_free is false, so
efct_node_registrable(node) still evaluates to true.

efct_lio_setup_session() then runs xa_store(&efct->lookup, id, tgt_node,
GFP_ATOMIC) after the xa_erase() in efct_scsi_del_initiator() has already
happened. When efct_lio_remove_session() subsequently frees tgt_node
without erasing id again, won't efct->lookup retain a dangling tgt_node
pointer that efct_node_find() in drivers/scsi/elx/efct/efct_unsol.c can
dereference via kref_get(&node->ref)?

[ ... ]

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