Re: [PATCH v18 12/13] scsi: ibmvfc: register and use asynchronous sub CRQ for events

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

sashiko-bot@kernel.org writes:

>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index 7b52401ae4d52..6e1ecf6891c71 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>
> [ ... ]
>
>> @@ -988,8 +989,17 @@ static int ibmvfc_reenable_crq_queue(struct ibmvfc_host *vhost)
>>  	spin_unlock(vhost->crq.q_lock);
>>  	spin_unlock_irqrestore(&vhost->host->host_lock, flags);
>>  
>> -	ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs);
>> -	ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs);
>> +	if (ibmvfc_reg_sub_crqs(vhost, &vhost->async_scrqs) ||
>> +	    ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs) ||
>> +	    ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs)) {
>> +		ibmvfc_dereg_sub_crqs(vhost, &vhost->async_scrqs);
>> +		ibmvfc_dereg_sub_crqs(vhost, &vhost->scsi_scrqs);
>> +		ibmvfc_dereg_sub_crqs(vhost, &vhost->nvme_scrqs);
>> +		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 rc;
>>  }
>
> [ ... ]
>
>> @@ -1044,8 +1055,17 @@ static int ibmvfc_reset_crq(struct ibmvfc_host *vhost)
>>  	spin_unlock(vhost->crq.q_lock);
>>  	spin_unlock_irqrestore(&vhost->host->host_lock, flags);
>>  
>> -	ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs);
>> -	ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs);
>> +	if (ibmvfc_reg_sub_crqs(vhost, &vhost->async_scrqs) ||
>> +	    ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs) ||
>> +	    ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs)) {
>> +		ibmvfc_dereg_sub_crqs(vhost, &vhost->async_scrqs);
>> +		ibmvfc_dereg_sub_crqs(vhost, &vhost->scsi_scrqs);
>> +		ibmvfc_dereg_sub_crqs(vhost, &vhost->nvme_scrqs);
>> +		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 rc;
>>  }
>
> [Severity: Medium]
> When ibmvfc_reg_sub_crqs() fails in ibmvfc_reenable_crq_queue() or
> ibmvfc_reset_crq(), all three channel sets are deregistered via
> ibmvfc_dereg_sub_crqs() and vhost->do_enquiry and vhost->mq_enabled are
> cleared to fall back to single-queue mode, but channels->active_queues is
> never reset to 0:
>
> drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_dereg_sub_crqs() {
> 	if (!channels->scrqs)
> 		return;
>
> 	for (i = 0; i < channels->max_queues; i++)
> 		ibmvfc_deregister_channel(vhost, channels, i);
> }
>
> Because clearing vhost->mq_enabled and vhost->do_enquiry causes subsequent
> NPIV login to skip ibmvfc_channel_setup_done(), active_queues remains
> non-zero on the deregistered channel sets.
>
> In drivers/scsi/ibmvscsi/ibmvfc-nvme.c, ibmvfc_nvme_create_queue() checks
> only active_queues:
>
> 	if (!vhost->nvme_scrqs.active_queues)
> 		return -ENODEV;
>
> Won't leaving active_queues non-zero after deregistering the sub-CRQs cause
> ibmvfc_nvme_create_queue() to bind NVMe queues to deregistered nvme_scrqs
> entries that no longer have hypervisor registrations or IRQ handlers?

This will be fixed in v19.

>> @@ -1586,9 +1606,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);
>> +		login_info->capabilities |= cpu_to_be64(IBMVFC_CAN_HANDLE_FPIN);
>> +		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);
>>  		}
>>  	}
>
> [Severity: Critical]
> Now that ibmvfc_set_login_info() advertises IBMVFC_USE_ASYNC_SUBQ and
> IBMVFC_CAN_HANDLE_FPIN, the VIOS can deliver FPIN events that are queued to
> ibmvfc_process_async_work() via
> ibmvfc_interrupt_async_subq()->ibmvfc_drain_async_subq()->ibmvfc_handle_async().
>
> In ibmvfc_process_async_work(), there appear to be two regressions when an
> FPIN event arrives with all-zero target identifiers (!scsi_id && !wwpn &&
> !node_name && !assoc_id), such as a host-scope IBMVFC_AE_FPIN_LINK_CONGESTED
> or IBMVFC_AE_FPIN_CONGESTION_CLEARED event:
>
> drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_process_async_work() {
> 	struct ibmvfc_async_sub_crq *subq = NULL;
> 	struct ibmvfc_async_work *aw;
> 	struct ibmvfc_async_crq *crq = NULL;
> 	struct ibmvfc_target *tgt;
> 	...
> 	if (aw->event.type == IBMVFC_ASYNC_CRQ_SUB) {
> 		subq = &aw->event.subq;
> 		...
> 	} else {
> 		crq = &aw->event.async_crq;
> 		...
> 	}
>
> 	if (!scsi_id && !wwpn && !node_name && !assoc_id) {
> 		if (fpin_status == IBMVFC_AE_FPIN_LINK_CONGESTED ||
> 		    fpin_status == IBMVFC_AE_FPIN_CONGESTION_CLEARED)
> 			fpin = ibmvfc_basic_fpin_to_desc(crq,
> 					cpu_to_be64(fc_host_port_name(vhost->host)), 0);
> 		goto send_fpin;
> 	}
> 	...
> send_fpin:
> 	...
> 	if (tgt)
> 		kref_put(&tgt->kref, ibmvfc_release_tgt);
> }
>
> First, tgt is declared without being initialized to NULL, and the
> (!scsi_id && !wwpn && !node_name && !assoc_id) branch jumps directly to
> send_fpin before tgt is assigned by ibmvfc_find_target(), so kref_put()
> dereferences an uninitialized stack pointer.
>
> Second, when aw->event.type == IBMVFC_ASYNC_CRQ_SUB, crq is NULL, but that
> same branch unconditionally calls ibmvfc_basic_fpin_to_desc(crq, ...)
> instead of checking crq vs subq and calling ibmvfc_full_fpin_to_desc(subq,
> ...), which immediately dereferences crq->fpin_status in
> ibmvfc_basic_fpin_to_desc().
>
> Can tgt be initialized to NULL and this branch dispatch between
> ibmvfc_basic_fpin_to_desc() and ibmvfc_full_fpin_to_desc() like the other
> call sites in ibmvfc_process_async_work()?

This will be fixed in patch 6 in v19.

>> @@ -5907,6 +5929,7 @@ static void ibmvfc_channel_setup_done(struct ibmvfc_event *evt)
>>  {
>>  	struct ibmvfc_host *vhost = evt->vhost;
>>  	struct ibmvfc_channel_setup *setup = vhost->channel_setup_buf;
>> +	struct ibmvfc_channels *async = &vhost->async_scrqs;
>>  	struct ibmvfc_channels *scsi = &vhost->scsi_scrqs;
>>  	struct ibmvfc_channels *nvme = &vhost->nvme_scrqs;
>>  	u32 mad_status = be16_to_cpu(evt->xfer_iu->channel_setup.common.status);
>> @@ -5920,8 +5943,11 @@ static void ibmvfc_channel_setup_done(struct ibmvfc_event *evt)
>>  		ibmvfc_dbg(vhost, "Channel Setup succeeded\n");
>>  		flags = be32_to_cpu(setup->flags);
>>  		vhost->do_enquiry = 0;
>> -		scsi->active_queues = be32_to_cpu(setup->num_scsi_subq_channels);
>> -		nvme->active_queues = be32_to_cpu(setup->num_nvme_subq_channels);
>> +		scsi->active_queues = min(be32_to_cpu(setup->num_scsi_subq_channels),
>> +					  scsi->max_queues);
>> +		nvme->active_queues = min(be32_to_cpu(setup->num_nvme_subq_channels),
>> +					  nvme->max_queues);
>> +		async->active_queues = (async->scrqs && setup->async_sub_crq_handle) ? 1 : 0;
>>  
>>  		if (flags & IBMVFC_CHANNELS_CANCELED) {
>>  			ibmvfc_dbg(vhost, "Channels Canceled\n");
>> @@ -5937,6 +5963,8 @@ static void ibmvfc_channel_setup_done(struct ibmvfc_event *evt)
>>  		for (i = 0; i < nvme->active_queues; i++)
>>  			nvme->scrqs[i].vios_cookie =
>>  				be64_to_cpu(setup->channel_handles[scsi->active_queues + i]);
>> +		for (i = 0; i < async->active_queues; i++)
>> +			async->scrqs[i].vios_cookie = be64_to_cpu(setup->async_sub_crq_handle);
>>  
>>  		ibmvfc_dbg(vhost, "Using %u SCSI channels\n",
>>  			   scsi->active_queues);
>> @@ -5971,6 +5999,7 @@ static void ibmvfc_channel_setup(struct ibmvfc_host *vhost)
>>  	struct ibmvfc_event *evt = ibmvfc_get_reserved_event(&vhost->crq);
>>  	struct ibmvfc_channels *scsi = &vhost->scsi_scrqs;
>>  	struct ibmvfc_channels *nvme = &vhost->nvme_scrqs;
>> +	struct ibmvfc_channels *async = &vhost->async_scrqs;
>>  	unsigned int scsi_channels =
>>  		min(scsi->desired_queues, vhost->max_vios_scsi_channels);
>>  	unsigned int nvme_channels =
>> @@ -5998,6 +6027,9 @@ static void ibmvfc_channel_setup(struct ibmvfc_host *vhost)
>>  				cpu_to_be64(nvme->scrqs[i].cookie);
>>  	}
>>  
>> +	if (async->scrqs)
>> +		setup_buf->async_sub_crq_handle = cpu_to_be64(async->scrqs[0].cookie);
>> +
>>  	ibmvfc_init_event(evt, ibmvfc_channel_setup_done, IBMVFC_MAD_FORMAT);
>>  	mad = &evt->iu.channel_setup;
>>  	memset(mad, 0, sizeof(*mad));
>
> [ ... ]
>
>> @@ -7191,9 +7223,74 @@ static void ibmvfc_release_channels(struct ibmvfc_host *vhost,
>>  	}
>>  }
>>  
>> +/**
>> + * ibmvfc_init_sub_crqs - Allocate and register all sub-CRQ channel sets
>> + * @vhost:	ibmvfc host struct
>> + *
>> + * Allocates and registers the async, SCSI, and (if nvme_enabled) NVMe
>> + * sub-CRQ channel sets in order. Returns immediately if mq_enabled is 0.
>> + *
>> + * On failure at any stage, previously registered and allocated channel sets
>> + * are deregistered and freed in reverse order, and vhost->mq_enabled and
>> + * vhost->do_enquiry are cleared under host_lock to prevent the host state
>> + * machine from stalling in IBMVFC_HOST_ACTION_INIT_WAIT.
>> + */
>> +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;
>> +
>> +	if (ibmvfc_reg_sub_crqs(vhost, &vhost->async_scrqs))
>> +		goto release_async_sub_crq;
>> +
>> +	if (ibmvfc_alloc_channels(vhost, &vhost->scsi_scrqs))
>> +		goto deregister_async_sub_crq;
>> +
>> +	if (ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs))
>> +		goto release_scsi_sub_crqs;
>> +
>> +	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 if (ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs)) {
>> +			ibmvfc_release_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);
>> +		}
>> +	}
>
> [Severity: High]
> In ibmvfc_probe(), vhost->nvme_scrqs.desired_queues and
> vhost->nvme_scrqs.max_queues are initialized to non-zero values even when
> vhost->nvme_enabled is 0:
>
> drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_probe() {
> 	vhost->nvme_enabled = mq_enabled ? nvme_enabled : 0;
> 	vhost->nvme_scrqs.desired_queues = min(max_nvme_queues, nr_nvme_channels);
> 	vhost->nvme_scrqs.max_queues = max_nvme_queues;
> }
>
> When vhost->nvme_enabled is 0, or when allocating or registering
> vhost->nvme_scrqs fails in ibmvfc_init_sub_crqs() after async_scrqs and
> scsi_scrqs succeed, vhost->nvme_scrqs.scrqs remains NULL while
> vhost->mq_enabled and vhost->do_enquiry remain 1 and desired_queues and
> max_queues remain non-zero.
>
> During channel enquiry and setup
> (ibmvfc_channel_enquiry_done()->ibmvfc_channel_setup()), if the VIOS reports
> num_nvme_subq_channels > 0, ibmvfc_channel_setup() computes a non-zero
> nvme_channels without checking vhost->nvme_enabled or nvme->scrqs:
>
> 	for (i = 0; i < nvme_channels; i++)
> 		setup_buf->channel_handles[scsi_channels + i] =
> 			cpu_to_be64(nvme->scrqs[i].cookie);
>
> Similarly, in ibmvfc_channel_setup_done(), nvme->active_queues is clamped to
> nvme->max_queues without checking whether nvme->scrqs is non-NULL, which
> then dereferences nvme->scrqs[i].vios_cookie in the loop.
>
> Should ibmvfc_channel_setup() and ibmvfc_channel_setup_done() guard the
> nvme->scrqs accesses (or zero desired_queues and max_queues when
> nvme_enabled is 0) in the same way async->scrqs is guarded?

Yes, this will be fixed in v19.

-Dave