Re: [PATCH v6 1/8] scsi: ibmvfc: add basic FPIN support

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The `fpin_workq` workqueue is destroyed too early during driver removal, allowing a NULL pointer dereference if an FPIN event arrives before the CRQ is torn down.
- [Medium] KUnit test uses `KUNIT_EXPECT_NOT_NULL` instead of `KUNIT_ASSERT_NOT_NULL` on a pointer before dereferencing it, causing a crash if the pointer is NULL.
- [Medium] KUnit test uses a hardcoded `msleep(1U)` to wait for workqueue execution, leading to race conditions and test flakiness.
- [Medium] KUnit test accesses global and host-specific lists without holding the required locks, causing potential use-after-free or list corruption.
--

--- Patch [1]: [PATCH v6 1/8] scsi: ibmvfc: add basic FPIN support ---
commit ff91d1d40ba087181a6c3109c5264dac2870f637
Author: Dave Marquardt <[email protected]>

scsi: ibmvfc: add basic FPIN support

Implement support for a basic level of Fabric Performance Impact
Notifications (FPIN) in the ibmvfc driver to enable monitoring of
fabric congestion and link integrity events. Offloads FPIN
processing to a dedicated workqueue and adds a KUnit test module.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 93b9f699c2e53..22b4d639e74be 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c

[ ... ]

> @@ -6945,6 +7213,9 @@ static void ibmvfc_remove(struct vio_dev *vdev)
>  	ENTER;
>  	ibmvfc_remove_trace_file(&vhost->host->shost_dev.kobj, &ibmvfc_trace_attr);
>  
> +	destroy_workqueue(vhost->fpin_workq);
> +	vhost->fpin_workq = NULL;
> +

[Severity: High]
Is the fpin_workq destroyed too early during driver removal?

If an asynchronous FPIN event arrives from the fabric after this point but
before the CRQ is torn down in ibmvfc_release_crq_queue() later in the
function, ibmvfc_handle_async() could be called and attempt to queue work:

ibmvfc_handle_async()
    ...
    queue_work(vhost->fpin_workq, &aw->async_work_s);

Could this result in a NULL pointer dereference?

>  	spin_lock_irqsave(vhost->host->host_lock, flags);
>  	ibmvfc_link_down(vhost, IBMVFC_HOST_OFFLINE);
>  	spin_unlock_irqrestore(vhost->host->host_lock, flags);

[ ... ]

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> new file mode 100644
> index 0000000000000..64a517a74af8e
> --- /dev/null
> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c

[ ... ]

> +static void ibmvfc_async_fpin_test(struct kunit *test)
> +{
> +	u64 post[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1];
> +	u64 pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1];
> +	enum ibmvfc_ae_fpin_status fs;
> +	struct fc_host_attrs *fc_host;
> +	struct ibmvfc_async_crq crq[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1];
> +	struct ibmvfc_target *tgt;
> +	struct ibmvfc_host *vhost;
> +	struct list_head *queue;
> +	struct list_head *headp;
> +
> +	headp = ibmvfc_get_headp();
> +	if (list_empty(headp))
> +		kunit_skip(test, "No ibmvfc devices available");
> +	queue = headp->next;
> +	vhost = container_of_const(queue, struct ibmvfc_host, queue);
> +
> +	KUNIT_ASSERT_GE_MSG(test, vhost->scsi_scrqs.num_targets, 1, "No targets");
> +	tgt = list_first_entry(&vhost->scsi_scrqs.targets, struct ibmvfc_target, queue);

[Severity: Medium]
Is it safe to iterate over the global ibmvfc_head list and access
vhost->scsi_scrqs.targets without acquiring the ibmvfc_driver_lock or
host_lock?

If a device is unbound or a target logs out concurrently while the test
runs, could the lists mutate and cause an invalid pointer dereference?

> +	KUNIT_EXPECT_NOT_NULL(test, tgt->rport);
> +
> +	fc_host = shost_to_fc_host(vhost->host);
> +
> +	pre[IBMVFC_AE_FPIN_LINK_CONGESTED] = READ_ONCE(fc_host->fpin_stats.cn_device_specific);
> +	pre[IBMVFC_AE_FPIN_PORT_CONGESTED] = READ_ONCE(tgt->rport->fpin_stats.cn);

[Severity: Medium]
Since KUNIT_EXPECT_NOT_NULL() allows execution to continue on failure, could a
NULL tgt->rport be immediately dereferenced on the following lines when reading
tgt->rport->fpin_stats.cn?

Should this use KUNIT_ASSERT_NOT_NULL() instead to abort the test and prevent a
crash?

> +	pre[IBMVFC_AE_FPIN_PORT_CLEARED] = READ_ONCE(tgt->rport->fpin_stats.cn_clear);
> +	pre[IBMVFC_AE_FPIN_PORT_DEGRADED] = READ_ONCE(tgt->rport->fpin_stats.li_failure_unknown);
> +	pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED] = READ_ONCE(fc_host->fpin_stats.cn_clear);
> +
> +	for (fs = IBMVFC_AE_FPIN_LINK_CONGESTED; fs <= IBMVFC_AE_FPIN_CONGESTION_CLEARED; fs++) {
> +		crq[fs].valid = 0x80;
> +		crq[fs].link_state = IBMVFC_AE_LS_LINK_UP;
> +		crq[fs].fpin_status = fs;
> +		crq[fs].event = cpu_to_be64(IBMVFC_AE_FPIN);
> +		crq[fs].scsi_id = cpu_to_be64(tgt->scsi_id);
> +		crq[fs].wwpn = cpu_to_be64(tgt->wwpn);
> +		crq[fs].node_name = cpu_to_be64(tgt->ids.node_name);
> +		ibmvfc_handle_async(&crq[fs], vhost);
> +		msleep(1U);
> +	}

[Severity: Medium]
Is msleep(1U) a reliable way to wait for the fpin_workq to process the
offloaded FPIN event?

Under load, it seems like 1 millisecond might be insufficient, causing the test
to read stale counters and fail intermittently. Would it be safer to use
flush_workqueue(vhost->fpin_workq) here instead of a hardcoded sleep?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1