Re: [PATCH v11 09/11] scsi: ibmvfc: extend channel reg/dereg helpers for async sub-CRQ

[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: kernel-doc incorrectly instructs passing negative index
> --
>
> commit 6798e7ba7398cb1f91e10a6432ae9b3192653f37
> Author: Dave Marquardt <davemarq@linux.ibm.com>
>
> scsi: ibmvfc: extend channel reg/dereg helpers for async sub-CRQ
>
> ibmvfc_register_channel() and ibmvfc_deregister_channel() previously only
> handled SCSI and NVMe sub-CRQ channels identified by the channel protocol.
> The async sub-CRQ had no registration path through these helpers, requiring
> separate handling. This commit adds IBMVFC_PROTO_ASYNC to extend both
> functions to handle the async sub-CRQ.
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index b354bfb1a9b67..b967c82e99430 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> [ ... ]
>> @@ -6812,6 +6812,21 @@ static int ibmvfc_init_crq(struct ibmvfc_host *vhost)
>>  	return retrc;
>>  }
>>  
>> +/**
>> + * ibmvfc_register_channel - Register a sub-CRQ channel with the hypervisor
>> + * @vhost:	ibmvfc host struct
>> + * @channels:	ibmvfc channels struct containing the channel array and protocol
>> + * @index:	index into the channels array for the queue to register, or
>> + *		a negative value to register the async sub-CRQ
>> + *
>> + * Register a sub-CRQ with the hypervisor via h_reg_sub_crq, map its hardware
>> + * IRQ to a Linux IRQ, and bind an interrupt handler to it. The handler is
>> + * selected based on the channel protocol (SCSI or NVMe) for normal queues, or
>> + * set to the async sub-CRQ handler when @index is negative.
>> + *
>> + * Return value:
>> + *	0 on success / non-zero on failure
>> + **/
>>  static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
>>  				   struct ibmvfc_channels *channels,
>>  				   int index)
>
> [Severity: Low]
> Does this kernel-doc correctly document the expected index values?
>
> The documentation explicitly directs the caller to pass a negative index for
> async sub-CRQs, but ibmvfc_register_channel() begins with an unconditional
> array access:
>
>     struct ibmvfc_queue *scrq = &channels->scrqs[index];
>
> If a developer follows the documentation and passes a negative index, wouldn't
> this result in an immediate out-of-bounds array access? Currently, callers
> pass 0 for the async sub-CRQ, so this crash isn't reachable today, but the
> comment could mislead future users.

Right. This suggestion to use a negative index is an artifact of older
implementation choices and was missed in more recent patch
revisions. I'll fix it in v12.

> [ ... ]
>> @@ -6883,6 +6905,18 @@ static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
>>  	return rc;
>>  }
>>  
>> +/**
>> + * 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, or -1 to deregister the
>> + *		asynchronous sub-CRQ
>> + *
>> + * 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.
>> + */
>>  static void ibmvfc_deregister_channel(struct ibmvfc_host *vhost,
>>  				      struct ibmvfc_channels *channels,
>>  				      int index)
>
> [Severity: Low]
> Is this the same documentation issue as above?
>
> The kernel-doc for ibmvfc_deregister_channel() directs using -1 for the
> asynchronous sub-CRQ, but similar to ibmvfc_register_channel(), it accesses
> the array unconditionally:
>
>     struct ibmvfc_queue *scrq = &channels->scrqs[index];
>
> Could this lead to an out-of-bounds array access if someone passes -1 as
> documented?

I'll fix this also.

-Dave