Re: [PATCH v6 2/8] scsi: ibmvfc: Add NOOP command support

[email protected] writes:

>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index 22b4d639e74be..19a3f12615648 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>
> [ ... ]
>
>> @@ -4220,9 +4230,15 @@ static void ibmvfc_handle_scrq(struct ibmvfc_crq *crq, struct ibmvfc_host *vhost
>>  	case IBMVFC_CRQ_CMD_RSP:
>>  		break;
>>  	case IBMVFC_CRQ_XPORT_EVENT:
>> +	case IBMVFC_NOOP:
>
> [Severity: High]
> Does this code check IBMVFC_NOOP against the correct field?
>
> The switch statement evaluates crq->valid, but IBMVFC_NOOP is a message
> format (0x03).  Will the NOOP case be functionally dead here, causing NOOP
> messages to fall through to the command response handling?

Yep, my mistake. Fixed in v7.

>>  		return;
>>  	default:
>> -		dev_err(vhost->dev, "Got and invalid message type 0x%02x\n", crq->valid);
>> +		dev_err(vhost->dev, "Got an invalid message type 0x%02x\n", crq->valid);
>> +		return;
>> +	}
>> +
>> +	if (unlikely(!evt)) {
>> +		dev_err(vhost->dev, "Received null event\n");
>
> [Severity: High]
> Does this error path get triggered erroneously for every NOOP sent by the
> VIOS?
>
> If a NOOP message falls through to the command response handling, ioba is
> likely zero, which would cause evt to be null.  Will this spam the kernel
> log with 'Received null event' at a high rate and potentially cause disk
> exhaustion?

Fixed in v7 by checking for VFC_NOOP after the switch.

> [Severity: High]
> This is a pre-existing issue, but is it safe to dereference evt->queue
> before validation?
>
> Immediately after this null check, the code executes:
>
>         if (unlikely(!ibmvfc_valid_event(&evt->queue->evt_pool, evt))) {

There's

        if (unlikely(!evt)) {
        .
        .
        .

above. So this *HAS* been validated.

> [Severity: High]
> Could the destruction of vhost->fpin_workq during module removal cause a
> use-after-free?
>
> In ibmvfc_remove():
>
>         destroy_workqueue(vhost->fpin_workq);
>         vhost->fpin_workq = NULL;
>
> If hardware interrupts and tasklets are not disabled until
> ibmvfc_release_crq_queue() is called later, an IBMVFC_AE_FPIN event could
> trigger the interrupt handler and schedule the tasklet.
>
> Would this result in ibmvfc_handle_async() executing:
>
>         queue_work(vhost->fpin_workq, &aw->async_work_s);
>
> on the explicitly destroyed and NULL-ed workqueue?

Fixed in patch 1 in v7.

-Dave