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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] scsi: ibmvfc: uninitialized tgt pointer passed to kref_put() on FPIN path
- [High] scsi: ibmvfc: NULL crq deref and missed ext FPIN on host-scope paths
- [Medium] scsi: ibmvfc: wrong default severity in ibmvfc_ext_fpin_to_desc()
- [Low] scsi: ibmvfc: commit message mismatch with changes in earlier patch
--

commit a2aeb896595835b42eba9922a25c766496e63499
Author: Dave Marquardt <davemarq@linux.ibm.com>

scsi: ibmvfc: handle extended FPIN events

This commit implements support for extended FPIN messages received via the
asynchronous sub-queue by adding ibmvfc_ext_fpin_to_desc(), extending
ibmvfc_common_fpin_to_desc() with a severity parameter, negotiating
IBMVFC_CAN_HANDLE_FPIN_EXT / IBMVFC_SUPPORT_FPIN_EXT capabilities, and
adding a KUnit test.

>     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.

> 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.

> @@ -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?

>  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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-ibmvfc-fpin-support-v18-0-d683b74736a3@linux.ibm.com?part=13