Re: [PATCH v4 5/8] ibmvfc: allocate asynchronous sub-queue

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly allocated asynchronous sub-queue (`async_scrq`) is never registered with the hypervisor, leaving its cookie uninitialized and the queue non-functional.

Pre-existing issues:
- [High] Unprotected dereference of queue pointers in `ibmvfc_channel_setup()` leads to a kernel panic if MQ is disabled or if queue allocation fails.
--

--- Patch [5]: [PATCH v4 5/8] ibmvfc: allocate asynchronous sub-queue ---
commit d243c6fd5b265f83cb24c3737fb1dfe6fd989a93
Author: Dave Marquardt <davemarq@linux.ibm.com>

ibmvfc: allocate asynchronous sub-queue

This commit allocates and initializes an asynchronous sub-queue required for
receiving full and extended FPIN events from the VIOS.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c

[ ... ]

> @@ -5425,6 +5428,7 @@ static void ibmvfc_channel_setup(struct ibmvfc_host *vhost)
>  		setup_buf->num_scsi_subq_channels = cpu_to_be32(num_channels);
>  		for (i = 0; i < num_channels; i++)
>  			setup_buf->channel_handles[i] = cpu_to_be64(scrqs->scrqs[i].cookie);
> +		setup_buf->async_subq_handle = cpu_to_be64(scrqs->async_scrq->cookie);

[Severity: High]
This is a pre-existing issue, but does this code unconditionally dereference
the queue pointers?

If MQ is disabled or if queue allocation fails in ibmvfc_alloc_channels(),
scrqs->scrqs and scrqs->async_scrq are left as NULL.

If the VIOS subsequently indicates channel support in its capabilities,
ibmvfc_channel_setup() is called and these unallocated pointers are
dereferenced here, which would result in a kernel panic.

Could we add a check to ensure the queues were successfully allocated before
dereferencing them during channel setup?

[ ... ]

> @@ -6392,6 +6396,17 @@ static int ibmvfc_alloc_channels(struct ibmvfc_host *vhost,
>  	if (!channels->scrqs)
>  		return -ENOMEM;
>  
> +	channels->async_scrq = kzalloc_obj(*channels->async_scrq, GFP_KERNEL);
> +	if (!channels->async_scrq) {
> +		rc = -ENOMEM;
> +		goto free_scrqs;
> +	}
> +
> +	rc = ibmvfc_alloc_queue(vhost, channels->async_scrq,
> +				IBMVFC_SUB_CRQ_FMT);
> +	if (rc)
> +		goto free_async;

[Severity: High]
Is this missing a step to register the asynchronous sub-queue with the
hypervisor?

The queue is allocated here and mapped in memory, but it doesn't appear to
be registered via h_reg_sub_crq() anywhere.

Because it is never registered, its cookie remains 0, meaning an
uninitialized handle is sent to the VIOS during ibmvfc_channel_setup(),
and no hardware IRQ is mapped or requested, which renders the async queue
non-functional.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-ibmvfc-fpin-support-v4-0-ef031ac19520@linux.ibm.com?part=5