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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Extended FPIN events are incorrectly discarded because a structure union overlap corrupts the `node_name` search key.
- [High] A race condition during driver removal triggers a NULL pointer dereference in the interrupt handler due to premature teardown of `fpin_workq`.
- [Medium] The `ibmvfc_extended_fpin_test` suite lacks KUnit assertions to verify statistic updates and relies on flaky `msleep()` synchronization.
--

--- Patch [8]: [PATCH v6 8/8] scsi: ibmvfc: handle extended FPIN events ---
commit 3ae60b854350b472f08abc0cb2cc22c2f6e12865
Author: Dave Marquardt <davemarq@linux.ibm.com>

scsi: ibmvfc: handle extended FPIN events

Implement support for extended FPIN messages received via the asynchronous
sub-queue. This converts extended FPIN messages from async sub-queue format
to fc_els_fpin structures and updates capabilities and tests.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 0368fb1763c3a..e36c3c0a81fbf 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
[ ... ]
> @@ -3504,6 +3540,7 @@ 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_subq_fpin *sqfpin;
>  	struct ibmvfc_async_work *aw;
>  	struct ibmvfc_async_crq *crq = NULL;

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

[ ... ]
>  	if (crq)
>  		fpin = ibmvfc_basic_fpin_to_desc(crq, tgt->wwpn);
> -	else
> -		fpin = ibmvfc_full_fpin_to_desc(subq);
> +	else {
> +		sqfpin = (struct ibmvfc_async_subq_fpin *)subq;
> +		if ((subq->flags & IBMVFC_ASYNC_IS_FPIN_EXT) == 0) {

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

> +			fpin = ibmvfc_full_fpin_to_desc(subq);
> +		} else if (!(sqfpin->fpin_data.flags & IBMVFC_FPIN_EVENT_TYPE_VALID)) {
[ ... ]
> 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.

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

> +
> +/**
> + * 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?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817-ibmvfc-fpin-support-v6-0-62a2d525e70d@linux.ibm.com?part=8