Re: [PATCH v13 04/11] scsi: ibmvfc: add basic FPIN support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: ibmvfc: NULL pointer dereference in queue_work on FPIN event during remove
--

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

scsi: ibmvfc: add basic FPIN support

Implement support for a basic level of Fabric Performance Impact Notifications
(FPIN) in the ibmvfc driver to enable monitoring of fabric congestion and link
integrity events. This adds an async event handler for IBMVFC_AE_FPIN events
that offloads processing to a dedicated workqueue.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -3361,11 +3609,25 @@ static void ibmvfc_handle_async(struct ibmvfc_async_crq *crq,
>  	case IBMVFC_AE_HALT:
>  		ibmvfc_link_down(vhost, IBMVFC_HALTED);
>  		break;
> +	case IBMVFC_AE_FPIN:
> +		if (vhost->state == IBMVFC_HOST_OFFLINE)
> +			break;
> +		aw = kzalloc_obj(*aw, GFP_ATOMIC);
> +		if (aw) {
> +			INIT_WORK(&aw->async_work_s, ibmvfc_process_async_work);
> +			aw->vhost = vhost;
> +			aw->crq = *crq;
> +			queue_work(vhost->fpin_workq, &aw->async_work_s);

[Severity: High]
Does this code risk a NULL pointer dereference or use-after-free during driver
removal?

During ibmvfc_remove(), vhost->fpin_workq is destroyed and set to NULL before
the CRQ interrupts are disabled and the tasklet is killed (which occurs later
in ibmvfc_release_crq_queue()).

If a transport event (IBMVFC_CRQ_XPORT_EVENT) arrives concurrently,
ibmvfc_handle_crq() unconditionally overwrites vhost->state to IBMVFC_NO_CRQ.
This erases the IBMVFC_HOST_OFFLINE state set earlier by ibmvfc_remove().

If an IBMVFC_AE_FPIN event is subsequently processed by the still-active
tasklet, the vhost->state == IBMVFC_HOST_OFFLINE safety check here in
ibmvfc_handle_async() fails to block it. This causes queue_work() to be
called with a NULL or already-destroyed fpin_workq pointer.

> +		} else
> +			dev_err_ratelimited(vhost->dev,
> +					    "can't offload async CRQ to work queue\n");
> +		break;
>  	default:

[ ... ]

> @@ -6958,6 +7229,8 @@ static void ibmvfc_remove(struct vio_dev *vdev)
>  	ibmvfc_wait_while_resetting(vhost);
>  	kthread_stop(vhost->work_thread);
>  	flush_work(&vhost->rport_add_work_q);
> +	destroy_workqueue(vhost->fpin_workq);
> +	vhost->fpin_workq = NULL;
>  	fc_remove_host(vhost->host);
>  	scsi_remove_host(vhost->host);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007-ibmvfc-fpin-support-v13-0-b816abfa4fcc@linux.ibm.com?part=4