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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Unconditional extraction of `node_name` from sub-CRQs causes an aliasing bug for extended FPIN events, leading to target lookup failures.
- [High] KUnit test structures for extended FPIN events are allocated on the stack without zero initialization.
- [Medium] KUnit tests lack validation assertions for statistics and suffer from race conditions due to an unsafe sleep-based wait.

Pre-existing issues:
- [Critical] Using the list iterator `tgt` after `list_for_each_entry_safe` terminates without a match causes severe out-of-bounds memory accesses.
- [High] Missing reference counting on `tgt` when accessed outside of `host_lock` leads to a potential Use-After-Free.
--

commit 5f5e44381670a22f5b1837898b25fb9f7a477edd
Author: Dave Marquardt <davemarq@linux.ibm.com>
Subject: ibmvfc: handle extended FPIN events

This commit adds extended FPIN handling to the ibmvfc driver. It tells the
VIOS that ibmvfc can handle extended FPIN messages and converts any received
FPIN messages to struct fc_els descriptors. It also extends
ibmvfc_process_async_work to handle these extended FPIN messages.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> index 36abca0bbd34d..93622cb76adc0 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c

[ ... ]

> @@ -3437,8 +3474,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 function safely handle the node_name extraction for extended FPIN
events?

When an extended FPIN event is received, subq->id.node_name is read
unconditionally earlier in this function. For extended FPINs, this memory
region perfectly aliases with fpin_data.event_threshold and event_count.

This results in node_name being populated with this FPIN data. Will the
target lookup then compare the target's WWNN against this garbage node_name,
fail to find a match, and reject legitimate extended FPIN events?

[Severity: Critical]
This is a pre-existing issue, but does using tgt after the list lookup loop
terminates without a match cause out-of-bounds memory accesses?

If list_for_each_entry_safe() finishes without finding a match, tgt will
point to an invalid address computed from the list head. Since standard
Linux list iterators are not NULL when the loop terminates, a check like
(!tgt) will evaluate to false.

Could accessing tgt->wwpn or tgt->vhost here cause arbitrary memory reads
or a kernel panic?

[Severity: High]
This is a pre-existing issue, but is there a missing reference count on tgt
when it is accessed outside of the host_lock?

Earlier in the function, spin_unlock_irqrestore() drops the lock after
finding the target, but no kref is taken. If a concurrent teardown or link
down event frees the target, will accessing tgt->wwpn or tgt->vhost below
result in a use-after-free?

> +		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,
> +					    "Invalid extended FPIN event received\n");

[ ... ]

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> index 6bde921975492..fab36891dbc25 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c

[ ... ]

> +#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]
Does this macro actually verify the post-condition statistics?

The post statistics are read, but there are no KUNIT_EXPECT_* assertions to
validate them against the pre statistics. Will this allow the test to silently
pass even if the driver logic fails?

Additionally, is using msleep(1U) to wait for the asynchronous work queue to
process the event race-prone?

> +
> +/**
> + * 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)
> +{
> +	enum ibmvfc_ae_fpin_status fs;
> +	struct ibmvfc_async_subq_fpin crq[IBMVFC_AE_FPIN_CONGESTION_CLEARED+1];

[Severity: High]
Are these arrays of struct ibmvfc_async_subq_fpin safely initialized?

These arrays are allocated on the stack. The IBMVFC_TEST_FPIN_EXT macro
partially initializes them but leaves fields like fpin_data.event_threshold
uninitialized.

Could this stack garbage be passed to the driver, causing erratic test
behavior depending on previous stack contents?

> +	struct ibmvfc_async_subq_fpin
> +		crqcn[IBMVFC_AE_FPIN_PORT_CONGESTED][FPIN_CONGN_DEVICE_SPEC+1];
> +	struct ibmvfc_async_subq_fpin crqportdg[FPIN_LI_DEVICE_SPEC+1];

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