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

[email protected] writes:

> 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.

The commit message is updated in v7.

>> 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.

Already fixed in previous patches in v7.

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

This is fixed in v7.

-Dave