Re: [PATCH 2/5] ibmvfc: Add NOOP command support

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing empty list check in `ibmvfc_noop_test` causes a kernel panic if no VFC hardware is present.
- [Medium] The KUnit test spuriously fails if the physical adapter or VIOS does not support the NOOP capability.
- [Low] Passing `NULL` to `cpu_to_be64()` triggers an incompatible integer-to-pointer conversion warning.
--

--- Patch [2]: [PATCH 2/5] ibmvfc: Add NOOP command support ---
commit e47751d42fb9d0e5cf4c7d82f8f6ec6ab8f6405c
Author: Dave Marquardt <[email protected]>

ibmvfc: Add NOOP command support

This commit adds support for the VFC_NOOP command to the ibmvfc driver. It
updates the capabilities sent during login to include NOOP support and
handles NOOP CRQs. It also adds a KUnit test for the new command.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> index 1c238896049f8..3359e4ebebe29 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c

[ ... ]

> @@ -79,8 +79,35 @@ static void ibmvfc_handle_fpin_event_test(struct kunit *test)
>  	}
>  }
>  
> +/**
> + * ibmvfc_noop_test - unit test for VFC_NOOP command
> + * @test: pointer to kunit structure
> + *
> + * Return: void
> + */
> +static void ibmvfc_noop_test(struct kunit *test)
> +{
> +	struct ibmvfc_host *vhost;
> +	struct list_head *queue;
> +	struct ibmvfc_crq crq;
> +	struct list_head *headp;
> +	LIST_HEAD(evtq);
> +
> +	headp = ibmvfc_get_headp();
> +	queue = headp->next;
> +	vhost = container_of(queue, struct ibmvfc_host, queue);

[Severity: High]
Does this code dereference a wild pointer if no VFC hardware is present?

Unlike the previous test which safely aborts using
KUNIT_ASSERT_PTR_NE(test, queue, headp), it looks like this test accesses
headp->next unconditionally. If the list is empty, such as when running in
a CI environment without physical or virtual IBMVFC adapters, could calculating
the vhost pointer and subsequently dereferencing it crash the test runner?

> +
> +	KUNIT_EXPECT_TRUE(test, ibmvfc_check_caps(vhost, IBMVFC_SUPPORT_NOOP_CMD));

[Severity: Medium]
Will this test spuriously fail if the physical adapter or VIOS firmware is
older and does not support the NOOP capability? Should the test gracefully
skip instead of failing in environments without IBMVFC_SUPPORT_NOOP_CMD
support?

> +
> +	crq.valid = 0x80;
> +	crq.format = IBMVFC_VFC_NOOP;
> +	crq.ioba = cpu_to_be64(NULL);

[Severity: Low]
Does passing NULL to cpu_to_be64() trigger an incompatible integer-to-pointer
conversion warning? NULL is a pointer type, but the byte-swapping macro expects
a 64-bit integer, which might break the build if CONFIG_WERROR is enabled.

> +	ibmvfc_handle_crq(&crq, vhost, &evtq);
> +}

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