From c6742b14fe7352059cd4954a356a8105757af31b Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 14 Jul 2015 13:45:34 +0200 Subject: [PATCH] memory: fix refcount leak in memory_region_present memory_region_present() leaks a reference to a MemoryRegion in the case "mr == container". While fixing it, avoid reference counting altogether for memory_region_present(), by using RCU only. The return value could in principle be already invalid immediately after memory_region_present returns, but presumably the caller knows that and it's using memory_region_present to probe for devices that are unpluggable, or something like that. The RCU critical section is needed anyway, because it protects as->current_map. Reported-by: Peter Maydell Signed-off-by: Paolo Bonzini --- memory.c | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/memory.c b/memory.c index 5a0cc66982..0acebb1f22 100644 --- a/memory.c +++ b/memory.c @@ -1887,23 +1887,16 @@ static FlatRange *flatview_lookup(FlatView *view, AddrRange addr) sizeof(FlatRange), cmp_flatrange_addr); } -bool memory_region_present(MemoryRegion *container, hwaddr addr) -{ - MemoryRegion *mr = memory_region_find(container, addr, 1).mr; - if (!mr || (mr == container)) { - return false; - } - memory_region_unref(mr); - return true; -} - bool memory_region_is_mapped(MemoryRegion *mr) { return mr->container ? true : false; } -MemoryRegionSection memory_region_find(MemoryRegion *mr, - hwaddr addr, uint64_t size) +/* Same as memory_region_find, but it does not add a reference to the + * returned region. It must be called from an RCU critical section. + */ +static MemoryRegionSection memory_region_find_rcu(MemoryRegion *mr, + hwaddr addr, uint64_t size) { MemoryRegionSection ret = { .mr = NULL }; MemoryRegion *root; @@ -1924,11 +1917,10 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr, } range = addrrange_make(int128_make64(addr), int128_make64(size)); - rcu_read_lock(); view = atomic_rcu_read(&as->current_map); fr = flatview_lookup(view, range); if (!fr) { - goto out; + return ret; } while (fr > view->ranges && addrrange_intersects(fr[-1].addr, range)) { @@ -1944,12 +1936,32 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr, ret.size = range.size; ret.offset_within_address_space = int128_get64(range.start); ret.readonly = fr->readonly; - memory_region_ref(ret.mr); -out: + return ret; +} + +MemoryRegionSection memory_region_find(MemoryRegion *mr, + hwaddr addr, uint64_t size) +{ + MemoryRegionSection ret; + rcu_read_lock(); + ret = memory_region_find_rcu(mr, addr, size); + if (ret.mr) { + memory_region_ref(ret.mr); + } rcu_read_unlock(); return ret; } +bool memory_region_present(MemoryRegion *container, hwaddr addr) +{ + MemoryRegion *mr; + + rcu_read_lock(); + mr = memory_region_find_rcu(container, addr, 1).mr; + rcu_read_unlock(); + return mr && mr != container; +} + void address_space_sync_dirty_bitmap(AddressSpace *as) { FlatView *view;