Re: [PATCH v6 4/8] scsi: ibmvfc: extend async event handlers to handle async sub queue events

[email protected] writes:

> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] Double clear of `valid` in the CRQ ring buffer introduces a race condition that can clobber subsequent hardware events.
> - [High] Missing check for `IBMVFC_ASYNC_ID_IS_ASSOC_ID` causes `assoc_id` to be incorrectly treated as a WWNN, preventing target lookup and dropping events.
> - [Medium] KUnit tests use `msleep()` to wait for asynchronous workqueue execution, leading to flaky test failures.
> - [Low] Dereferencing a `void *` pointer in the unmodified `nvme_scrqs` loop inside `ibmvfc_handle_async` causes a compilation error.
> --
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index 19a3f12615648..4d1630d2ab1b5 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> @@ -3462,28 +3501,41 @@ static struct ibmvfc_target *ibmvfc_find_target(struct list_head *target_list,
>>   */
>>  static void ibmvfc_process_async_work(struct work_struct *work)
>>  {
>> +	struct ibmvfc_async_sub_crq *subq = NULL;
>>  	struct ibmvfc_async_work *aw;
>> -	struct ibmvfc_async_crq *crq;
>> +	struct ibmvfc_async_crq *crq = NULL;
>>  	struct ibmvfc_target *tgt;
>>  	struct ibmvfc_host *vhost;
>> -	struct fc_els_fpin *fpin;
>> +	struct fc_els_fpin *fpin = NULL;
>>  	unsigned long flags;
>> +	__be64 node_name;
>> +	__be64 scsi_id;
>> +	bool is_subq;
>> +	__be64 wwpn;
>>  
>>  	aw = container_of_const(work, struct ibmvfc_async_work, async_work_s);
>>  	vhost = aw->vhost;
>> -	crq = &aw->crq;
>> +	is_subq = aw->is_subq;
>> +	if (is_subq) {
>> +		subq = &aw->crq.subq;
>> +		scsi_id = 0;
>> +		wwpn = subq->wwpn;
>> +		node_name = subq->id.node_name;
>
> [Severity: High]
> Does this incorrectly treat assoc_id as a WWNN? It looks like we should
> check the IBMVFC_ASYNC_ID_IS_ASSOC_ID flag before unconditionally reading
> node_name, as this could prevent target lookups and cause FPIN events
> to be dropped.

The v7 patches now check taht IBMVFC_ASYNC_ID_IS_ASSOC_ID is not set
before using subq->id.node_name.

> [ ... ]
>
>> @@ -3511,25 +3567,52 @@ static void ibmvfc_process_async_work(struct work_struct *work)
>>  
>>  /**
>>   * ibmvfc_handle_async - Handle an async event from the adapter
>> - * @crq:	crq to process
>> + * @crq:	ibmvfc_async_crq or ibmvfc_async_sub_crq
>>   * @vhost:	ibmvfc host struct
>> + * @is_sub_crq:	indicates whether the crq points to a struct ibmvfc_async_sub_crq
>>   *
>>   **/
>> -VISIBLE_IF_KUNIT void ibmvfc_handle_async(struct ibmvfc_async_crq *crq,
>> -					  struct ibmvfc_host *vhost)
>> +VISIBLE_IF_KUNIT void ibmvfc_handle_async(void *crq,
>> +					  struct ibmvfc_host *vhost,
>> +					  bool is_sub_crq)
>>  {
>
> [Severity: Low]
> Will this cause a compilation error? The crq parameter type is changed to a
> void pointer here, but there is still an unmodified nvme_scrqs loop later in
> ibmvfc_handle_async() that directly dereferences it like crq->scsi_id.

No, this does not cause a compilation error.

>> -	const struct ibmvfc_async_desc *desc = ibmvfc_get_ae_desc(be64_to_cpu(crq->event));
>> +	struct ibmvfc_async_crq *async_crq = NULL;
>> +	struct ibmvfc_async_sub_crq *subq = NULL;
>>  	const struct ibmvfc_async_desc *desc;
>>  	struct ibmvfc_async_work *aw;
>>  	struct ibmvfc_target *tgt;
>> -
>> -	ibmvfc_log(vhost, desc->log_level, "%s event received. scsi_id: %llx, wwpn: %llx,"
>> -		   " node_name: %llx%s\n", desc->desc, be64_to_cpu(crq->scsi_id),
>> -		   be64_to_cpu(crq->wwpn), be64_to_cpu(crq->node_name),
>> -		   ibmvfc_get_link_state(crq->link_state));
>> -
>> -	switch (be64_to_cpu(crq->event)) {
>> +	__be64 node_name;
>> +	__be64 scsi_id;
>> +	u8 link_state;
>> +	__be64 wwpn;
>> +	u64 event;
>> +
>> +	if (is_sub_crq) {
>> +		subq = crq;
>> +		event = be16_to_cpu(subq->event);
>> +		link_state = subq->link_state;
>> +		scsi_id = 0;
>> +		wwpn = subq->wwpn;
>> +		node_name = subq->id.node_name;
>
> [Severity: High]
> Is it possible to receive an assoc_id here instead of a node_name?
> Missing the check for IBMVFC_ASYNC_ID_IS_ASSOC_ID could result in mismatches
> during target lookup.

Fixed in v7.

> [ ... ]
>
>> @@ -3615,16 +3698,27 @@ VISIBLE_IF_KUNIT void ibmvfc_handle_async(struct ibmvfc_async_crq *crq,
>>  		if (aw) {
>>  			INIT_WORK(&aw->async_work_s, ibmvfc_process_async_work);
>>  			aw->vhost = vhost;
>> -			aw->crq = *crq;
>> +			aw->is_subq = is_sub_crq;
>> +			if (is_sub_crq)
>> +				aw->crq.subq = *subq;
>> +			else
>> +				aw->crq.async_crq = *async_crq;
>>  			queue_work(vhost->fpin_workq, &aw->async_work_s);
>>  		} else
>>  			dev_err_ratelimited(vhost->dev,
>>  					    "can't offload async CRQ to work queue\n");
>>  		break;
>>  	default:
>> -		dev_err(vhost->dev, "Unknown async event received: %lld\n", crq->event);
>> +		dev_err(vhost->dev, "Unknown async event received: %llu\n", event);
>>  		break;
>>  	}
>> +
>> +	rmb();
>> +	if (is_sub_crq)
>> +		subq->valid = 0;
>> +	else
>> +		async_crq->valid = 0;
>> +	wmb();
>>  }
>
> [Severity: High]
> Can clearing the valid byte here lead to silently dropped events?
>
> Since ibmvfc_handle_async() clears the valid byte to 0, it tells the hardware
> the slot can be reused.
>
> In the caller ibmvfc_tasklet():
>
> 	while ((async = ibmvfc_next_async_crq(vhost)) != NULL) {
> 		ibmvfc_handle_async(async, vhost, false);
> 		async->valid = 0;
> 		wmb();
> 	}
>
> If the hardware writes a new event into this slot between the first clear in
> ibmvfc_handle_async() and the redundant clear in ibmvfc_tasklet(), wouldn't
> the second clear overwrite the new event's valid byte and lose it?

In v7, the CRQ valid bit is now cleared by the caller of
ibmvfc_handle_async() in all cases.

> [ ... ]
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
>> index 64a517a74af8e..0b29c1e6478b5 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
>> @@ -58,7 +58,7 @@ static void ibmvfc_async_fpin_test(struct kunit *test)
>>  		crq[fs].scsi_id = cpu_to_be64(tgt->scsi_id);
>>  		crq[fs].wwpn = cpu_to_be64(tgt->wwpn);
>>  		crq[fs].node_name = cpu_to_be64(tgt->ids.node_name);
>> -		ibmvfc_handle_async(&crq[fs], vhost);
>> +		ibmvfc_handle_async(&crq[fs], vhost, false);
>>  		msleep(1U);
>>  	}
>
> [Severity: Medium]
> Will this lead to flaky tests on heavily loaded systems?
>
> Using msleep(1U) to wait for asynchronous workqueue execution might not give
> the workqueue enough time to run. Could we use flush_workqueue() instead?

In v7 all KUnit tests now use flush_workqueue().

> [ ... ]
>
>> @@ -93,7 +93,110 @@ static void ibmvfc_async_fpin_test(struct kunit *test)
>>  	crq[0].scsi_id = cpu_to_be64(tgt->scsi_id);
>>  	crq[0].wwpn = cpu_to_be64(tgt->wwpn);
>>  	crq[0].node_name = cpu_to_be64(tgt->ids.node_name);
>> -	ibmvfc_handle_async(&crq[0], vhost);
>> +	ibmvfc_handle_async(&crq[0], vhost, false);
>> +	msleep(1U);
>
> [Severity: Medium]
> Should we avoid using msleep(1U) here for synchronizing with the workqueue?
>
> If the workqueue doesn't run within 1 millisecond, the assertions that follow
> could fail.

See above.

> [ ... ]
>
>> +	/* bad path */
>> +	crq[0].valid = 0x80;
>> +	crq[0].link_state = IBMVFC_AE_LS_LINK_UP;
>> +	crq[0].fpin_status = 0; /* bad value */
>> +	crq[0].event = cpu_to_be16(IBMVFC_AE_FPIN);
>> +	crq[0].wwpn = cpu_to_be64(tgt->wwpn);
>> +	crq[0].id.node_name = cpu_to_be64(tgt->ids.node_name);
>> +	ibmvfc_handle_async(&crq[0], vhost, true);
>>  	msleep(1U);
>
> [Severity: Medium]
> Same concern here about using msleep(1U) to wait for the workqueue.

See above.

-Dave