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 <[email protected]>
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/[email protected]?part=8
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.