Re: [PATCH v2 1/2] tests/ide-test: Create a single unit-test covering more PRDT cases

[Alexander Popov <alex.popov@linux.com>]

Hello Kevin,

Thanks for your review!

On 19.12.2019 18:12, Kevin Wolf wrote:
> Am 16.12.2019 um 19:14 hat Alexander Popov geschrieben:
>> Fuzzing the Linux kernel with syzkaller allowed to find how to crash qemu
>> using a special SCSI_IOCTL_SEND_COMMAND. It hits the assertion in
>> ide_dma_cb() introduced in the commit a718978ed58a in July 2015.
>> Currently this bug is not reproduced by the unit tests.
>>
>> Let's improve the ide-test to cover more PRDT cases including one
>> that causes this particular qemu crash.
>>
>> The test is developed according to the Programming Interface for
>> Bus Master IDE Controller (Revision 1.0 5/16/94).
>>
>> Signed-off-by: Alexander Popov <alex.popov@linux.com>
> 
> Looks mostly good to me, but I have a few comments.
> 
> First of all, the patch order needs to be reversed to keep the tree
> bisectable (first fix the bug, then test that it's fixed).

Ok, I'll do that.

>> +/*
>> + * This test is developed according to the Programming Interface for
>> + * Bus Master IDE Controller (Revision 1.0 5/16/94)
>> + */
>> +static void test_bmdma_various_prdts(void)
>>  {
>> -    QTestState *qts;
>> -    QPCIDevice *dev;
>> -    QPCIBar bmdma_bar, ide_bar;
>> -    uint8_t status;
>> -
>> -    PrdtEntry prdt[] = {
>> -        {
>> -            .addr = 0,
>> -            .size = cpu_to_le32(0x1000 | PRDT_EOT),
>> -        },
>> -    };
>> -
>> -    qts = test_bmdma_setup();
>> -
>> -    dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
>> -
>> -    /* Normal request */
>> -    status = send_dma_request(qts, CMD_READ_DMA, 0, 1,
>> -                              prdt, ARRAY_SIZE(prdt), NULL);
>> -    g_assert_cmphex(status, ==, BM_STS_ACTIVE | BM_STS_INTR);
>> -    assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
>> +    uint32_t size = 0;
>> +    uint32_t prd_size = 0;
>> +    int req_sectors = 0;
>> +    uint32_t req_size = 0;
>> +    uint8_t s1 = 0, s2 = 0;
>> +
>> +    for (size = 0; size < 65536; size += 256) {
> 
> We're testing 64 * 4 = 256 cases here, each of them starting a new qemu
> process. Do we actually test anything new after the first couple of
> requests or does this just make the test slower than it needs to be?
> 
> This test case really takes a long time for me (minutes), whereas all
> other cases in ide-test combined run in like a second.
> 
> I would either test much less different sizes or at least run them in
> the same qemu process. Or both, of course.

Yes, it takes 3 minutes to run this test on my laptop.

Thanks for the idea. I'll try to run all the requests in a single qemu process.

>> +        /*
>> +         * Two bytes specify the count of the region in bytes.
>> +         * The bit 0 is always set to 0.
>> +         * A value of zero in these two bytes indicates 64K.
>> +         */
>> +        prd_size = size & 0xfffe;
>> +        if (prd_size == 0) {
>> +            prd_size = 65536;
>> +        }
>>  
>> -    /* Abort the request before it completes */
>> -    status = send_dma_request(qts, CMD_READ_DMA | CMDF_ABORT, 0, 1,
>> -                              prdt, ARRAY_SIZE(prdt), NULL);
>> -    g_assert_cmphex(status, ==, BM_STS_INTR);
>> -    assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
>> -    free_pci_device(dev);
>> -    test_bmdma_teardown(qts);
>> +        for (req_sectors = 1; req_sectors <= 256; req_sectors *= 2) {
>> +            req_size = req_sectors * 512;
>> +
>> +            /*
>> +             * 1. If PRDs specified a smaller size than the IDE transfer
>> +             * size, then the Interrupt and Active bits in the Controller
>> +             * status register are not set (Error Condition).
>> +             *
>> +             * 2. If the size of the physical memory regions was equal to
>> +             * the IDE device transfer size, the Interrupt bit in the
>> +             * Controller status register is set to 1, Active bit is set to 0.
>> +             *
>> +             * 3. If PRDs specified a larger size than the IDE transfer size,
>> +             * the Interrupt and Active bits in the Controller status register
>> +             * are both set to 1.
>> +             */
>> +            if (prd_size < req_size) {
>> +                s1 = 0;
>> +                s2 = 0;
>> +            } else if (prd_size == req_size) {
>> +                s1 = BM_STS_INTR;
>> +                s2 = BM_STS_INTR;
>> +            } else {
>> +                s1 = BM_STS_ACTIVE | BM_STS_INTR;
>> +                s2 = BM_STS_INTR;
>> +            }
>> +            test_bmdma_prdt(size, req_sectors, s1, s2);
>> +        }
>> +    }
>>  }
> 
> And finally, as mentioned in the reply for patch 2, I wonder if we
> should add a case with an empty PRDT (passing 0 as the PRDT size). This
> would be a separate patch, though.

Do you mean zero PRD size here?

The specification says that a value of zero in PRD size indicates 64K.
That's why we have the following code in bmdma_prepare_buf():
    len = prd.size & 0xfffe;
    if (len == 0)
        len = 0x10000;

That case is already tested in my version. Let me quote the code above:
>> +    for (size = 0; size < 65536; size += 256) {

Best regards,
Alexander