Re: [PATCH v18 09/13] scsi: ibmvfc: extend async event handlers for async sub-CRQ 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 b9b571db44649..7406e7182777c 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> [ ... ]
>> @@ -3483,33 +3528,59 @@ static struct ibmvfc_target *ibmvfc_find_target(struct list_head *target_list,
>>   */
>>  VISIBLE_IF_KUNIT 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;
>
> [Severity: High]
> Should tgt be initialized to NULL here?
>
> When scsi_id, wwpn, node_name, and assoc_id are all zero,
> ibmvfc_process_async_work() jumps directly to the send_fpin label before
> tgt is assigned by ibmvfc_find_target():
>
> 	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;
> 	}
>
> At the end of send_fpin, the cleanup checks:
>
> 	if (tgt)
> 		kref_put(&tgt->kref, ibmvfc_release_tgt);
>
> Wouldn't that evaluate and potentially dereference an uninitialized tgt
> pointer on the zero-identifier path?

This will be fixed in patch 6, as discussed in the response to patch 6.

>>  	struct ibmvfc_host *vhost;
>> -	struct fc_els_fpin *fpin;
>> +	struct fc_els_fpin *fpin = NULL;
>>  	unsigned long flags;
>> +	__be64 assoc_id;
>> +	__be64 node_name;
>> +	__be64 scsi_id;
>> +	__be64 wwpn;
>> +	u8 fpin_status;
>>  
>>  	aw = container_of_const(work, struct ibmvfc_async_work, async_work_s);
>>  	vhost = aw->vhost;
>> -	crq = &aw->crq;
>> +	if (aw->event.type == IBMVFC_ASYNC_CRQ_SUB) {
>> +		subq = &aw->event.subq;
>> +		scsi_id = 0;
>> +		wwpn = subq->wwpn;
>> +		fpin_status = subq->fpin_status;
>> +		if (subq->flags & IBMVFC_ASYNC_ID_IS_ASSOC_ID) {
>> +			node_name = 0;
>> +			assoc_id = subq->id.assoc_id;
>> +		} else if (subq->flags & IBMVFC_ASYNC_IS_FPIN_EXT) {
>> +			node_name = 0;
>> +			assoc_id = 0;
>> +		} else {
>> +			node_name = subq->id.node_name;
>> +			assoc_id = 0;
>> +		}
>> +	} else {
>> +		crq = &aw->event.async_crq;
>> +		scsi_id = crq->scsi_id;
>> +		wwpn = crq->wwpn;
>> +		node_name = crq->node_name;
>> +		fpin_status = crq->fpin_status;
>> +		assoc_id = 0;
>> +	}
>>  
>> -	if (!crq->scsi_id && !crq->wwpn && !crq->node_name) {
>> -		if (crq->fpin_status == IBMVFC_AE_FPIN_LINK_CONGESTED ||
>> -		    crq->fpin_status == IBMVFC_AE_FPIN_CONGESTION_CLEARED)
>> +	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;
>>  	}
>
> [Severity: High]
> Can this dereference a NULL crq pointer when handling an async sub-CRQ
> event?
>
> When aw->event.type is IBMVFC_ASYNC_CRQ_SUB, subq is populated and crq
> remains NULL. If scsi_id, wwpn, node_name, and assoc_id are all zero for a
> host-scope IBMVFC_AE_FPIN_LINK_CONGESTED or
> IBMVFC_AE_FPIN_CONGESTION_CLEARED event, this block unconditionally calls
> ibmvfc_basic_fpin_to_desc(crq, ...), which immediately dereferences
> crq->fpin_status:
>
> static struct fc_els_fpin *
> ibmvfc_basic_fpin_to_desc(struct ibmvfc_async_crq *crq, __be64 detecting_wwpn,
> 			  u64 wwpn)
> {
> 	__be16 type;
>
> 	switch (crq->fpin_status) {
>
> Should this path check if (crq) and dispatch to
> ibmvfc_full_fpin_to_desc(subq, ...) when crq is NULL, matching the target
> fallback path further down in ibmvfc_process_async_work()?

Yes, this is problem. For the next revision the code will check whether
crq is NULL before calling ibmvfc_basic_fpin_to_desc(). If crq is NULL,
the else clause will call ibmvfc_full_fpin_to_desc() with subq.

-Dave