Re: [PATCH v3 7/8] ibmvfc: register and use asynchronous sub-queue for events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Missing `host_lock` acquisition in `ibmvfc_drain_async_subq` leads to data races and kernel panics.
- [High] Invalid pointer dereference due to `list_for_each_entry_safe` cursor leak in `ibmvfc_process_async_work` triggers a kernel panic.
- [Medium] Protocol violation: `IBMVFC_USE_ASYNC_SUBQ` capability is unconditionally advertised to the VIOS.
- [Medium] Memory leak of `channels->async_scrq` in `ibmvfc_release_channels`.
- [Low] Incorrect queue naming in `ibmvfc_channel_index` due to `active_queues` being 0 during registration.
- [Low] The `async_scrq` is not assigned an `hwq_id`, leaving it at 0 and causing misleading debug logs.
--

commit 1426674b04407a5087c581ceae1dd979f487b41b
Author: Dave Marquardt <davemarq@linux.ibm.com>

ibmvfc: register and use asynchronous sub-queue for events

Set up interrupt handling for asynchronous sub-queue, register the
asynchronous sub-queue as a channel, and enable its use by NPIV login.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> index 4678d76c84fe2..36abca0bbd34d 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
> @@ -1514,7 +1514,8 @@ static void ibmvfc_set_login_info(struct ibmvfc_host *vhost)
>  	login_info->max_cmds = cpu_to_be32(max_cmds);
>  	login_info->capabilities =
>  		cpu_to_be64(IBMVFC_CAN_MIGRATE | IBMVFC_CAN_SEND_VF_WWPN |
> -			    IBMVFC_CAN_USE_NOOP_CMD);
> +			    IBMVFC_CAN_USE_NOOP_CMD | IBMVFC_YES_SCSI |
> +			    IBMVFC_USE_ASYNC_SUBQ | IBMVFC_CAN_HANDLE_FPIN);

[Severity: Medium]
Should IBMVFC_USE_ASYNC_SUBQ only be advertised when vhost->mq_enabled is
true?

If the user disables MQ, the async sub-queue is never registered, but the
VIOS might still attempt to send async sub-queue formatted events, falling
back to the legacy async_crq. The driver processes legacy CRQ events with
is_subq=false expecting the legacy layout, which could lead to incorrect
event processing.

>  
>  	if (vhost->mq_enabled || vhost->using_channels)
>  		login_info->capabilities |= cpu_to_be64(IBMVFC_CAN_USE_CHANNELS);

[ ... ]

> @@ -4235,6 +4236,49 @@ static struct ibmvfc_crq *ibmvfc_next_scrq(struct ibmvfc_queue *scrq)
>  	return crq;
>  }
>  
> +static void ibmvfc_drain_async_subq(struct ibmvfc_queue *scrq)
> +{
> +	struct ibmvfc_crq *crq;
> +	unsigned long flags;
> +	int done = 0;
> +
> +	spin_lock_irqsave(scrq->q_lock, flags);
> +	while (!done) {
> +		while ((crq = ibmvfc_next_scrq(scrq)) != NULL) {
> +			ibmvfc_handle_async(crq, scrq->vhost, true);

[Severity: High]
Does this code call ibmvfc_handle_async() without holding the host_lock?

ibmvfc_handle_async() can modify host state and invoke functions like
__ibmvfc_reset_host(), which traverses vhost->targets. The global
vhost->host->host_lock is required to protect these lists. Executing them
concurrently without it could result in list corruption and kernel panics.

> +			crq->valid = 0;
> +			wmb();	/* complete write */
> +		}
> +
> +		ibmvfc_toggle_scrq_irq(scrq, 1);
> +		crq = ibmvfc_next_scrq(scrq);
> +		if (crq != NULL) {
> +			ibmvfc_toggle_scrq_irq(scrq, 0);
> +			ibmvfc_handle_async(crq, scrq->vhost, true);
> +			crq->valid = 0;
> +			wmb();	/* complete write */
> +		} else
> +			done = 1;
> +	}
> +	spin_unlock_irqrestore(scrq->q_lock, flags);
> +}
> +

[ ... ]

> @@ -6331,14 +6375,27 @@ static int ibmvfc_init_crq(struct ibmvfc_host *vhost)
>  	return retrc;
>  }
>  
> -static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
> -				   struct ibmvfc_channels *channels,
> -				   int index)
> +static inline char *ibmvfc_channel_index(struct ibmvfc_channels *channels,
> +					 struct ibmvfc_queue *scrq,
> +					 char *buf, size_t bufsize)
> +{
> +	if (scrq < channels->scrqs || scrq >= channels->scrqs + channels->active_queues)

[Severity: Low]
When ibmvfc_reg_sub_crqs() is executed during probe, is
channels->active_queues still 0 here?

If so, the condition will evaluate to true for all SCSI hardware queues,
incorrectly naming all sub-queues as "ibmvfc-<unit>-scsiasync" instead of
assigning their numeric index. This could break /proc/interrupts
identification.

> +		strscpy(buf, "async", 6);
> +	else
> +		snprintf(buf, bufsize, "%ld", scrq - channels->scrqs);
> +	return buf;
> +}
> +
> +static int ibmvfc_register_channel_handler(struct ibmvfc_host *vhost,

[ ... ]

> @@ -6381,32 +6441,43 @@ static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
>  	rc = request_irq(scrq->irq, scrq->handler, 0, scrq->name, scrq);
>  
>  	if (rc) {
> -		dev_err(dev, "Couldn't register sub-crq[%d] irq\n", index);
> +		dev_err(dev, "Couldn't register sub-crq[%s] irq\n",
> +			ibmvfc_channel_index(channels, scrq, buf, sizeof(buf)));
>  		irq_dispose_mapping(scrq->irq);
> +		scrq->irq = 0;
>  		goto irq_failed;
>  	}
>  
> -	scrq->hwq_id = index;
> +	if (scrq >= channels->scrqs && scrq < channels->scrqs + channels->max_queues)
> +		scrq->hwq_id = scrq - channels->scrqs;

[Severity: Low]
Does this exclude channels->async_scrq from receiving an hwq_id?

Since it is allocated as a standalone struct outside the scrqs array, its
hwq_id would remain 0. Subsequent errors on the async queue would log
against sub-crq[0], which could mislead debugging efforts toward the first
SCSI queue.

>  
>  	LEAVE;
>  	return 0;

[ ... ]

> @@ -6461,7 +6544,8 @@ static void ibmvfc_dereg_sub_crqs(struct ibmvfc_host *vhost,
>  		return;
>  
>  	for (i = 0; i < channels->max_queues; i++)
> -		ibmvfc_deregister_channel(vhost, channels, i);
> +		ibmvfc_deregister_channel(vhost, channels, &channels->scrqs[i]);
> +	ibmvfc_deregister_channel(vhost, channels, channels->async_scrq);
>  
>  	LEAVE;
>  }

[Severity: Medium]
Since channels->async_scrq is dynamically allocated, does
ibmvfc_release_channels() leak this memory?

When the driver tears down the channels in ibmvfc_release_channels(), it
calls ibmvfc_free_queue(vhost, channels->async_scrq) and then sets
channels->async_scrq = NULL without ever calling kfree(), which permanently
leaks the struct memory.

[Severity: High]
This is a pre-existing issue, but I have a question about cursor leaks in
ibmvfc_process_async_work().

ibmvfc_process_async_work() searches the vhost->targets list using
list_for_each_entry_safe(). If no target is found, the 'tgt' cursor exits
the loop pointing to the list head, not NULL. Does the subsequent check:

    if (!tgt || !tgt->rport) {

evaluate this leaked cursor? Because tgt->rport evaluates to an
out-of-bounds offset that aliases vhost->sg_pool, the check might
incorrectly succeed. The function would then access out-of-bounds
properties to process an event, feeding a corrupted pointer into
fc_host_fpin_rcv() and causing a kernel panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-ibmvfc-fpin-support-v3-0-d95b9547cf88@linux.ibm.com?part=7