Re: [PATCH v14 03/11] scsi: ibmvfc: fix potential NULL pointer dereference on failed queue allocation

[Dave Marquardt <davemarq@linux.ibm.com>]

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] scsi: ibmvfc: Build failure due to un-guarded KUnit visibility macros
> --
>
> commit 79ff3803bf62c41db84d726eab4c3cdbd582546c
> Author: Tyrel Datwyler <tyreld@linux.ibm.com>
>
> scsi: ibmvfc: fix potential NULL pointer dereference on failed queue allocation
>
> This commit guards ibmvfc_deregister_channel() against operating on unallocated
> queues and exposes it for KUnit testing. It also adds a new KUnit test module
> for the IBM POWER Virtual FC Client.
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index 145d559041f12..0fe6b6f4260ba 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>
> [ ... ]
>
>> -static void ibmvfc_deregister_channel(struct ibmvfc_host *vhost,
>> +/**
>> + * ibmvfc_deregister_channel - Deregister a sub-CRQ channel with the hypervisor
>> + * @vhost:	ibmvfc host struct
>> + * @channels:	ibmvfc channels struct containing the sub-CRQ array
>> + * @index:	index into the sub-CRQ array
>> + *
>> + * Frees the IRQ, disposes of the IRQ mapping, and calls H_FREE_SUB_CRQ to
>> + * release the sub-CRQ with the hypervisor. On success the queue message
>> + * buffer is zeroed and the current index is reset. If H_FREE_SUB_CRQ fails,
>> + * an error is logged but the channel resources are cleaned up regardless.
>> + */
>> +VISIBLE_IF_KUNIT void ibmvfc_deregister_channel(struct ibmvfc_host *vhost,
>>  				      struct ibmvfc_channels *channels,
>>  				      int index)
>>  {
>
> [Severity: Low]
> Does this cause a build failure when CONFIG_KUNIT is disabled?
>
> Since <kunit/visibility.h> is only included conditionally in ibmvfc.h under
> #if IS_ENABLED(CONFIG_KUNIT), won't the unconditional use of VISIBLE_IF_KUNIT
> here (and EXPORT_SYMBOL_IF_KUNIT below) result in syntax errors during a
> standard build without KUnit?

No, this is not a problem. The only code that calls
ibmvfc_deregister_channel(), which is defined in ibmvfc-core.c, is also
in ibmvfc-core.c in the case where CONFIG_KUNIT is not defined. I've
cleared CONFIG_KUNIT in my config, run "make clean", and built the
kernel, and there were no errors.

> [ ... ]
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.h b/drivers/scsi/ibmvscsi/ibmvfc.h
>> index ca80ceffe53a5..421ce8bc3ea33 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc.h
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc.h
>
> [ ... ]
>
>> +#if IS_ENABLED(CONFIG_KUNIT)
>> +#include <kunit/visibility.h>
>> +VISIBLE_IF_KUNIT void ibmvfc_deregister_channel(struct ibmvfc_host *vhost,
>> +						struct ibmvfc_channels *channels,
>> +						int index);
>> +VISIBLE_IF_KUNIT struct ibmvfc_host *ibmvfc_get_first_vhost(void);
>> +#endif