Re: [PATCH v6 6/8] scsi: ibmvfc: extend channel registration and deregistration for async subq

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Kernel panic due to workqueue use-after-free during device teardown.
- [Medium] Silent behavioral mutations: Undocumented parsing changes and test alterations.
- [Low] Dead code and incomplete feature: async_sub_crq is never registered and lacks allocation.
--

--- Patch [6]: [PATCH v6 6/8] scsi: ibmvfc: extend channel registration and deregistration for async subq ---
commit 31a5fe949f10ffb5d84b0f4c1f35d59e3b5dc785
Author: Dave Marquardt <[email protected]>
Subject: scsi: ibmvfc: extend channel registration and deregistration for async subq

This patch extends ibmvfc_register_channel() and ibmvfc_deregister_channel()
to accept a negative index. This convention signals that the async sub-CRQ
should be operated on instead of an indexed scrq entry.

> Extend both functions to accept a negative index as a sentinel value
> signalling that the async sub-CRQ should be operated on instead of an
> indexed scrq entry. When index < 0, the queue pointer is set to
> &vhost->async_sub_crq, the IRQ is named "ibmvfc-<addr>-async", and the
> handler is set to ibmvfc_interrupt_async_subq rather than the per-protocol
> ibmvfc_interrupt_mq handler. hwq_id assignment is skipped for the async
> queue since it has no meaningful hardware queue index.

[Severity: Medium]
Could the commit message be updated to mention the behavioral updates?

The commit message thoroughly explains the new channel registration logic,
but the patch also introduces undocumented parsing changes to node_name
handling using IBMVFC_ASYNC_ID_IS_ASSOC_ID in ibmvfc_process_async_work(),
along with silent alterations to the KUnit tests checking
cn_device_specific.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 3b5732f2fb6ec..4e6d2ff190751 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
[ ... ]
> @@ -3593,7 +3593,7 @@ VISIBLE_IF_KUNIT void ibmvfc_handle_async(void *crq,
>  		link_state = subq->link_state;
>  		scsi_id = 0;
>  		wwpn = subq->wwpn;
> -		node_name = subq->id.node_name;
> +		node_name = subq->flags & IBMVFC_ASYNC_ID_IS_ASSOC_ID ? 0 : subq->id.node_name;

[Severity: Critical]
Could this lead to a use-after-free regression during device teardown?

If ibmvfc_remove() destroys and nullifies vhost->fpin_workq before the
adapter interrupts are disabled via ibmvfc_release_crq_queue(), and an
asynchronous FPIN event is received during this window,
ibmvfc_handle_async() will invoke:

    queue_work(vhost->fpin_workq, &aw->async_work_s);

Since the workqueue pointer is already NULL, this would cause a fatal
dereference.

[ ... ]
> @@ -6798,13 +6791,29 @@ 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)
>  {
>  	struct device *dev = vhost->dev;
>  	struct vio_dev *vdev = to_vio_dev(dev);
> -	struct ibmvfc_queue *scrq = &channels->scrqs[index];
> +	bool is_async = index < 0;
> +	struct ibmvfc_queue *scrq = !is_async ? &channels->scrqs[index] : &vhost->async_sub_crq;

[Severity: Low]
Is this negative index path reachable?

Searching the codebase, it doesn't appear that any callers pass a negative
index into ibmvfc_register_channel() or ibmvfc_deregister_channel().

Additionally, if this path were reached, it looks like vhost->async_sub_crq
is never initialized via ibmvfc_alloc_queue(). This could lead to passing an
uninitialized msg_token to the hypervisor and a null pointer dereference
during memset().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6