Re: [PATCH v6 1/8] scsi: ibmvfc: add basic FPIN support

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

sashiko-bot@kernel.org writes:

>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index 93b9f699c2e53..22b4d639e74be 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>
> [ ... ]
>
>> @@ -6945,6 +7213,9 @@ static void ibmvfc_remove(struct vio_dev *vdev)
>>  	ENTER;
>>  	ibmvfc_remove_trace_file(&vhost->host->shost_dev.kobj, &ibmvfc_trace_attr);
>>  
>> +	destroy_workqueue(vhost->fpin_workq);
>> +	vhost->fpin_workq = NULL;
>> +
>
> [Severity: High]
> Is the fpin_workq destroyed too early during driver removal?
>
> If an asynchronous FPIN event arrives from the fabric after this point but
> before the CRQ is torn down in ibmvfc_release_crq_queue() later in the
> function, ibmvfc_handle_async() could be called and attempt to queue work:
>
> ibmvfc_handle_async()
>     ...
>     queue_work(vhost->fpin_workq, &aw->async_work_s);
>
> Could this result in a NULL pointer dereference?

Yes, this is a problem. I've fixed it for v7.

>>  	spin_lock_irqsave(vhost->host->host_lock, flags);
>>  	ibmvfc_link_down(vhost, IBMVFC_HOST_OFFLINE);
>>  	spin_unlock_irqrestore(vhost->host->host_lock, flags);
>
> [ ... ]
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
>> new file mode 100644
>> index 0000000000000..64a517a74af8e
>> --- /dev/null
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
>
> [ ... ]
>
>> +static void ibmvfc_async_fpin_test(struct kunit *test)
>> +{
>> +	u64 post[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1];
>> +	u64 pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1];
>> +	enum ibmvfc_ae_fpin_status fs;
>> +	struct fc_host_attrs *fc_host;
>> +	struct ibmvfc_async_crq crq[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1];
>> +	struct ibmvfc_target *tgt;
>> +	struct ibmvfc_host *vhost;
>> +	struct list_head *queue;
>> +	struct list_head *headp;
>> +
>> +	headp = ibmvfc_get_headp();
>> +	if (list_empty(headp))
>> +		kunit_skip(test, "No ibmvfc devices available");
>> +	queue = headp->next;
>> +	vhost = container_of_const(queue, struct ibmvfc_host, queue);
>> +
>> +	KUNIT_ASSERT_GE_MSG(test, vhost->scsi_scrqs.num_targets, 1, "No targets");
>> +	tgt = list_first_entry(&vhost->scsi_scrqs.targets, struct ibmvfc_target, queue);
>
> [Severity: Medium]
> Is it safe to iterate over the global ibmvfc_head list and access
> vhost->scsi_scrqs.targets without acquiring the ibmvfc_driver_lock or
> host_lock?
>
> If a device is unbound or a target logs out concurrently while the test
> runs, could the lists mutate and cause an invalid pointer dereference?

I'm not particularly concerned about this. I'm running the KUnit tests
on a quiet system.

>> +	KUNIT_EXPECT_NOT_NULL(test, tgt->rport);
>> +
>> +	fc_host = shost_to_fc_host(vhost->host);
>> +
>> +	pre[IBMVFC_AE_FPIN_LINK_CONGESTED] = READ_ONCE(fc_host->fpin_stats.cn_device_specific);
>> +	pre[IBMVFC_AE_FPIN_PORT_CONGESTED] = READ_ONCE(tgt->rport->fpin_stats.cn);
>
> [Severity: Medium]
> Since KUNIT_EXPECT_NOT_NULL() allows execution to continue on failure, could a
> NULL tgt->rport be immediately dereferenced on the following lines when reading
> tgt->rport->fpin_stats.cn?
>
> Should this use KUNIT_ASSERT_NOT_NULL() instead to abort the test and prevent a
> crash?

Changed to KUNIT_ASSERT_NOT_NULL() in v7.

>> +	pre[IBMVFC_AE_FPIN_PORT_CLEARED] = READ_ONCE(tgt->rport->fpin_stats.cn_clear);
>> +	pre[IBMVFC_AE_FPIN_PORT_DEGRADED] = READ_ONCE(tgt->rport->fpin_stats.li_failure_unknown);
>> +	pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED] = READ_ONCE(fc_host->fpin_stats.cn_clear);
>> +
>> +	for (fs = IBMVFC_AE_FPIN_LINK_CONGESTED; fs <= IBMVFC_AE_FPIN_CONGESTION_CLEARED; fs++) {
>> +		crq[fs].valid = 0x80;
>> +		crq[fs].link_state = IBMVFC_AE_LS_LINK_UP;
>> +		crq[fs].fpin_status = fs;
>> +		crq[fs].event = cpu_to_be64(IBMVFC_AE_FPIN);
>> +		crq[fs].scsi_id = cpu_to_be64(tgt->scsi_id);
>> +		crq[fs].wwpn = cpu_to_be64(tgt->wwpn);
>> +		crq[fs].node_name = cpu_to_be64(tgt->ids.node_name);
>> +		ibmvfc_handle_async(&crq[fs], vhost);
>> +		msleep(1U);
>> +	}
>
> [Severity: Medium]
> Is msleep(1U) a reliable way to wait for the fpin_workq to process the
> offloaded FPIN event?
>
> Under load, it seems like 1 millisecond might be insufficient, causing the test
> to read stale counters and fail intermittently. Would it be safer to use
> flush_workqueue(vhost->fpin_workq) here instead of a hardcoded sleep?

Good idea. I've changed all of the KUnit tests throughout the series to
use flush_workqueue() rather than msleep() in v7.

-Dave