Re: [PATCH v6 8/8] scsi: ibmvfc: handle extended FPIN events

[email protected] writes:

> [Severity: High]
> Will extended FPIN events be incorrectly discarded here because of how the
> target lookup is performed?
>
> In the earlier part of ibmvfc_process_async_work(), node_name is read from
> the subq structure:
>
> ibmvfc_process_async_work() {
>         ...
>         if (is_subq) {
>                 subq = &aw->crq.subq;
>                 scsi_id = 0;
>                 wwpn = subq->wwpn;
>                 node_name = (subq->flags & IBMVFC_ASYNC_ID_IS_ASSOC_ID) ? 0 : subq->id.node_name;
>         }
>         ...
>         tgt = ibmvfc_find_target(&vhost->scsi_scrqs.targets, scsi_id, wwpn, node_name);
>         ...
>
> Because the ibmvfc_async_subq_fpin structure overlaps with the id union in
> ibmvfc_async_sub_crq, reading subq->id.node_name for extended FPINs will
> extract garbage bytes from the fpin_data.event_threshold and event_data
> fields.
>
> Does this cause ibmvfc_find_target() to fail and drop valid events?

Yes, quite possibly. I've updated the code that sets node_name in
ibmvfc_process_async_work() and ibmvfc_handle_async() to take into
account that node_name isn't available in the extended FPIN case.

> [Severity: High]
> Can this lead to a NULL pointer dereference during driver removal?
>
> When ibmvfc_remove() is called, the fpin_workq is destroyed:
>
> ibmvfc_remove() {
>         ...
>         destroy_workqueue(vhost->fpin_workq);
>         vhost->fpin_workq = NULL;
>         ...
> }
>
> If an FPIN interrupt arrives before the CRQ queues are disabled, the
> interrupt handler unconditionally queues work to it:
>
> ibmvfc_handle_async() {
>         ...
>         case IBMVFC_AE_FPIN:
>                 aw = kzalloc(sizeof(struct ibmvfc_async_work), GFP_ATOMIC);
>                 if (aw) {
>                         ...
>                         queue_work(vhost->fpin_workq, &aw->async_work_s);
>                 ...
> }
>
> Should the interrupts be disabled prior to tearing down the workqueue, or
> should we check if fpin_workq is NULL before queuing work?

Yes, fixed in v8.

>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
>> index 444e6e6e1c390..ce938b93ecb45 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> [ ... ]
>> @@ -225,9 +226,118 @@ static void ibmvfc_full_fpin_test(struct kunit *test)
>>  			post[IBMVFC_AE_FPIN_CONGESTION_CLEARED]);
>>  }
>>  
>> +#define IBMVFC_TEST_FPIN_EXT(fs, ev, stat, crq) {		\
>> +	crq.valid = 0x80;					\
>> +	crq.flags = IBMVFC_ASYNC_IS_FPIN_EXT;			\
>> +	crq.link_state = IBMVFC_AE_LS_LINK_UP;			\
>> +	crq.fpin_status = (fs);					\
>> +	crq.event = cpu_to_be16(IBMVFC_AE_FPIN);		\
>> +	crq.wwpn = cpu_to_be64(tgt->wwpn);			\
>> +	crq.fpin_data.flags = IBMVFC_FPIN_EVENT_TYPE_VALID;	\
>> +	crq.fpin_data.event_type = cpu_to_be16((ev));		\
>> +	pre = READ_ONCE(tgt->rport->fpin_stats.stat);		\
>> +	ibmvfc_handle_async((struct ibmvfc_crq *)&crq, vhost, true);	\
>> +	msleep(1U);							\
>> +	post = READ_ONCE(tgt->rport->fpin_stats.stat);		\
>> +}
>
> [Severity: Medium]
> Is it expected that this macro only reads the pre and post statistics without
> asserting any changes? The KUNIT_EXPECT_* macros seem to be missing here
> to validate the update.

Yes, there is no validation. This is merely meant to exercise the
extended FPIN processing paths in the kernel and in multipathd.

> Additionally, could relying on msleep(1U) for synchronization lead to flaky
> test results when running under heavy load?

Fixed to use flush_workqueue().

>> +
>> +/**
>> + * ibmvfc_extended_fpin_test - unit test for extended FPIN events
>> + * @test: pointer to kunit structure
>> + *
>> + * Tests
>> + *
>> + * Return: void
>> + */
>> +static void ibmvfc_extended_fpin_test(struct kunit *test)
>> +{
> [ ... ]
>> +	for (fs = IBMVFC_AE_FPIN_LINK_CONGESTED; fs <= IBMVFC_AE_FPIN_CONGESTION_CLEARED; fs++) {
>> +		switch (fs) {
>> +		case IBMVFC_AE_FPIN_PORT_CLEARED:
>> +		case IBMVFC_AE_FPIN_CONGESTION_CLEARED:
>> +			crq[fs].valid = 0x80;
>> +			crq[fs].flags = IBMVFC_ASYNC_IS_FPIN_EXT;
>> +			crq[fs].link_state = IBMVFC_AE_LS_LINK_UP;
>> +			crq[fs].fpin_status = fs;
>> +			crq[fs].event = cpu_to_be16(IBMVFC_AE_FPIN);
>> +			crq[fs].wwpn = cpu_to_be64(tgt->wwpn);
>> +			crq[fs].fpin_data.flags = IBMVFC_FPIN_EVENT_TYPE_VALID;
>> +			crq[fs].fpin_data.event_type = cpu_to_be16(FPIN_CONGN_CLEAR);
>> +			pre = READ_ONCE(tgt->rport->fpin_stats.cn_clear);
>> +			ibmvfc_handle_async((struct ibmvfc_crq *)&crq[fs], vhost, true);
>> +			msleep(1U);
>> +			post = READ_ONCE(tgt->rport->fpin_stats.cn_clear);
>> +			break;
>
> [Severity: Medium]
> Similar to the macro above, this inline manual test logic is missing
> assertions to verify the stats actually changed. Does it need a KUNIT_EXPECT_*
> call here?

Same answer as above. Exercising the extended FPIN paths.

-Dave