| Message ID | 1470857149-32003-1-git-send-email-cota@braap.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 10 Aug 2016 15:25:49 -0400 "Emilio G. Cota" <cota@braap.org> wrote: Emilio, Pls also see https://www.mail-archive.com/qemu-devel@nongnu.org/msg391669.html which fixed issue for me, I've CCed you. > tb_flush() is called when debugging the guest (under both KVM > and TCG accelerators) with gdb. tb_flush() resets TCG's qht, which > segfaults if we're using KVM due to the qht not being initialized. > > Fix this adding a magic number field to struct qht to track whether a qht > has been initialized with qht_init(). Then, explicitly allow > passing uninitialized qht's to qht_reset() and qht_reset_size(), > just like we do with qht_statistics_init(). > > Reported-by: Brent Baccala <cosine@freesoft.org> > Reported-by: Igor Mammedov <imammedo@redhat.com> > Signed-off-by: Emilio G. Cota <cota@braap.org> > --- > include/qemu/qht.h | 7 +++++++ > tests/test-qht.c | 3 +++ > util/qht.c | 20 +++++++++++++++++--- > 3 files changed, 27 insertions(+), 3 deletions(-) > > diff --git a/include/qemu/qht.h b/include/qemu/qht.h > index 311139b..39dd5e8 100644 > --- a/include/qemu/qht.h > +++ b/include/qemu/qht.h > @@ -15,6 +15,7 @@ struct qht { > struct qht_map *map; > QemuMutex lock; /* serializes setters of ht->map */ > unsigned int mode; > + unsigned int magic; > }; > > /** > @@ -124,6 +125,8 @@ bool qht_remove(struct qht *ht, const void *p, uint32_t hash); > * If concurrent readers may exist, the objects pointed to by the hash table > * must remain valid for the existing RCU grace period -- see qht_remove(). > * See also: qht_reset_size() > + * > + * Note: it is OK to pass an uninitialized @ht. > */ > void qht_reset(struct qht *ht); > > @@ -138,6 +141,8 @@ void qht_reset(struct qht *ht); > * If concurrent readers may exist, the objects pointed to by the hash table > * must remain valid for the existing RCU grace period -- see qht_remove(). > * See also: qht_reset(), qht_resize(). > + * > + * Note: it is OK to pass an uninitialized @ht. > */ > bool qht_reset_size(struct qht *ht, size_t n_elems); > > @@ -173,6 +178,8 @@ void qht_iter(struct qht *ht, qht_iter_func_t func, void *userp); > * > * When done with @stats, pass the struct to qht_statistics_destroy(). > * Failing to do this will leak memory. > + * > + * Note: it is OK to pass an uninitialized @ht. > */ > void qht_statistics_init(struct qht *ht, struct qht_stats *stats); > > diff --git a/tests/test-qht.c b/tests/test-qht.c > index 46a64b6..a923b2e 100644 > --- a/tests/test-qht.c > +++ b/tests/test-qht.c > @@ -97,6 +97,9 @@ static void qht_do_test(unsigned int mode, size_t init_entries) > { > /* under KVM we might fetch stats from an uninitialized qht */ > check_n(0); > + /* resetting an uninitialized qht can happen as well, e.g. KVM + gdb */ > + qht_reset(&ht); > + qht_reset_size(&ht, 0); > > qht_init(&ht, 0, mode); > > diff --git a/util/qht.c b/util/qht.c > index 16a8d79..e4c90d6 100644 > --- a/util/qht.c > +++ b/util/qht.c > @@ -89,6 +89,8 @@ > #define QHT_BUCKET_ENTRIES 4 > #endif > > +#define QHT_MAGIC 0xbebec4fe > + > /* > * Note: reading partially-updated pointers in @pointers could lead to > * segfaults. We thus access them with atomic_read/set; this guarantees > @@ -182,6 +184,11 @@ static inline void qht_map_debug__all_locked(struct qht_map *map) > { } > #endif /* QHT_DEBUG */ > > +static inline bool qht_inited(const struct qht *ht) > +{ > + return ht->magic == QHT_MAGIC; > +} > + > static inline size_t qht_elems_to_buckets(size_t n_elems) > { > return pow2ceil(n_elems / QHT_BUCKET_ENTRIES); > @@ -356,6 +363,7 @@ void qht_init(struct qht *ht, size_t n_elems, unsigned int mode) > size_t n_buckets = qht_elems_to_buckets(n_elems); > > ht->mode = mode; > + ht->magic = QHT_MAGIC; > qemu_mutex_init(&ht->lock); > map = qht_map_create(n_buckets); > atomic_rcu_set(&ht->map, map); > @@ -403,6 +411,10 @@ void qht_reset(struct qht *ht) > { > struct qht_map *map; > > + if (unlikely(!qht_inited(ht))) { > + return; > + } > + > qht_map_lock_buckets__no_stale(ht, &map); > qht_map_reset__all_locked(map); > qht_map_unlock_buckets(map); > @@ -415,6 +427,9 @@ bool qht_reset_size(struct qht *ht, size_t n_elems) > size_t n_buckets; > bool resize = false; > > + if (unlikely(!qht_inited(ht))) { > + return false; > + } > n_buckets = qht_elems_to_buckets(n_elems); > > qemu_mutex_lock(&ht->lock); > @@ -787,17 +802,16 @@ void qht_statistics_init(struct qht *ht, struct qht_stats *stats) > struct qht_map *map; > int i; > > - map = atomic_rcu_read(&ht->map); > - > stats->used_head_buckets = 0; > stats->entries = 0; > qdist_init(&stats->chain); > qdist_init(&stats->occupancy); > /* bail out if the qht has not yet been initialized */ > - if (unlikely(map == NULL)) { > + if (unlikely(!qht_inited(ht))) { > stats->head_buckets = 0; > return; > } > + map = atomic_rcu_read(&ht->map); > stats->head_buckets = map->n_buckets; > > for (i = 0; i < map->n_buckets; i++) {
diff --git a/include/qemu/qht.h b/include/qemu/qht.h index 311139b..39dd5e8 100644 --- a/include/qemu/qht.h +++ b/include/qemu/qht.h @@ -15,6 +15,7 @@ struct qht { struct qht_map *map; QemuMutex lock; /* serializes setters of ht->map */ unsigned int mode; + unsigned int magic; }; /** @@ -124,6 +125,8 @@ bool qht_remove(struct qht *ht, const void *p, uint32_t hash); * If concurrent readers may exist, the objects pointed to by the hash table * must remain valid for the existing RCU grace period -- see qht_remove(). * See also: qht_reset_size() + * + * Note: it is OK to pass an uninitialized @ht. */ void qht_reset(struct qht *ht); @@ -138,6 +141,8 @@ void qht_reset(struct qht *ht); * If concurrent readers may exist, the objects pointed to by the hash table * must remain valid for the existing RCU grace period -- see qht_remove(). * See also: qht_reset(), qht_resize(). + * + * Note: it is OK to pass an uninitialized @ht. */ bool qht_reset_size(struct qht *ht, size_t n_elems); @@ -173,6 +178,8 @@ void qht_iter(struct qht *ht, qht_iter_func_t func, void *userp); * * When done with @stats, pass the struct to qht_statistics_destroy(). * Failing to do this will leak memory. + * + * Note: it is OK to pass an uninitialized @ht. */ void qht_statistics_init(struct qht *ht, struct qht_stats *stats); diff --git a/tests/test-qht.c b/tests/test-qht.c index 46a64b6..a923b2e 100644 --- a/tests/test-qht.c +++ b/tests/test-qht.c @@ -97,6 +97,9 @@ static void qht_do_test(unsigned int mode, size_t init_entries) { /* under KVM we might fetch stats from an uninitialized qht */ check_n(0); + /* resetting an uninitialized qht can happen as well, e.g. KVM + gdb */ + qht_reset(&ht); + qht_reset_size(&ht, 0); qht_init(&ht, 0, mode); diff --git a/util/qht.c b/util/qht.c index 16a8d79..e4c90d6 100644 --- a/util/qht.c +++ b/util/qht.c @@ -89,6 +89,8 @@ #define QHT_BUCKET_ENTRIES 4 #endif +#define QHT_MAGIC 0xbebec4fe + /* * Note: reading partially-updated pointers in @pointers could lead to * segfaults. We thus access them with atomic_read/set; this guarantees @@ -182,6 +184,11 @@ static inline void qht_map_debug__all_locked(struct qht_map *map) { } #endif /* QHT_DEBUG */ +static inline bool qht_inited(const struct qht *ht) +{ + return ht->magic == QHT_MAGIC; +} + static inline size_t qht_elems_to_buckets(size_t n_elems) { return pow2ceil(n_elems / QHT_BUCKET_ENTRIES); @@ -356,6 +363,7 @@ void qht_init(struct qht *ht, size_t n_elems, unsigned int mode) size_t n_buckets = qht_elems_to_buckets(n_elems); ht->mode = mode; + ht->magic = QHT_MAGIC; qemu_mutex_init(&ht->lock); map = qht_map_create(n_buckets); atomic_rcu_set(&ht->map, map); @@ -403,6 +411,10 @@ void qht_reset(struct qht *ht) { struct qht_map *map; + if (unlikely(!qht_inited(ht))) { + return; + } + qht_map_lock_buckets__no_stale(ht, &map); qht_map_reset__all_locked(map); qht_map_unlock_buckets(map); @@ -415,6 +427,9 @@ bool qht_reset_size(struct qht *ht, size_t n_elems) size_t n_buckets; bool resize = false; + if (unlikely(!qht_inited(ht))) { + return false; + } n_buckets = qht_elems_to_buckets(n_elems); qemu_mutex_lock(&ht->lock); @@ -787,17 +802,16 @@ void qht_statistics_init(struct qht *ht, struct qht_stats *stats) struct qht_map *map; int i; - map = atomic_rcu_read(&ht->map); - stats->used_head_buckets = 0; stats->entries = 0; qdist_init(&stats->chain); qdist_init(&stats->occupancy); /* bail out if the qht has not yet been initialized */ - if (unlikely(map == NULL)) { + if (unlikely(!qht_inited(ht))) { stats->head_buckets = 0; return; } + map = atomic_rcu_read(&ht->map); stats->head_buckets = map->n_buckets; for (i = 0; i < map->n_buckets; i++) {
tb_flush() is called when debugging the guest (under both KVM and TCG accelerators) with gdb. tb_flush() resets TCG's qht, which segfaults if we're using KVM due to the qht not being initialized. Fix this adding a magic number field to struct qht to track whether a qht has been initialized with qht_init(). Then, explicitly allow passing uninitialized qht's to qht_reset() and qht_reset_size(), just like we do with qht_statistics_init(). Reported-by: Brent Baccala <cosine@freesoft.org> Reported-by: Igor Mammedov <imammedo@redhat.com> Signed-off-by: Emilio G. Cota <cota@braap.org> --- include/qemu/qht.h | 7 +++++++ tests/test-qht.c | 3 +++ util/qht.c | 20 +++++++++++++++++--- 3 files changed, 27 insertions(+), 3 deletions(-)