Re: [PATCH v10 8/9] scsi: ibmvfc: register and use asynchronous sub CRQ for events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Unconditional register, deregister, and free operations on `async_sub_crq` without checking allocation status lead to double-frees, DMA API violations, and invalid hypercalls.
--

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

scsi: ibmvfc: register and use asynchronous sub CRQ for events

Wire the async sub-CRQ into the sub-CRQ lifecycle so it is allocated,
registered, and freed alongside the SCSI channel queues.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index c78e221f182a2..70a3046135c7b 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -964,6 +966,7 @@ static int ibmvfc_reenable_crq_queue(struct ibmvfc_host *vhost)
>  	struct vio_dev *vdev = to_vio_dev(vhost->dev);
>  	unsigned long flags;
>  
> +	ibmvfc_deregister_channel(vhost, &vhost->scsi_scrqs, -1);
>  	ibmvfc_dereg_sub_crqs(vhost, &vhost->scsi_scrqs);
>  	ibmvfc_dereg_sub_crqs(vhost, &vhost->nvme_scrqs);

[Severity: Critical]
If multi-queue is disabled or failed to initialize, vhost->async_sub_crq
won't be allocated.  Will this unconditionally deregister an unallocated queue?

[ ... ]
> @@ -1042,6 +1047,7 @@ static int ibmvfc_reset_crq(struct ibmvfc_host *vhost)
>  	spin_unlock(vhost->crq.q_lock);
>  	spin_unlock_irqrestore(&vhost->host->host_lock, flags);
>  
> +	ibmvfc_register_channel(vhost, &vhost->scsi_scrqs, -1);
>  	ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs);
>  	ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs);

[Severity: Critical]
Will unconditionally registering the async queue here pass an uninitialized
msg_token to the hypervisor if the queue was never allocated?

[ ... ]
> @@ -6953,8 +6964,10 @@ static void ibmvfc_deregister_channel(struct ibmvfc_host *vhost,
>  	}
>  
>  	/* Clean out the queue */
> -	memset(scrq->msgs.crq, 0, PAGE_SIZE);
> -	scrq->cur = 0;
> +	if (scrq->msgs.handle) {
> +		memset(scrq->msgs.crq, 0, PAGE_SIZE);
> +		scrq->cur = 0;
> +	}

[Severity: Critical]
The commit message notes this memset is guarded to prevent a NULL dereference
when the queue was never allocated.  However, earlier in this function
plpar_hcall_norets(H_FREE_SUB_CRQ, ...) is called using scrq->cookie.

Does this hcall also need to be guarded to avoid passing an uninitialized
cookie?

[ ... ]
> @@ -7046,6 +7071,15 @@ static void ibmvfc_init_sub_crqs(struct ibmvfc_host *vhost)
>  	}
>  
>  	LEAVE;
> +	return;
> +
> + deregister_async_sub_crq:
> +	ibmvfc_deregister_channel(vhost, &vhost->scsi_scrqs, -1);
> +free_async_sub_crq:
> +	ibmvfc_free_queue(vhost, &vhost->async_sub_crq);

[Severity: Critical]
If ibmvfc_alloc_channels() fails, we deregister and free async_sub_crq
here.  Later, won't ibmvfc_release_sub_crqs() attempt to unconditionally
free it again?

> +	vhost->do_enquiry = 0;
> +	vhost->mq_enabled = 0;
> +	return;
>  }
>  
>  static void ibmvfc_release_channels(struct ibmvfc_host *vhost,
> @@ -7069,6 +7103,9 @@ static void ibmvfc_release_channels(struct ibmvfc_host *vhost,
>  static void ibmvfc_release_sub_crqs(struct ibmvfc_host *vhost)
>  {
>  	ENTER;
> +	ibmvfc_deregister_channel(vhost, &vhost->scsi_scrqs, -1);
> +	ibmvfc_free_queue(vhost, &vhost->async_sub_crq);
> +

[Severity: Critical]
Since async_sub_crq might not have been allocated (e.g., if mq_enabled
is false) or might have been freed during a prior initialization failure,
could this result in a double-free or freeing of uninitialized memory?

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