Re: [PATCH v5 6/7] scsi: ibmvfc: register and use asynchronous sub-queue for events

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The driver enables IBMVFC_USE_ASYNC_SUBQ but fails to pass the async sub-queue handle to the VIOS during channel setup, rendering the feature non-functional.
- [High] The driver incorrectly allocates multiple asynchronous sub-queues (one for SCSI, one for NVMe) instead of a single host-wide queue.
- [High] Manual destruction of a devm-managed workqueue in the probe error path leads to a double-free when devres unwinds.
- [High] Pending FPIN asynchronous works can execute after the host struct is freed during device removal, resulting in a use-after-free.
- [Low] The ibmvfc_handle_async function attempts to dereference a void * parameter, resulting in a severe compiler error.
--

--- Patch [6]: [PATCH v5 6/7] scsi: ibmvfc: register and use asynchronous sub-queue for events ---
commit 75a27ff4a34478f19a67ddd8e7759ecc0644a5e2
Author: Dave Marquardt <[email protected]>

scsi: ibmvfc: register and use asynchronous sub-queue for events

Complete async sub-queue integration by setting up interrupt handling,
registering the queue as a channel, and enabling its use during NPIV
login.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 97856a609bde4..b6ac87c2042b5 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -1583,9 +1583,11 @@ static void ibmvfc_set_login_info(struct ibmvfc_host *vhost)
>  
>  	if (vhost->mq_enabled || vhost->using_channels) {
>  		login_info->capabilities |= cpu_to_be64(IBMVFC_CAN_USE_CHANNELS);
> +		login_info->capabilities |= cpu_to_be64(IBMVFC_USE_ASYNC_SUBQ);

[Severity: High]
Does enabling IBMVFC_USE_ASYNC_SUBQ here work as intended? Looking at
ibmvfc_channel_setup(), setup_buf->async_sub_crq_handle is never assigned after
memset(setup_buf, 0, sizeof(*setup_buf)), meaning the hypervisor never
learns the async sub-queue cookie.

Also, does allocating channels->async_scrq per channel collection (both SCSI
and NVMe separately) in ibmvfc_alloc_channels() cause issues, given the MAD
protocol only supports a single host-wide async sub-queue handle?

> +		login_info->capabilities |= cpu_to_be64(IBMVFC_CAN_HANDLE_FPIN);

[Severity: High]
Since this patch enables FPIN handling, could there be a use-after-free during
device removal? Pending FPIN asynchronous works (ibmvfc_process_async_work) can
execute after the host struct is freed during ibmvfc_remove() via
scsi_host_put(vhost->host).

Additionally, could the error path in ibmvfc_probe() cause a double-free? It
calls destroy_workqueue(vhost->fpin_workq) manually, but if the workqueue was
allocated with devm_alloc_workqueue, devres will attempt to destroy it again
during error unwinding.

> +		login_info->capabilities |= cpu_to_be64(IBMVFC_YES_SCSI);
>  		if (vhost->nvme_enabled) {
>  			login_info->capabilities |= cpu_to_be64(IBMVFC_YES_NVMEOF);
> -			login_info->capabilities |= cpu_to_be64(IBMVFC_YES_SCSI);
>  			login_info->capabilities |= cpu_to_be64(IBMVFC_CAN_USE_WWPN_ALL);
>  		}
>  	}

[ ... ]

> @@ -4356,6 +4358,52 @@ static struct ibmvfc_crq *ibmvfc_next_scrq(struct ibmvfc_queue *scrq)
>  	return crq;
>  }
>  
> +static void ibmvfc_drain_async_subq(struct ibmvfc_queue *scrq)
> +{
> +	struct ibmvfc_host *vhost = scrq->vhost;
> +	struct ibmvfc_crq *crq;
> +	unsigned long flags;
> +	int done = 0;
> +
> +	spin_lock_irqsave(vhost->host->host_lock, flags);
> +	spin_lock(scrq->q_lock);
> +	while (!done) {
> +		while ((crq = ibmvfc_next_scrq(scrq)) != NULL) {
> +			ibmvfc_handle_async(crq, scrq->vhost, true);

[Severity: Low]
Does this code compile successfully? ibmvfc_handle_async() takes a void pointer
parameter, but looking at its implementation, it attempts direct member access
like crq->scsi_id, which violates C syntax for void pointers.

> +			crq->valid = 0;
> +			wmb();	/* complete write */
> +		}
> +
> +		ibmvfc_toggle_scrq_irq(scrq, 1);
> +		crq = ibmvfc_next_scrq(scrq);
> +		if (crq != NULL) {
> +			ibmvfc_toggle_scrq_irq(scrq, 0);
> +			ibmvfc_handle_async(crq, scrq->vhost, true);
> +			crq->valid = 0;
> +			wmb();	/* complete write */
> +		} else
> +			done = 1;
> +	}
> +	spin_unlock(scrq->q_lock);
> +	spin_unlock_irqrestore(vhost->host->host_lock, flags);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6