Re: [PATCH v16 03/12] 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 when CONFIG_KUNIT is disabled
> --
>
> commit e6767e190b474e7d3a6e33d2ed96de8dbb05ff1a
> 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 freeing an
> unregistered sub-CRQ or clearing an unallocated message buffer, and adds a
> KUnit test suite to verify the guard conditions.
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index 145d559041f12..f0502d7482105 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> @@ -6450,11 +6450,23 @@ static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
>>  		hcall_rc = plpar_hcall_norets(H_FREE_SUB_CRQ, vdev->unit_address, scrq->cookie);
>>  	} while (hcall_rc == H_BUSY || H_IS_LONG_BUSY(hcall_rc));
>>  reg_failed:
>> +	scrq->cookie = 0;
>>  	LEAVE;
>>  	return rc;
>>  }
>>  
>> -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)
>>  {
>> @@ -6471,20 +6483,28 @@ static void ibmvfc_deregister_channel(struct ibmvfc_host *vhost,
>>  		scrq->irq = 0;
>>  	}
>>  
>> -	do {
>> -		rc = plpar_hcall_norets(H_FREE_SUB_CRQ, vdev->unit_address,
>> -					scrq->cookie);
>> -	} while (rc == H_BUSY || H_IS_LONG_BUSY(rc));
>> +	if (scrq->cookie) {
>> +		do {
>> +			rc = plpar_hcall_norets(H_FREE_SUB_CRQ, vdev->unit_address,
>> +						scrq->cookie);
>> +		} while (rc == H_BUSY || H_IS_LONG_BUSY(rc));
>>  
>> -	if (rc)
>> -		dev_err(dev, "Failed to free sub-crq[%d]: rc=%ld\n", index, rc);
>> +		if (rc)
>> +			dev_err(dev, "Failed to free protocol (%d) sub-crq[%d]: rc=%ld\n",
>> +				channels->protocol, index, rc);
>> +		else
>> +			scrq->cookie = 0;
>> +	}
>>  
>>  	/* Clean out the queue */
>> -	memset(scrq->msgs.crq, 0, PAGE_SIZE);
>> -	scrq->cur = 0;
>> +	if (scrq->msgs.handle) {
>> +		memset(scrq->msgs.crq, 0, PAGE_SIZE);
>> +		scrq->cur = 0;
>> +	}
>>  
>>  	LEAVE;
>>  }
>> +EXPORT_SYMBOL_IF_KUNIT(ibmvfc_deregister_channel);
>
> [ ... ]
>
>> 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
>> @@ -1088,4 +1088,12 @@ static inline struct ibmvfc_host *ibmvfc_channels_to_vhost(struct ibmvfc_channel
>>  #define ibmvfc_remove_trace_file(kobj, attr) do { } while (0)
>>  #endif
>>  
>> +#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
>
> [Severity: Low]
> Does this break the build when CONFIG_SCSI_IBMVFC is enabled and CONFIG_KUNIT
> is disabled?
>
> In drivers/scsi/ibmvscsi/ibmvfc-core.c, ibmvfc_deregister_channel() uses
> VISIBLE_IF_KUNIT and EXPORT_SYMBOL_IF_KUNIT(ibmvfc_deregister_channel)
> outside of any CONFIG_KUNIT guard.
>
> However, drivers/scsi/ibmvscsi/ibmvfc.h only includes <kunit/visibility.h>
> when CONFIG_KUNIT is enabled, so the fallback definitions in
> include/kunit/visibility.h:
>
> #else
>     #define VISIBLE_IF_KUNIT static
>     #define EXPORT_SYMBOL_IF_KUNIT(symbol)
> #endif
>
> are never pulled in when CONFIG_KUNIT is disabled, causing compilation of
> drivers/scsi/ibmvscsi/ibmvfc-core.c to fail due to unknown macros.
>
> A later commit in the series, commit 1bc41de0e60f ("scsi: ibmvfc: add basic
> FPIN support"), adds an unconditional #include <kunit/visibility.h> to
> drivers/scsi/ibmvscsi/ibmvfc-core.c, though leaving this commit and commit
> 1b37c48a1902 ("scsi: ibmvfc: add link state to host struct") unbuildable with
> CONFIG_KUNIT=n during bisection.
>
> Should <kunit/visibility.h> be included unconditionally in
> drivers/scsi/ibmvscsi/ibmvfc.h (or drivers/scsi/ibmvscsi/ibmvfc-core.c) in
> this commit?

I'm configured a kernel build pseries_le_defconfig and then turned
CONFIG_KUNIT off. Indeed, there's an error when building ibmvfc-core.c.

I tried including <kunit/visibility.h> unconditionally in ibmvfc.h, but
that caused build warnings for ibmvfc-nvme.c about unused
functions. Including <kunit/visibility.h> in ibmvfc-core.c resolved the
issue.

This will be the fix in v17.

-Dave