Message ID | 20170921085110.25598-5-aik@ozlabs.ru |
---|---|
State | New |
Headers | show |
Series | memory: Store physical root MR in FlatView | expand |
On 21/09/2017 10:50, Alexey Kardashevskiy wrote: > * since FlatView::rcu is used now to dispose FV, call_rcu() in > address_space_update_topology() is replaced with direct call to > flatview_unref() Hmm, this is not correct, as you could have thread 1 thread 2 RCU thread ------------------------------------------------------------- rcu_read_lock read as->current_map set as->current_map flatview_unref '--> call_rcu flatview_ref rcu_read_unlock flatview_destroy I need to think a bit more about this (and possibly ask Paul...). Paolo
On 21/09/17 21:51, Paolo Bonzini wrote: > On 21/09/2017 10:50, Alexey Kardashevskiy wrote: >> * since FlatView::rcu is used now to dispose FV, call_rcu() in >> address_space_update_topology() is replaced with direct call to >> flatview_unref() > > Hmm, this is not correct, as you could have > > > thread 1 thread 2 RCU thread > ------------------------------------------------------------- > rcu_read_lock > read as->current_map > set as->current_map > flatview_unref > '--> call_rcu > flatview_ref > rcu_read_unlock > flatview_destroy > > I need to think a bit more about this (and possibly ask Paul...). > > Paolo > Nah, you're right, it should be like this: diff --git a/memory.c b/memory.c index 35b2fc5f7f..689bf53866 100644 --- a/memory.c +++ b/memory.c @@ -317,7 +317,7 @@ static void flatview_ref(FlatView *view) static void flatview_unref(FlatView *view) { if (atomic_fetch_dec(&view->ref) == 1) { - call_rcu(view, flatview_destroy, rcu); + flatview_destroy(view); } } @@ -768,7 +768,7 @@ static FlatView *generate_memory_topology(MemoryRegion *mr) flatview_simplify(view); if (!view->nr) { - flatview_destroy(view); + flatview_unref(view); use_empty = true; } } @@ -1026,7 +1026,7 @@ static void address_space_set_flatview(AddressSpace *as) /* Writes are protected by the BQL. */ atomic_rcu_set(&as->current_map, new_view); if (old_view) { - flatview_unref(old_view); + call_rcu(view, flatview_unref, rcu); }
On 21/09/2017 15:44, Alexey Kardashevskiy wrote: > On 21/09/17 21:51, Paolo Bonzini wrote: >> On 21/09/2017 10:50, Alexey Kardashevskiy wrote: >>> * since FlatView::rcu is used now to dispose FV, call_rcu() in >>> address_space_update_topology() is replaced with direct call to >>> flatview_unref() >> >> Hmm, this is not correct, as you could have >> >> >> thread 1 thread 2 RCU thread >> ------------------------------------------------------------- >> rcu_read_lock >> read as->current_map >> set as->current_map >> flatview_unref >> '--> call_rcu >> flatview_ref >> rcu_read_unlock >> flatview_destroy >> >> I need to think a bit more about this (and possibly ask Paul...). >> >> Paolo >> > > Nah, you're right, it should be like this: > > > diff --git a/memory.c b/memory.c > index 35b2fc5f7f..689bf53866 100644 > --- a/memory.c > +++ b/memory.c > @@ -317,7 +317,7 @@ static void flatview_ref(FlatView *view) > static void flatview_unref(FlatView *view) > { > if (atomic_fetch_dec(&view->ref) == 1) { > - call_rcu(view, flatview_destroy, rcu); > + flatview_destroy(view); > } > } > > @@ -768,7 +768,7 @@ static FlatView *generate_memory_topology(MemoryRegion *mr) > flatview_simplify(view); > > if (!view->nr) { > - flatview_destroy(view); > + flatview_unref(view); > use_empty = true; > } > } > @@ -1026,7 +1026,7 @@ static void address_space_set_flatview(AddressSpace *as) > /* Writes are protected by the BQL. */ > atomic_rcu_set(&as->current_map, new_view); > if (old_view) { > - flatview_unref(old_view); > + call_rcu(view, flatview_unref, rcu); > } This still doesn't cover address_space_get_flatview, i.e. it is a pre-existing bug. I found a similar case in Linux, here is how they solved it: commit 358136532dd29e9ed96e0e523d2d510e71bda003 Author: Paolo Bonzini <pbonzini@redhat.com> Date: Thu Sep 21 14:32:47 2017 +0200 memory: avoid "resurrection" of dead FlatViews It's possible for address_space_get_flatview() as it currently stands to cause a use-after-free for the returned FlatView, if the reference count is incremented after the FlatView has been replaced by a writer: thread 1 thread 2 RCU thread ------------------------------------------------------------- rcu_read_lock read as->current_map set as->current_map flatview_unref '--> call_rcu flatview_ref [ref=1] rcu_read_unlock flatview_destroy <badness> Since FlatViews are not updated very often, we can just detect the situation using a new atomic op atomic_fetch_inc_nonzero, similar to Linux's atomic_inc_not_zero, which performs the refcount increment only if it hasn't already hit zero. This is similar to Linux commit de09a9771a53 ("CRED: Fix get_task_cred() and task_state() to not resurrect dead credentials", 2010-07-29). Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> diff --git a/docs/devel/atomics.txt b/docs/devel/atomics.txt index 048e5f23cb..10c5fa37e8 100644 --- a/docs/devel/atomics.txt +++ b/docs/devel/atomics.txt @@ -64,6 +64,7 @@ operations: typeof(*ptr) atomic_fetch_and(ptr, val) typeof(*ptr) atomic_fetch_or(ptr, val) typeof(*ptr) atomic_fetch_xor(ptr, val) + typeof(*ptr) atomic_fetch_inc_nonzero(ptr) typeof(*ptr) atomic_xchg(ptr, val) typeof(*ptr) atomic_cmpxchg(ptr, old, new) diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h index b6b62fb771..44ad1e6c32 100644 --- a/include/qemu/atomic.h +++ b/include/qemu/atomic.h @@ -197,6 +197,15 @@ atomic_cmpxchg__nocheck(ptr, old, new); \ }) +#define atomic_fetch_inc_nonzero(ptr) ({ \ + QEMU_BUILD_BUG_ON(sizeof(*ptr) > ATOMIC_REG_SIZE); \ + typeof_strip_qual(*ptr) _oldn = atomic_read(ptr); \ + while (_oldn && atomic_cmpxchg(ptr, _oldn, _oldn + 1) != _oldn) { \ + _oldn = atomic_read(ptr); \ + } \ + _oldn; \ +}) + /* Provide shorter names for GCC atomic builtins, return old value */ #define atomic_fetch_inc(ptr) __atomic_fetch_add(ptr, 1, __ATOMIC_SEQ_CST) #define atomic_fetch_dec(ptr) __atomic_fetch_sub(ptr, 1, __ATOMIC_SEQ_CST) diff --git a/memory.c b/memory.c index 2b90117c60..51f54ab430 100644 --- a/memory.c +++ b/memory.c @@ -294,9 +294,9 @@ static void flatview_destroy(FlatView *view) g_free(view); } -static void flatview_ref(FlatView *view) +static bool flatview_ref(FlatView *view) { - atomic_inc(&view->ref); + return atomic_fetch_inc_nonzero(&view->ref) > 0; } static void flatview_unref(FlatView *view) @@ -773,8 +773,12 @@ static FlatView *address_space_get_flatview(AddressSpace *as) FlatView *view; rcu_read_lock(); - view = atomic_rcu_read(&as->current_map); - flatview_ref(view); + do { + view = atomic_rcu_read(&as->current_map); + /* If somebody has replaced as->current_map concurrently, + * flatview_ref returns false. + */ + } while (!flatview_ref(view)); rcu_read_unlock(); return view; }
diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h index 9abde2f11c..6e08eda256 100644 --- a/include/exec/memory-internal.h +++ b/include/exec/memory-internal.h @@ -22,16 +22,18 @@ #ifndef CONFIG_USER_ONLY typedef struct AddressSpaceDispatch AddressSpaceDispatch; -void address_space_destroy_dispatch(AddressSpace *as); - extern const MemoryRegionOps unassigned_mem_ops; bool memory_region_access_valid(MemoryRegion *mr, hwaddr addr, unsigned size, bool is_write); -void mem_add(AddressSpace *as, MemoryRegionSection *section); -void mem_begin(AddressSpace *as); -void mem_commit(AddressSpace *as); +void mem_add(AddressSpace *as, FlatView *fv, MemoryRegionSection *section); +AddressSpaceDispatch *mem_begin(AddressSpace *as); +void mem_commit(AddressSpaceDispatch *d); + +AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as); +AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv); +void address_space_dispatch_free(AddressSpaceDispatch *d); #endif #endif diff --git a/include/exec/memory.h b/include/exec/memory.h index 9581f7a7db..2346f8b863 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -316,8 +316,6 @@ struct AddressSpace { int ioeventfd_nb; struct MemoryRegionIoeventfd *ioeventfds; - struct AddressSpaceDispatch *dispatch; - struct AddressSpaceDispatch *next_dispatch; QTAILQ_HEAD(memory_listeners_as, MemoryListener) listeners; QTAILQ_ENTRY(AddressSpace) address_spaces_link; }; diff --git a/exec.c b/exec.c index 1626d254bb..afd64127e6 100644 --- a/exec.c +++ b/exec.c @@ -187,8 +187,6 @@ typedef struct PhysPageMap { } PhysPageMap; struct AddressSpaceDispatch { - struct rcu_head rcu; - MemoryRegionSection *mru_section; /* This is a multi-level map on the physical address space. * The bottom level has pointers to MemoryRegionSections. @@ -485,7 +483,7 @@ static MemoryRegionSection address_space_do_translate(AddressSpace *as, IOMMUMemoryRegionClass *imrc; for (;;) { - AddressSpaceDispatch *d = atomic_rcu_read(&as->dispatch); + AddressSpaceDispatch *d = address_space_to_dispatch(as); section = address_space_translate_internal(d, addr, &addr, plen, is_mmio); iommu_mr = memory_region_get_iommu(section->mr); @@ -1222,7 +1220,7 @@ hwaddr memory_region_section_get_iotlb(CPUState *cpu, } else { AddressSpaceDispatch *d; - d = atomic_rcu_read(§ion->address_space->dispatch); + d = address_space_to_dispatch(section->address_space); iotlb = section - d->map.sections; iotlb += xlat; } @@ -1347,9 +1345,9 @@ static void register_multipage(AddressSpaceDispatch *d, phys_page_set(d, start_addr >> TARGET_PAGE_BITS, num_pages, section_index); } -void mem_add(AddressSpace *as, MemoryRegionSection *section) +void mem_add(AddressSpace *as, FlatView *fv, MemoryRegionSection *section) { - AddressSpaceDispatch *d = as->next_dispatch; + AddressSpaceDispatch *d = flatview_to_dispatch(fv); MemoryRegionSection now = *section, remain = *section; Int128 page_size = int128_make64(TARGET_PAGE_SIZE); @@ -2672,7 +2670,7 @@ static void io_mem_init(void) NULL, UINT64_MAX); } -void mem_begin(AddressSpace *as) +AddressSpaceDispatch *mem_begin(AddressSpace *as) { AddressSpaceDispatch *d = g_new0(AddressSpaceDispatch, 1); uint16_t n; @@ -2688,26 +2686,19 @@ void mem_begin(AddressSpace *as) d->phys_map = (PhysPageEntry) { .ptr = PHYS_MAP_NODE_NIL, .skip = 1 }; d->as = as; - as->next_dispatch = d; + + return d; } -static void address_space_dispatch_free(AddressSpaceDispatch *d) +void address_space_dispatch_free(AddressSpaceDispatch *d) { phys_sections_free(&d->map); g_free(d); } -void mem_commit(AddressSpace *as) +void mem_commit(AddressSpaceDispatch *d) { - AddressSpaceDispatch *cur = as->dispatch; - AddressSpaceDispatch *next = as->next_dispatch; - - phys_page_compact_all(next, next->map.nodes_nb); - - atomic_rcu_set(&as->dispatch, next); - if (cur) { - call_rcu(cur, address_space_dispatch_free, rcu); - } + phys_page_compact_all(d, d->map.nodes_nb); } static void tcg_commit(MemoryListener *listener) @@ -2723,21 +2714,11 @@ static void tcg_commit(MemoryListener *listener) * We reload the dispatch pointer now because cpu_reloading_memory_map() * may have split the RCU critical section. */ - d = atomic_rcu_read(&cpuas->as->dispatch); + d = address_space_to_dispatch(cpuas->as); atomic_rcu_set(&cpuas->memory_dispatch, d); tlb_flush(cpuas->cpu); } -void address_space_destroy_dispatch(AddressSpace *as) -{ - AddressSpaceDispatch *d = as->dispatch; - - atomic_rcu_set(&as->dispatch, NULL); - if (d) { - call_rcu(d, address_space_dispatch_free, rcu); - } -} - static void memory_map_init(void) { system_memory = g_malloc(sizeof(*system_memory)); diff --git a/memory.c b/memory.c index 9babd7d423..27d7aeffc2 100644 --- a/memory.c +++ b/memory.c @@ -229,6 +229,7 @@ struct FlatView { FlatRange *ranges; unsigned nr; unsigned nr_allocated; + struct AddressSpaceDispatch *dispatch; }; typedef struct AddressSpaceOps AddressSpaceOps; @@ -289,6 +290,9 @@ static void flatview_destroy(FlatView *view) { int i; + if (view->dispatch) { + address_space_dispatch_free(view->dispatch); + } for (i = 0; i < view->nr; i++) { memory_region_unref(view->ranges[i].mr); } @@ -304,10 +308,25 @@ static void flatview_ref(FlatView *view) static void flatview_unref(FlatView *view) { if (atomic_fetch_dec(&view->ref) == 1) { - flatview_destroy(view); + call_rcu(view, flatview_destroy, rcu); } } +static FlatView *address_space_to_flatview(AddressSpace *as) +{ + return atomic_rcu_read(&as->current_map); +} + +AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv) +{ + return fv->dispatch; +} + +AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as) +{ + return flatview_to_dispatch(address_space_to_flatview(as)); +} + static bool can_merge(FlatRange *r1, FlatRange *r2) { return int128_eq(addrrange_end(r1->addr), r2->addr.start) @@ -886,13 +905,13 @@ static void address_space_update_topology(AddressSpace *as) FlatView *new_view = generate_memory_topology(as->root); int i; - mem_begin(as); + new_view->dispatch = mem_begin(as); for (i = 0; i < new_view->nr; i++) { MemoryRegionSection mrs = section_from_flat_range(&new_view->ranges[i], as); - mem_add(as, &mrs); + mem_add(as, new_view, &mrs); } - mem_commit(as); + mem_commit(new_view->dispatch); if (!QTAILQ_EMPTY(&as->listeners)) { address_space_update_topology_pass(as, old_view, new_view, false); @@ -901,7 +920,7 @@ static void address_space_update_topology(AddressSpace *as) /* Writes are protected by the BQL. */ atomic_rcu_set(&as->current_map, new_view); - call_rcu(old_view, flatview_unref, rcu); + flatview_unref(old_view); /* Note that all the old MemoryRegions are still alive up to this * point. This relieves most MemoryListeners from the need to @@ -2631,7 +2650,6 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name) QTAILQ_INIT(&as->listeners); QTAILQ_INSERT_TAIL(&address_spaces, as, address_spaces_link); as->name = g_strdup(name ? name : "anonymous"); - as->dispatch = NULL; memory_region_update_pending |= root->enabled; memory_region_transaction_commit(); } @@ -2640,7 +2658,6 @@ static void do_address_space_destroy(AddressSpace *as) { bool do_free = as->malloced; - address_space_destroy_dispatch(as); assert(QTAILQ_EMPTY(&as->listeners)); flatview_unref(as->current_map);
As we are going to share FlatView's between AddressSpace's, and AddressSpaceDispatch is a structure to perform quick lookup in FlatView, this moves ASD to FlatView. After previosly open coded ASD rendering, we can also remove as->next_dispatch as the new FlatView pointer is stored on a stack and set to an AS atomically. flatview_destroy() is executed under RCU instead of address_space_dispatch_free() now. This makes mem_begin/mem_commit to work with ASD and mem_add with FV as later on mem_add will be taking FV as an argument anyway. This should cause no behavioural change. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- Changes: v4: * since FlatView::rcu is used now to dispose FV, call_rcu() in address_space_update_topology() is replaced with direct call to flatview_unref() --- include/exec/memory-internal.h | 12 +++++++----- include/exec/memory.h | 2 -- exec.c | 41 +++++++++++------------------------------ memory.c | 31 ++++++++++++++++++++++++------- 4 files changed, 42 insertions(+), 44 deletions(-)