Re: [PATCH v3 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:
- [High] The dynamically allocated `channels->async_scrq` structure is leaked during channel release in `ibmvfc_release_channels()`.
- [High] Potential unconditional dereference of `async_scrq` in `ibmvfc_channel_setup_done()` when handling unexpected VIOS responses with MQ disabled.
--

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

ibmvfc: allocate asynchronous sub-queue

Allocate and set up the asynchronous sub-queue for asynchronous
events, as required for full and extended FPIN support.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> index 6dd1e726d9b94..804887fb56357 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
> @@ -5371,6 +5371,8 @@ static void ibmvfc_channel_setup_done(struct ibmvfc_event *evt)
>  			for (i = 0; i < active_queues; i++)
>  				scrqs->scrqs[i].vios_cookie =
>  					be64_to_cpu(setup->channel_handles[i]);
> +			scrqs->async_scrq->vios_cookie =
> +				be64_to_cpu(setup->async_subq_handle);

[Severity: High]
Does this code dereference a NULL pointer when MQ is disabled?

If vhost->mq_enabled is false, ibmvfc_alloc_channels() is skipped, leaving
scrqs->async_scrq as NULL. However, vhost->do_enquiry remains 1.

If the VIOS returns the IBMVFC_CAN_SUPPORT_CHANNELS capability,
ibmvfc_channel_setup() will be invoked with 0 channels and send
IBMVFC_CANCEL_CHANNELS. If the VIOS returns a success status but
unexpectedly omits the IBMVFC_CHANNELS_CANCELED flag in its response,
the driver falls through to the block containing this assignment.

Could this crash the kernel if the VIOS firmware responds unexpectedly?

[ ... ]

> @@ -6437,6 +6459,10 @@ static void ibmvfc_release_channels(struct ibmvfc_host *vhost,
>  
>  		kfree(channels->scrqs);
>  		channels->scrqs = NULL;
> +
> +		ibmvfc_free_queue(vhost, channels->async_scrq);
> +		channels->async_scrq = NULL;
> +

[Severity: High]
Does this code leak the memory allocated for async_scrq?

In ibmvfc_alloc_channels(), async_scrq is allocated with kzalloc_obj().
During teardown here, ibmvfc_free_queue() correctly releases internal queue
resources, but kfree() is missing before setting the pointer to NULL.

Should kfree(channels->async_scrq) be called here similar to how
kfree(channels->scrqs) is handled above?

>  		channels->active_queues = 0;
>  	}
>  }

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