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

[Dave Marquardt <davemarq@linux.ibm.com>]

sashiko-bot@kernel.org writes:

> 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?
>
> [ ... ]

Yes, this is broken. I will remove the check for mq_enabled, as the
check for channels->scrq is all that is needed here.

>> @@ -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?

This code will be reworked for ibmvfc_reg_sub_crqs() returns int and
ibmvfc_init_sub_crqs() will take appropriate clean up actions when
ibmvfc_reg_sub_crqs() returns non-zero.

-Dave