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

[Kevin Wolf <kwolf@redhat.com>]

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

> +/*
> + * 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.

> +        /*
> +         * 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.

Kevin