Re: [PATCH v13 10/11] scsi: ibmvfc: register and use asynchronous sub CRQ for events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: ibmvfc: host state machine stall if ibmvfc_reg_sub_crqs() fails
- [High] scsi: ibmvfc: NULL pointer deref in channel setup when mq_enabled=0
--

commit 3dd02df8d62690f440ff03238afa2613397c578a
Author: Dave Marquardt <davemarq@linux.ibm.com>

scsi: ibmvfc: register and use asynchronous sub CRQ for events
    
Wire the async sub-CRQ into the sub-CRQ lifecycle so it is allocated,
registered, and freed alongside the SCSI channel queues. Updates
ibmvfc_set_login_info() to advertise related capabilities.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 76246002ac724..acc11f3a237e1 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
[ ... ]
> @@ -5821,6 +5828,7 @@ static void ibmvfc_channel_setup_done(struct ibmvfc_event *evt)
>  		for (i = 0; i < nvme->active_queues; i++)
>  			nvme->scrqs[i].vios_cookie =
>  				be64_to_cpu(setup->channel_handles[scsi->active_queues + i]);
> +		async->scrqs[0].vios_cookie = be64_to_cpu(setup->async_sub_crq_handle);

[Severity: High]
Can this cause a NULL pointer dereference when the module parameter mq_enabled
is 0?

If mq_enabled is 0, ibmvfc_init_sub_crqs() returns early and skips allocation,
leaving async->scrqs as NULL but vhost->do_enquiry as 1.

If the VIOS later advertises IBMVFC_CAN_SUPPORT_CHANNELS during NPIV login,
ibmvfc_npiv_login_done() sees do_enquiry is true and issues a channel enquiry.
When that completes, we reach this code and unconditionally dereference the
unallocated async->scrqs array.

[ ... ]
> @@ -5880,6 +5889,7 @@ static void ibmvfc_channel_setup(struct ibmvfc_host *vhost)
>  		for (i = 0; i < nvme_channels; i++)
>  			setup_buf->channel_handles[scsi_channels + i] =
>  				cpu_to_be64(nvme->scrqs[i].cookie);
> +		setup_buf->async_sub_crq_handle = cpu_to_be64(async->scrqs[0].cookie);
>  	}

[Severity: High]
Similarly, can this unconditional access to async->scrqs[0].cookie trigger a
NULL pointer dereference when mq_enabled is 0?

[ ... ]
> @@ -7023,17 +7039,38 @@ static int ibmvfc_alloc_channels(struct ibmvfc_host *vhost,
[ ... ]
>  static void ibmvfc_init_sub_crqs(struct ibmvfc_host *vhost)
>  {
>  	ENTER;
>  	if (!vhost->mq_enabled)
>  		return;
>  
> -	if (ibmvfc_alloc_channels(vhost, &vhost->scsi_scrqs)) {
> -		vhost->do_enquiry = 0;
> -		vhost->mq_enabled = 0;
> -		return;
> -	}
> +	/* register async_sub_crq channel */
> +	if (ibmvfc_alloc_channels(vhost, &vhost->async_scrqs))
> +		goto out;
> +
> +	ibmvfc_reg_sub_crqs(vhost, &vhost->async_scrqs);

[Severity: High]
Does this cause a host state machine stall if ibmvfc_reg_sub_crqs() fails?

If ibmvfc_reg_sub_crqs() fails internally, it clears vhost->do_enquiry but
leaves vhost->mq_enabled set to 1. Because mq_enabled remains true,
ibmvfc_set_login_info() continues to advertise IBMVFC_CAN_USE_CHANNELS to
the VIOS.

When the VIOS responds with IBMVFC_CAN_SUPPORT_CHANNELS,
ibmvfc_npiv_login_done() enters the multi-queue branch, but because
do_enquiry is now 0, it skips calling ibmvfc_channel_enquiry().

It also bypasses the fallback block that would normally set
IBMVFC_HOST_ACTION_QUERY and wake up vhost->work_wait_q:

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_npiv_login_done() {
    if (ibmvfc_check_caps(vhost, IBMVFC_CAN_SUPPORT_CHANNELS)) {
        if (ibmvfc_check_caps(vhost, IBMVFC_SUPPORT_SCSI))
            vhost->do_scsi_login = 1;
        if (ibmvfc_check_caps(vhost, IBMVFC_SUPPORT_NVMEOF))
            vhost->do_nvme_login = 1;
        if (vhost->do_enquiry)
            ibmvfc_channel_enquiry(vhost);
    } else {
        vhost->do_enquiry = 0;
        ibmvfc_set_host_action(vhost, IBMVFC_HOST_ACTION_QUERY);
        wake_up(&vhost->work_wait_q);
    }
}

Will this cause the initialization process to stall indefinitely in
IBMVFC_HOST_ACTION_INIT_WAIT?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007-ibmvfc-fpin-support-v13-0-b816abfa4fcc@linux.ibm.com?part=10