Re: [PATCH v18 13/13] scsi: ibmvfc: handle extended FPIN events

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

sashiko-bot@kernel.org writes:

>>     scsi: ibmvfc: handle extended FPIN events
>>     
>>     Implement support for extended FPIN messages received via the
>>     asynchronous sub-queue, completing full FPIN functionality.
>>     
>>     Extended FPIN messages provide more detailed information about fabric
>>     events compared to basic FPIN messages, including specific event types,
>>     modifiers, thresholds, event counts, and severity.
>>     
>>     Add ibmvfc_ext_fpin_to_desc() to convert extended FPIN messages from
>>     async sub-queue format to fc_els_fpin structures with complete descriptor
>>     information. Thread detecting_wwpn (fc_host_port_name()) through
>>     ibmvfc_ext_fpin_to_desc() to ibmvfc_common_fpin_to_desc() consistent
>>     with ibmvfc_basic_fpin_to_desc() and ibmvfc_full_fpin_to_desc(). Update
>>     ibmvfc_process_async_work() and ibmvfc_handle_async() to zero node_name
>>     and dispatch to ibmvfc_ext_fpin_to_desc() when the
>>     IBMVFC_ASYNC_IS_FPIN_EXT flag is set.
>>     
>>     Extend ibmvfc_common_fpin_to_desc() with a severity parameter so that
>>     extended FPIN events can propagate the severity value from the VIOS when
>>     IBMVFC_FPIN_SEVERITY_VALID is set, rather than always defaulting to
>>     FPIN_CONGN_SEVERITY_WARNING. Severity is only carried by the congestion
>>     descriptor used for IBMVFC_AE_FPIN_LINK_CONGESTED and
>>     IBMVFC_AE_FPIN_CONGESTION_CLEARED; the peer congestion and link
>>     integrity descriptors have no severity field, so a severity supplied
>>     for the other FPIN statuses is ignored.
>>     
>>     Set the IBMVFC_CAN_HANDLE_FPIN_EXT capability bit (0x800) during login
>>     to inform the VIOS that the client can process extended FPIN messages.
>>     Add the IBMVFC_CAN_CLS_CRQ_PRE_DMP capability bit (0x400) definition
>>     for the intervening protocol-defined capability. Also add the
>>     IBMVFC_SUPPORT_FPIN_EXT response capability bit definition used to gate
>>     extended FPIN processing.
>>     
>>     In ibmvfc_process_async_work(), use goto to bypass the generic "FPIN
>>     event received, unable to process" error message when a more specific
>>     diagnostic has already been emitted for invalid or unsupported extended
>>     FPIN events.
>>     
>>     Add a KUnit test, ibmvfc_extended_fpin_test(), that sends extended FPIN
>>     events through ibmvfc_process_async_work() for each FPIN status with
>>     the event types valid for that status, setting only
>>     IBMVFC_FPIN_EVENT_TYPE_VALID, and checks that the matching fc_host or
>>     rport FPIN statistic is incremented. The test is skipped if no ibmvfc
>>     device, online target rport, or IBMVFC_SUPPORT_FPIN_EXT capability is
>>     available. It does not exercise the modifier, threshold, severity or
>>     event count fields, nor the error paths for a missing
>>     IBMVFC_FPIN_EVENT_TYPE_VALID flag or an unadvertised
>>     IBMVFC_SUPPORT_FPIN_EXT capability.
>>     
>>     Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
>>     Signed-off-by: Dave Marquardt <davemarq@linux.ibm.com>
>
> [Severity: Low]
> This isn't a bug in the code, but does the commit message match the changes
> in this patch?
>
> The third paragraph states that this patch updates
> ibmvfc_process_async_work() and ibmvfc_handle_async() to zero node_name when
> IBMVFC_ASYNC_IS_FPIN_EXT is set, however ibmvfc_handle_async() is untouched
> here and zeroing node_name in both functions was already introduced in
> commit 6abed589f0fb ("scsi: ibmvfc: extend async event handlers for async
> sub-CRQ events").
>
> Conversely, setting severity = 0 for IBMVFC_AE_FPIN_PORT_CLEARED and
> IBMVFC_AE_FPIN_CONGESTION_CLEARED in ibmvfc_basic_fpin_to_desc() and
> ibmvfc_full_fpin_to_desc() is done in this commit, whereas it was described
> in the commit message for 6abed589f0fb.

Right on both counts. node_name zeroing is already added in patch 9, and
patch 13 doesn't change ibmvfc_handle_async(). severity = 0 handling is
implemented in patch 13.

>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index 6e1ecf6891c71..bc65c4d9cb7ec 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>
> [ ... ]
>
>> @@ -3507,7 +3517,48 @@ ibmvfc_full_fpin_to_desc(struct ibmvfc_async_sub_crq *ibmvfc_fpin,
>>  	return ibmvfc_common_fpin_to_desc(ibmvfc_fpin->fpin_status, detecting_wwpn,
>>  					  cpu_to_be64(wwpn), type, cpu_to_be16(0),
>>  					  cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_THRESHOLD),
>> -					  cpu_to_be32(1));
>> +					  cpu_to_be32(1), severity);
>> +}
>> +
>> +/**
>> + * ibmvfc_ext_fpin_to_desc(): allocate and populate a struct fc_els_fpin struct
>> + * containing a descriptor.
>> + * @ibmvfc_fpin:	Pointer to async subq FPIN data
>> + * @detecting_wwpn:	WWPN of the local port that detected the event (big-endian)
>> + * @wwpn:		attached port WWPN (host-endian)
>> + *
>> + * Allocate a struct fc_els_fpin containing a descriptor and populate
>> + * based on data from *ibmvfc_fpin. The caller must have verified that
>> + * IBMVFC_FPIN_EVENT_TYPE_VALID is set in the FPIN data flags.
>> + *
>> + * Return:
>> + * NULL     - unable to allocate structure
>> + * non-NULL - pointer to populated struct fc_els_fpin
>> + */
>> +static struct fc_els_fpin *
>> +ibmvfc_ext_fpin_to_desc(struct ibmvfc_async_subq_fpin *ibmvfc_fpin,
>> +			__be64 detecting_wwpn, u64 wwpn)
>> +{
>> +	u8 flags = ibmvfc_fpin->fpin_data.flags;
>> +	__be32 threshold = cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_THRESHOLD);
>> +	__be16 modifier = 0;
>> +	__be32 count = cpu_to_be32(1);
>> +	__be16 type = ibmvfc_fpin->fpin_data.event_type;
>> +	__u8 severity = FPIN_CONGN_SEVERITY_WARNING;
>> +
>> +	if (flags & IBMVFC_FPIN_MODIFIER_VALID)
>> +		modifier = ibmvfc_fpin->fpin_data.event_type_modifier;
>> +	if (flags & IBMVFC_FPIN_THRESHOLD_VALID)
>> +		threshold = ibmvfc_fpin->fpin_data.event_threshold;
>> +	if (flags & IBMVFC_FPIN_EVENT_COUNT_VALID)
>> +		count = ibmvfc_fpin->fpin_data.event_data.event_count;
>> +	if (flags & IBMVFC_FPIN_SEVERITY_VALID)
>> +		severity = ibmvfc_fpin->fpin_data.event_data.severity;
>> +
>> +	return ibmvfc_common_fpin_to_desc(ibmvfc_fpin->fpin_status, detecting_wwpn,
>> +					  cpu_to_be64(wwpn), type,
>> +					  modifier, threshold, count,
>> +					  severity);
>>  }
>
> [Severity: Medium]
> Should severity in ibmvfc_ext_fpin_to_desc() default to 0 for cleared
> congestion events when IBMVFC_FPIN_SEVERITY_VALID is not set?
>
> In ibmvfc_basic_fpin_to_desc() and ibmvfc_full_fpin_to_desc(), this patch
> sets severity = 0 for IBMVFC_AE_FPIN_PORT_CLEARED and
> IBMVFC_AE_FPIN_CONGESTION_CLEARED:
>
> 	case IBMVFC_AE_FPIN_PORT_CLEARED:
> 	case IBMVFC_AE_FPIN_CONGESTION_CLEARED:
> 		type = cpu_to_be16(FPIN_CONGN_CLEAR);
> 		severity = 0;
> 		break;
>
> In ibmvfc_ext_fpin_to_desc(), severity is initialized to
> FPIN_CONGN_SEVERITY_WARNING and is only updated if
> IBMVFC_FPIN_SEVERITY_VALID is set in flags. When an extended FPIN event for
> IBMVFC_AE_FPIN_CONGESTION_CLEARED (or with event_type FPIN_CONGN_CLEAR) is
> received without the optional IBMVFC_FPIN_SEVERITY_VALID flag (as in
> ibmvfc_extended_fpin_test()), cdesc->severity in the ELS FPIN descriptor is
> populated with FPIN_CONGN_SEVERITY_WARNING instead of 0.

I agree. In v19, for IBMVFC_AE_FPIN_PORT_CLEARED and
IBMVFC_AE_FPIN_CONGESTION_CLEARED, ibmvfc_ext_fpin_to_desc() will
default severity to 0, like the basic and full conversion routines, and
only used the VIOS-provided severity when IBMVFC_FPIN_SEVERITY_VALID is
set.

>> @@ -3624,14 +3681,30 @@ VISIBLE_IF_KUNIT void ibmvfc_process_async_work(struct work_struct *work)
>>  		goto free;
>>  	}
>>  
>> -	if (crq)
>> +	if (crq) {
>>  		fpin = ibmvfc_basic_fpin_to_desc(crq,
>>  				cpu_to_be64(fc_host_port_name(vhost->host)),
>>  				tgt->wwpn);
>> -	else
>> -		fpin = ibmvfc_full_fpin_to_desc(subq,
>> -				cpu_to_be64(fc_host_port_name(vhost->host)),
>> -				tgt->wwpn);
>> +	} else {
>> +		sqfpin = (struct ibmvfc_async_subq_fpin *)subq;
>> +		if ((subq->flags & IBMVFC_ASYNC_IS_FPIN_EXT) == 0) {
>> +			fpin = ibmvfc_full_fpin_to_desc(subq,
>> +					cpu_to_be64(fc_host_port_name(vhost->host)),
>> +					tgt->wwpn);
>> +		} else if (!(sqfpin->fpin_data.flags & IBMVFC_FPIN_EVENT_TYPE_VALID)) {
>> +			dev_err_ratelimited(vhost->dev,
>> +					    "Invalid extended FPIN event received\n");
>> +			goto skip_generic_error;
>> +		} else if (!ibmvfc_check_caps(vhost, IBMVFC_SUPPORT_FPIN_EXT)) {
>> +			dev_err_ratelimited(vhost->dev,
>> +					    "Unexpected extended FPIN event received\n");
>> +			goto skip_generic_error;
>> +		} else {
>> +			fpin = ibmvfc_ext_fpin_to_desc(sqfpin,
>> +					cpu_to_be64(fc_host_port_name(vhost->host)),
>> +					tgt->wwpn);
>> +		}
>> +	}
>
> [Severity: High]
> What happens in ibmvfc_process_async_work() when a host-scope FPIN event
> (IBMVFC_AE_FPIN_LINK_CONGESTED or IBMVFC_AE_FPIN_CONGESTION_CLEARED) arrives
> on the async sub-CRQ with wwpn == 0 or with a WWPN that does not match a
> target in scsi_scrqs or nvme_scrqs?
>
> Earlier in ibmvfc_process_async_work(), when aw->event.type ==
> IBMVFC_ASYNC_CRQ_SUB and IBMVFC_ASYNC_IS_FPIN_EXT is set, scsi_id,
> node_name, and assoc_id are all set to 0, while crq remains NULL:
>
> 	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;
> 		}
> 	}
>
> If wwpn is also 0, the zero-identifier check right after that is taken:
>
> 	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;
> 	}
>
> Here crq is NULL, so ibmvfc_basic_fpin_to_desc(crq, ...) dereferences
> crq->fpin_status on a NULL pointer.
>
> In addition, if wwpn is non-zero (for example, set to the host's own WWPN or
> a target whose rport/nvme_remote_port is no longer present) so
> ibmvfc_find_target() returns NULL, the fallback path only calls
> ibmvfc_basic_fpin_to_desc() or ibmvfc_full_fpin_to_desc():
>
> 	} else {
> 		spin_unlock_irqrestore(&vhost->host->host_lock, flags);
> 		if (fpin_status == IBMVFC_AE_FPIN_LINK_CONGESTED ||
> 		    fpin_status == IBMVFC_AE_FPIN_CONGESTION_CLEARED) {
> 			if (crq)
> 				fpin = ibmvfc_basic_fpin_to_desc(crq,
> 						cpu_to_be64(fc_host_port_name(vhost->host)), 0);
> 			else
> 				fpin = ibmvfc_full_fpin_to_desc(subq,
> 						cpu_to_be64(fc_host_port_name(vhost->host)), 0);
> 			goto send_fpin;
> 		}
>
> Because IBMVFC_ASYNC_IS_FPIN_EXT and ibmvfc_ext_fpin_to_desc() are only
> handled on the tgt != NULL path, shouldn't the zero-identifier and
> target-not-found host-scope paths also handle subq and
> IBMVFC_ASYNC_IS_FPIN_EXT?

The zero identifier branch will be addressed in patch 9 in the next
revision. You're right that the host-scop paths only use the full
helper. I'll fix this in v19.

>>  send_fpin:
>>  	if (fpin) {
>> @@ -3642,6 +3715,7 @@ VISIBLE_IF_KUNIT void ibmvfc_process_async_work(struct work_struct *work)
>>  	} else
>>  		dev_err_ratelimited(vhost->dev, "FPIN event received, unable to process\n");
>>  
>> + skip_generic_error:
>>  	if (tgt)
>>  		kref_put(&tgt->kref, ibmvfc_release_tgt);
>>   free:
>
> [Severity: Critical]
> Can tgt be uninitialized when reaching kref_put(&tgt->kref,
> ibmvfc_release_tgt) in ibmvfc_process_async_work()?
>
> At the top of ibmvfc_process_async_work(), tgt is declared without being
> initialized to NULL:
>
> 	struct ibmvfc_target *tgt;
>
> When all target identifiers are zero (!scsi_id && !wwpn && !node_name &&
> !assoc_id, such as for host-scope FPIN events or the zero-identifier test
> cases in ibmvfc_async_fpin_test()), the function jumps to send_fpin 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;
> 	}
>
> After send_fpin, execution falls through to skip_generic_error and checks
> if (tgt) kref_put(&tgt->kref, ibmvfc_release_tgt) using the uninitialized
> tgt pointer on the stack. Should tgt be initialized to NULL at declaration?

This will be fixed in patch 6 by initializing tgt and fpin to NULL, and
the NULL crq issue will be fixed in patch 9.

-Dave