[Qemu-devel] BUG? Using memory after freeing

[Qemu-devel] BUG? Using memory after freeing

[Aleksey Kuleshov]

Hello!

Explanation of what I saw is follows.

In hw/pci/pci_bridge.c function pci_bridge_update_mappings does follows:
```
void pci_bridge_update_mappings(PCIBridge *br)
{
    PCIBridgeWindows *w = br->windows;

    /* Make updates atomic to: handle the case of one VCPU updating the bridge
     * while another accesses an unaffected region. */
    memory_region_transaction_begin();
    pci_bridge_region_del(br, br->windows);
    br->windows = pci_bridge_region_init(br);
    memory_region_transaction_commit();
    pci_bridge_region_cleanup(br, w);
}
```

It calls memory_region_transaction_commit which calls flatview_unref:
```
static void flatview_unref(FlatView *view)
{
    if (atomic_fetch_dec(&view->ref) == 1) {
        trace_flatview_destroy_rcu(view, view->root);
        assert(view->root);
        call_rcu(view, flatview_destroy, rcu);
    }
}
```
As far as I understood, call_rcu can be considered as a deferred call to flatview_destroy.

Then in pci_bridge_update_mappings there is a call to pci_bridge_region_cleanup which does:
```
static void pci_bridge_region_cleanup(PCIBridge *br, PCIBridgeWindows *w)
{
    object_unparent(OBJECT(&w->alias_io));
    object_unparent(OBJECT(&w->alias_mem));
    object_unparent(OBJECT(&w->alias_pref_mem));
    object_unparent(OBJECT(&w->alias_vga[QEMU_PCI_VGA_IO_LO]));
    object_unparent(OBJECT(&w->alias_vga[QEMU_PCI_VGA_IO_HI]));
    object_unparent(OBJECT(&w->alias_vga[QEMU_PCI_VGA_MEM]));
    g_free(w);
}
```
Note g_free(w). "w" holds MemoryRegions, which are part of that FlatView
which is going to be destroyed some time later in the future.

When RCU thread kicks in, flatview_destroy is called on MemoryRegions which were
part of that "w" which is now freed and QEMU seg faults.

Re: [Qemu-devel] BUG? Using memory after freeing

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2181 bytes --]

On Wed, Mar 07, 2018 at 01:49:01PM +0300, Aleksey Kuleshov wrote:
> Hello!

Thanks for the email!  I have CCed the PCI maintainers, Michael Tsirkin
and Marcel Apfelbaum.

In the future you can find out who to contact using:

  $ scripts/get_maintainers -f hw/pci/pci_bridge.c

> Explanation of what I saw is follows.
> 
> In hw/pci/pci_bridge.c function pci_bridge_update_mappings does follows:
> ```
> void pci_bridge_update_mappings(PCIBridge *br)
> {
>     PCIBridgeWindows *w = br->windows;
> 
>     /* Make updates atomic to: handle the case of one VCPU updating the bridge
>      * while another accesses an unaffected region. */
>     memory_region_transaction_begin();
>     pci_bridge_region_del(br, br->windows);
>     br->windows = pci_bridge_region_init(br);
>     memory_region_transaction_commit();
>     pci_bridge_region_cleanup(br, w);
> }
> ```
> 
> It calls memory_region_transaction_commit which calls flatview_unref:
> ```
> static void flatview_unref(FlatView *view)
> {
>     if (atomic_fetch_dec(&view->ref) == 1) {
>         trace_flatview_destroy_rcu(view, view->root);
>         assert(view->root);
>         call_rcu(view, flatview_destroy, rcu);
>     }
> }
> ```
> As far as I understood, call_rcu can be considered as a deferred call to flatview_destroy.
> 
> Then in pci_bridge_update_mappings there is a call to pci_bridge_region_cleanup which does:
> ```
> static void pci_bridge_region_cleanup(PCIBridge *br, PCIBridgeWindows *w)
> {
>     object_unparent(OBJECT(&w->alias_io));
>     object_unparent(OBJECT(&w->alias_mem));
>     object_unparent(OBJECT(&w->alias_pref_mem));
>     object_unparent(OBJECT(&w->alias_vga[QEMU_PCI_VGA_IO_LO]));
>     object_unparent(OBJECT(&w->alias_vga[QEMU_PCI_VGA_IO_HI]));
>     object_unparent(OBJECT(&w->alias_vga[QEMU_PCI_VGA_MEM]));
>     g_free(w);
> }
> ```
> Note g_free(w). "w" holds MemoryRegions, which are part of that FlatView
> which is going to be destroyed some time later in the future.
> 
> When RCU thread kicks in, flatview_destroy is called on MemoryRegions which were
> part of that "w" which is now freed and QEMU seg faults.
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]