Re: [PATCH v14 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: race reading uninitialized hwq_id on early interrupt
> --
>
> commit 305244f5c1aeffae812f2cadfc3586d45148fbbe
> Author: Dave Marquardt <davemarq@linux.ibm.com>
>
> scsi: ibmvfc: extend channel reg/dereg helpers for async sub-CRQ
>     
> This commit extends ibmvfc_register_channel() and ibmvfc_deregister_channel()
> to handle IBMVFC_PROTO_ASYNC channels. It removes the __maybe_unused
> annotation from ibmvfc_interrupt_async_subq and updates error messages to
> include the channel protocol.
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index fc69012efccc0..b12655f028879 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>
> [ ... ]
>
>> @@ -6882,7 +6901,8 @@ static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
>>  	rc = request_irq(scrq->irq, scrq->handler, 0, scrq->name, scrq);
>
> [Severity: Low]
> Is there a potential race condition here when the interrupt is requested?
>
> If a hardware interrupt triggers immediately after request_irq() enables it,
> the newly bound handler ibmvfc_interrupt_async_subq() could run before
> scrq->hwq_id is initialized:
>
> ibmvfc_register_channel()
>     ...
>     rc = request_irq(scrq->irq, scrq->handler, 0, scrq->name, scrq);
>     ...
>     scrq->hwq_id = index;
>
> If an early interrupt happens, the handler calls ibmvfc_toggle_scrq_irq(),
> which uses scrq->hwq_id for error logging if the hypervisor call fails.
>
> Could this result in reading an uninitialized (zeroed) scrq->hwq_id in the
> dev_err() message? Would it be safer to initialize scrq->hwq_id before
> calling request_irq()?

Yes, it would be safer. Consequences, as you say, are low, as hwq_id is
used for error logging in ibmvfc_toggle_scrq_irq(). In the case
described, it would log "sub-crq[0]" rather than the correct sub-crq index.

Will fix in v15.

-Dave