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

[sashiko-bot@kernel.org]

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.

[ ... ]
> @@ -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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925044900.414340-1-tyreld@linux.ibm.com?part=9