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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Extended FPIN events are incorrectly dropped because `node_name` is populated with garbage data from overlapping `fpin_data` union fields, causing target lookup to fail.
- [High] The async workqueue `fpin_workq` is not properly flushed or destroyed before the `vhost` memory is freed during driver removal, leading to a Use-After-Free.
- [Medium] The newly added KUnit tests for extended FPIN events omit assertions and use zero-initialized data that masks the `node_name` extraction bug.
- [Medium] KUnit tests use an arbitrary 1ms sleep to wait for asynchronous work queues to complete, causing test flakiness.
--

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

ibmvfc: handle extended FPIN events

Implement support for extended FPIN messages via the asynchronous sub-queue.
This adds parsing for detailed fabric events, updates capability
advertisement, and includes KUnit validation for the new functionality.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> index eb786ac242745..b131a4a3041b7 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c

[ ... ]

> @@ -3372,12 +3373,48 @@ ibmvfc_full_fpin_to_desc(struct ibmvfc_async_subq *ibmvfc_fpin)
>  					  cpu_to_be32(1));
>  }
>  
> +/**
> + * ibmvfc_ext_fpin_to_desc(): allocate and populate a struct fc_els_fpin struct

[ ... ]

>  /**
>   * ibmvfc_process_async_work - Process IBMVFC_AE_FPIN async CRQ from work queue
>   * @work: pointer to work_struct
>   */
>  static void ibmvfc_process_async_work(struct work_struct *work)
>  {
> +	struct ibmvfc_async_subq_fpin *sqfpin;
>  	struct ibmvfc_target *tgt, *next;

[Severity: High]
Does this leave the work queue active during driver removal?

In ibmvfc_remove(), the driver drops the last reference to the SCSI host via
scsi_host_put(vhost->host), which frees the shost and the embedded vhost
structure. However, fpin_workq is allocated via devm_alloc_workqueue(). The
devres cleanup runs after ibmvfc_remove() returns.

If destroy_workqueue() drains pending work during devm teardown, it could
execute ibmvfc_process_async_work(), which accesses the freed vhost memory
when taking the host lock: spin_lock_irqsave(vhost->host->host_lock, flags).

Could we flush or destroy the work queue explicitly in ibmvfc_remove() before
releasing the SCSI host?

[ ... ]

> @@ -3439,8 +3476,20 @@ static void ibmvfc_process_async_work(struct work_struct *work)
>  
>  	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;

[Severity: High]
Does this drop extended FPIN events by matching on garbage data?

For an extended FPIN event, struct ibmvfc_async_subq and
struct ibmvfc_async_subq_fpin share memory layout. Earlier in
ibmvfc_process_async_work(), the code extracts:

    wwpn = subq->wwpn;
    node_name = subq->id.node_name;

The subq->id.node_name overlaps with fpin_data.event_threshold and
fpin_data.event_data.event_count. Since these data values are typically
non-zero, node_name is populated with garbage. This causes the target
lookup to fail because it attempts to match the garbage value:

    if (node_name && cpu_to_be64(tgt->ids.node_name) != node_name)
        continue;

Could this skip valid targets and unintentionally discard the events?

> +		if ((subq->flags & IBMVFC_ASYNC_IS_FPIN_EXT) == 0) {
> +			fpin = ibmvfc_full_fpin_to_desc(subq);
> +		} else if (!(sqfpin->fpin_data.flags & IBMVFC_FPIN_EVENT_TYPE_VALID)) {
> +			dev_err_ratelimited(vhost->dev,

[ ... ]

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> index 86c1f6daca586..2f43916b43a5a 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c

[ ... ]

> @@ -62,8 +63,6 @@ static void ibmvfc_async_fpin_test(struct kunit *test)
>  		msleep(1U);
>  	}
>  
> -	msleep(500U);

[Severity: Medium]
Is a 1ms delay sufficient to prevent racy tests?

The test calls ibmvfc_handle_async(), which queues work on vhost->fpin_workq.
Relying on msleep(1U) to wait for the worker thread to finish before reading
updated statistics is inherently racy. If the work is not scheduled within 1ms
under heavy load, the test may read stale values and fail.

Should this deterministically synchronize by calling flush_workqueue() instead?

>  	post[IBMVFC_AE_FPIN_LINK_CONGESTED] = READ_ONCE(fc_host->fpin_stats.cn_device_specific);
>  	post[IBMVFC_AE_FPIN_PORT_CONGESTED] = READ_ONCE(tgt->rport->fpin_stats.cn);
>  	post[IBMVFC_AE_FPIN_PORT_CLEARED] = READ_ONCE(tgt->rport->fpin_stats.cn_clear);

[ ... ]

> @@ -116,8 +115,117 @@ static void ibmvfc_async_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]
Are there missing test assertions here?

The IBMVFC_TEST_FPIN_EXT macro assigns pre and post statistic values but lacks
any KUNIT_EXPECT_* assertions to actually verify the outcomes.

Additionally, because the dummy crq structures are zero-initialized in
ibmvfc_extended_fpin_test(), event_threshold and event_count are 0. This
coincidentally masks the overlapping node_name extraction bug mentioned
earlier because node_name evaluates to 0, which bypasses the broken target
lookup check and allows the test to artificially succeed.

Could assertions be added, and non-zero dummy data be used to thoroughly test
this path?

Also, similar to ibmvfc_async_fpin_test(), the msleep(1U) used here might
lead to flaky test failures if the workqueue runs slower than expected.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-ibmvfc-fpin-support-v4-0-ef031ac19520@linux.ibm.com?part=8