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

[Kevin Wolf <kwolf@redhat.com>]

Am 23.12.2019 um 18:51 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>

The time this test takes is much better now (~5s for me).

> +/*
> + * 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);
> +    int sectors = 0;
> +    uint32_t size = 0;
> +
> +    for (sectors = 1; sectors <= 256; sectors *= 2) {
> +        QTestState *qts = NULL;
> +        QPCIDevice *dev = NULL;
> +        QPCIBar bmdma_bar, ide_bar;
> +
> +        qts = test_bmdma_setup();
> +        dev = get_pci_device(qts, &bmdma_bar, &ide_bar);

I'm wondering why the initialisation has to be inside the outer for
loop. I expected that moving it outside would further improve the speed.
But sure enough, doing that makes the test fail.

Did you have a look why this happens? I suppose we might be running out
of some resources in the qtest framework becasue each send_dma_request()
calls get_pci_device() again?

5 seconds isn't that bad, so this shouldn't block this series, but it's
still by far the slowest test in ide-test, so any improvement certainly
wouldn't hurt.

> +        for (size = 0; size < 65536; size += 256) {
> +            uint32_t req_size = sectors * 512;
> +            uint32_t prd_size = size & 0xfffe; /* bit 0 is always set to 0 */
> +            uint8_t ret = 0;
> +            uint8_t req_status = 0;

If you end up sending another version for some reason, I would also
consider renaming req_status, because reg_status already exists, which
looks almost the same. This confused me for a moment when reading the
code below.

Kevin