Re: [PATCH v15 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: clearing mq_enabled in ibmvfc_reg_sub_crqs() skips deregistration
- [High] scsi: ibmvfc: ibmvfc_init_sub_crqs() ignores ibmvfc_reg_sub_crqs() failures
--

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

scsi: ibmvfc: register and use asynchronous sub CRQ for events

This commit wires the asynchronous sub-CRQ into the sub-CRQ lifecycle by
allocating, registering, and freeing vhost->async_scrqs alongside the SCSI
and NVMe channel queues. It also updates channel setup and login info to
handle the async sub-CRQ and multi-queue capabilities.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 7e012281d368c..0d603542fe348 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c

[ ... ]

> @@ -6977,6 +6995,7 @@ EXPORT_SYMBOL_IF_KUNIT(ibmvfc_deregister_channel);
>  static void ibmvfc_reg_sub_crqs(struct ibmvfc_host *vhost,
>  				struct ibmvfc_channels *channels)
>  {
> +	unsigned long flags;
>  	int i, j;
>  
>  	ENTER;
> @@ -6987,7 +7006,10 @@ static void ibmvfc_reg_sub_crqs(struct ibmvfc_host *vhost,
>  		if (ibmvfc_register_channel(vhost, channels, i)) {
>  			for (j = i; j > 0; j--)
>  				ibmvfc_deregister_channel(vhost, channels, j - 1);
> +			spin_lock_irqsave(&vhost->host->host_lock, flags);
>  			vhost->do_enquiry = 0;
> +			vhost->mq_enabled = 0;
> +			spin_unlock_irqrestore(&vhost->host->host_lock, flags);
>  			return;
>  		}
>  	}

[Severity: High]
Does clearing vhost->mq_enabled here prevent previously registered channel
sets from ever being deregistered?

In ibmvfc_init_sub_crqs(), ibmvfc_reenable_crq_queue(), and
ibmvfc_reset_crq(), channel sets are registered sequentially:

	ibmvfc_reg_sub_crqs(vhost, &vhost->async_scrqs);
	ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs);
	ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs);

If ibmvfc_reg_sub_crqs() succeeds for vhost->async_scrqs (or
vhost->scsi_scrqs), and then fails on a later call for vhost->scsi_scrqs or
vhost->nvme_scrqs, the error path in ibmvfc_reg_sub_crqs() only unwinds the
current channels set and sets vhost->mq_enabled = 0, leaving the earlier
channel sets registered with the hypervisor and their Linux IRQs active.

Because ibmvfc_dereg_sub_crqs() guards deregistration on vhost->mq_enabled:

	if (!vhost->mq_enabled || !channels->scrqs)
		return;

	for (i = 0; i < channels->max_queues; i++)
		ibmvfc_deregister_channel(vhost, channels, i);

all subsequent calls to ibmvfc_dereg_sub_crqs() in
ibmvfc_reenable_crq_queue(), ibmvfc_reset_crq(), and
ibmvfc_release_sub_crqs() will return early without calling
ibmvfc_deregister_channel().

When ibmvfc_release_sub_crqs() later runs during teardown,
ibmvfc_dereg_sub_crqs() is skipped while ibmvfc_release_channels()
unconditionally unmaps and frees each queue page (scrq->msgs.handle) and
kfree()s channels->scrqs. Doesn't this leak the hypervisor sub-CRQ
registrations and kernel IRQs, leaving the IRQ handler and hypervisor
referencing freed scrq slab memory and DMA pages?

[ ... ]

> @@ -7081,9 +7084,53 @@ static void ibmvfc_release_channels(struct ibmvfc_host *vhost,
>  	}
>  }
>  
> +static void ibmvfc_init_sub_crqs(struct ibmvfc_host *vhost)
> +{
> +	unsigned long flags;
> +
> +	ENTER;
> +	if (!vhost->mq_enabled)
> +		return;
> +
> +	/* register async_sub_crq channel */
> +	if (ibmvfc_alloc_channels(vhost, &vhost->async_scrqs))
> +		goto out;
> +
> +	ibmvfc_reg_sub_crqs(vhost, &vhost->async_scrqs);
> +
> +	if (ibmvfc_alloc_channels(vhost, &vhost->scsi_scrqs))
> +		goto deregister_async_sub_crq;
> +
> +	ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs);
> +
> +	if (vhost->nvme_enabled) {
> +		if (ibmvfc_alloc_channels(vhost, &vhost->nvme_scrqs)) {
> +			spin_lock_irqsave(&vhost->host->host_lock, flags);
> +			vhost->nvme_enabled = 0;
> +			spin_unlock_irqrestore(&vhost->host->host_lock, flags);
> +		} else
> +			ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs);
> +	}
> +
> +	LEAVE;
> +	return;
> +
> +deregister_async_sub_crq:
> +	ibmvfc_dereg_sub_crqs(vhost, &vhost->async_scrqs);
> +	ibmvfc_release_channels(vhost, &vhost->async_scrqs);
> +out:
> +	spin_lock_irqsave(&vhost->host->host_lock, flags);
> +	vhost->do_enquiry = 0;
> +	vhost->mq_enabled = 0;
> +	spin_unlock_irqrestore(&vhost->host->host_lock, flags);
> +}

[Severity: High]
The commit message states that in ibmvfc_init_sub_crqs(), failure of either
ibmvfc_alloc_channels() or ibmvfc_reg_sub_crqs() disables multi-queue and
aborts init. However, ibmvfc_reg_sub_crqs() returns void and its failure is
not checked here.

If ibmvfc_reg_sub_crqs(vhost, &vhost->async_scrqs) fails, should
ibmvfc_init_sub_crqs() release vhost->async_scrqs and return rather than
continuing to allocate vhost->scsi_scrqs and vhost->nvme_scrqs?

Similarly, if ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs) fails after
vhost->async_scrqs has been registered, shouldn't it release
vhost->scsi_scrqs and jump to deregister_async_sub_crq so vhost->async_scrqs
is deregistered and released instead of proceeding to allocate
vhost->nvme_scrqs?

[ ... ]

> +static void ibmvfc_release_sub_crqs(struct ibmvfc_host *vhost)
>  {
>  	ENTER;
> +	ibmvfc_dereg_sub_crqs(vhost, &vhost->async_scrqs);
> +	ibmvfc_release_channels(vhost, &vhost->async_scrqs);
> +
>  	if (!vhost->scsi_scrqs.scrqs)
>  		return;

[ ... ]

>
>
>
>

[]

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