Re: [Qemu-devel] [PATCH PULL v2 09/10] hw/rdma: Implementation of PVRDMA device

[Marcel Apfelbaum]

On 27/04/2018 17:55, Peter Maydell wrote:
> On 19 February 2018 at 11:43, Marcel Apfelbaum <address@hidden> wrote:
>> From: Yuval Shaia <address@hidden>
>>
>> PVRDMA is the QEMU implementation of VMware's paravirtualized RDMA device.
>> It works with its Linux Kernel driver AS IS, no need for any special
>> guest modifications.
>>
>> While it complies with the VMware device, it can also communicate with
>> bare metal RDMA-enabled machines and does not require an RDMA HCA in the
>> host, it can work with Soft-RoCE (rxe).
>>
>> It does not require the whole guest RAM to be pinned allowing memory
>> over-commit and, even if not implemented yet, migration support will be
>> possible with some HW assistance.
>>
>> Implementation is divided into 2 components, rdma general and pvRDMA
>> specific functions and structures.
>>
>> The second PVRDMA sub-module - interaction with PCI layer.
>> - Device configuration and setup (MSIX, BARs etc).
>> - Setup of DSR (Device Shared Resources)
>> - Setup of device ring.
>> - Device management.
>>
>> Reviewed-by: Dotan Barak <address@hidden>
>> Reviewed-by: Zhu Yanjun <address@hidden>
>> Signed-off-by: Yuval Shaia <address@hidden>
>> Signed-off-by: Marcel Apfelbaum <address@hidden>
> 
> 
>> +static void free_ports(PVRDMADev *dev)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < MAX_PORTS; i++) {
>> +        g_free(dev->rdma_dev_res.ports[i].gid_tbl);
> 
> Coverity (CID 1390628) points out that this is attempting to
> call free on an array, which is not valid...
>

Definitely a bug, thanks! (git_tbl is part of RdmaRmPort,
it makes no sense to free it)

We didn't catch it because, funny thing,
the QOM 'exit' function is not called when QEMU exits
for any device created with "-device".

[Adding Michael and Markus]
Does anybody know anything about this issue? Is it on purpose?
Am I getting it wrong?


>> +    }
>> +}
>> +
>> +static void init_ports(PVRDMADev *dev, Error **errp)
>> +{
>> +    int i;
>> +
>> +    memset(dev->rdma_dev_res.ports, 0, sizeof(dev->rdma_dev_res.ports));
>> +
>> +    for (i = 0; i < MAX_PORTS; i++) {
>> +        dev->rdma_dev_res.ports[i].state = PVRDMA_PORT_DOWN;
>> +
>> +        dev->rdma_dev_res.ports[i].pkey_tbl =
>> +            g_malloc0(sizeof(*dev->rdma_dev_res.ports[i].pkey_tbl) *
>> +                      MAX_PORT_PKEYS);
> 
> ...init_ports() is allocated memory into ports[i].pkey_tbl,
> so maybe this is what free_ports() is intended to be freeing ?
> 

Right, we will fix it.

Thanks again for running Coverity!
Marcel

>> +    }
>> +}
> 
> thanks
> -- PMM
>