Re: [PATCH v2 16/33] ibmvfc: add NVMe/FC Implicit Logout and Move Login support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The Move Login completion handler incorrectly schedules a SCSI-specific Process Login (PRLI) for NVMe targets, breaking the target initialization state machine.

Pre-existing issues:
- [High] Event completion handlers modify shared host and target state without holding `host_lock`, causing data races with the worker thread.
--

--- Patch [16]: [PATCH v2 16/33] ibmvfc: add NVMe/FC Implicit Logout and Move Login support ---
commit 3f8ab5b8122810e98caf8a4efbdac3e6f3574d85
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

ibmvfc: add NVMe/FC Implicit Logout and Move Login support

This commit adds protocol-specific opcodes and debug logging for implicit
logout and move login operations on NVMe/FC targets. It extends the existing
target login and migration flows to distinguish between SCSI and NVMe target
protocols.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 1d2a88e9fe941..66fcff7ac834b 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -4318,7 +4318,7 @@ static void ibmvfc_tgt_implicit_logout_done(struct ibmvfc_event *evt)
>  
>  	switch (status) {
>  	case IBMVFC_MAD_SUCCESS:
> -		tgt_dbg(tgt, "Implicit Logout succeeded\n");
> +		tgt_dbg(tgt, "%s Implicit Logout succeeded\n", proto_type[tgt->protocol]);

[Severity: High]
This is a pre-existing issue, but it appears that event completion handlers
like ibmvfc_tgt_implicit_logout_done() and ibmvfc_tgt_move_login_done()
modify shared host and target state without holding host_lock.

Can this cause data races with the worker thread?

The handlers are invoked locklessly during CRQ draining:

ibmvfc_drain_sub_crq() {
    ...
    spin_unlock_irqrestore(scrq->q_lock, flags);
    list_for_each_entry_safe(evt, temp, &evt_doneq, queue_list) {
        ...
        evt->done(evt);
    }
}

The evt->done() call executes ibmvfc_tgt_implicit_logout_done() which does:

    vhost->discovery_threads--;
    ibmvfc_set_tgt_action(tgt, IBMVFC_TGT_ACTION_NONE);

If vhost->discovery_threads is decremented locklessly while the worker thread
(ibmvfc_do_work) increments it under host_lock, could we lose decrements?

If we do, the count could artificially inflate and eventually hit the
disc_threads limit, permanently aborting future target discovery and login
operations.

[ ... ]
> @@ -4486,7 +4491,8 @@ static void ibmvfc_tgt_move_login_done(struct ibmvfc_event *evt)
>  	ibmvfc_set_tgt_action(tgt, IBMVFC_TGT_ACTION_NONE);
>  	switch (status) {
>  	case IBMVFC_MAD_SUCCESS:
> -		tgt_dbg(tgt, "Move Login succeeded for new scsi_id: %llX\n", tgt->new_scsi_id);
> +		tgt_dbg(tgt, "%s Move Login succeeded for new scsi_id: %llX\n",
> +			proto_type[tgt->protocol], tgt->new_scsi_id);

[Severity: High]
Does this code inadvertently schedule a SCSI-specific process login for NVMe
targets?

Just below this debug statement in ibmvfc_tgt_move_login_done(), a
successful move login unconditionally initializes the target with
ibmvfc_tgt_send_prli:

    case IBMVFC_MAD_SUCCESS:
        ...
        ibmvfc_init_tgt(tgt, ibmvfc_tgt_send_prli);
        break;

However, ibmvfc_tgt_send_prli() hardcodes the SCSI FCP type and opcode
without any fallback for NVMe targets:

    prli->common.opcode = cpu_to_be32(IBMVFC_PROCESS_LOGIN);
    ...
    prli->parms.type = IBMVFC_SCSI_FCP_TYPE;

Since this patch adds support for distinguishing NVMe/FC targets during move
login, will an NVMe target fail to complete the login sequence here when it
receives an invalid SCSI-specific Process Login MAD?

>  		tgt->ids.node_name = wwn_to_u64(rsp->service_parms.node_name);
>  		tgt->ids.port_name = wwn_to_u64(rsp->service_parms.port_name);
>  		tgt->scsi_id = tgt->new_scsi_id;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723000149.969416-1-tyreld@linux.ibm.com?part=16