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

[Dave Marquardt <davemarq@linux.ibm.com>]

sashiko-bot@kernel.org writes:

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

This problem has been mentioned more than once, and I thought it was
fixed. In thinking about this, I've decided to remove fpin_workq from
struct ibmvfc_host, and queue the work items to a system queue using
schedule_work().

The KUnit tests are another problem, which will be resolved by using
each test using its own work queue rather than the system work queue.

-Dave