Message ID | 1469205390-14369-1-git-send-email-cota@braap.org |
---|---|
State | New |
Headers | show |
On 22/07/2016 18:36, Emilio G. Cota wrote: > So far, QHT functions assume that the passed qht has previously been > initialized--otherwise they segfault. > > This patch makes an exception for qht_statistics_init, with the goal > of simplifying calling code. For instance, qht_statistics_init is > called from the 'info jit' dump, and given that under KVM the TB qht > is never initialized, we get a segfault. Thus, instead of complicating > the 'info jit' code with additional checks, let's allow passing an > uninitialized qht to qht_statistics_init. > > While at it, add a test for this to test-qht. > > Before the patch (for $ qemu -enable-kvm [...]): > (qemu) info jit > [...] > direct jump count 0 (0%) (2 jumps=0 0%) > Program received signal SIGSEGV, Segmentation fault. > > After the patch: > (qemu) info jit > [...] > direct jump count 0 (0%) (2 jumps=0 0%) > TB hash buckets 0/0 (-nan% head buckets used) > TB hash occupancy nan% avg chain occ. Histogram: (null) > TB hash avg chain nan buckets. Histogram: (null) > [...] > > Reported by: Changlong Xie <xiecl.fnst@cn.fujitsu.com> > Signed-off-by: Emilio G. Cota <cota@braap.org> > --- > tests/test-qht.c | 4 ++++ > util/qht.c | 7 ++++++- > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/tests/test-qht.c b/tests/test-qht.c > index c8eb930..e2f9e14 100644 > --- a/tests/test-qht.c > +++ b/tests/test-qht.c > @@ -96,8 +96,12 @@ static void iter_check(unsigned int count) > > 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); > + > qht_init(&ht, 0, mode); > > + check_n(0); > insert(0, N); > check(0, N, true); > check_n(N); > diff --git a/util/qht.c b/util/qht.c > index 6f74909..0cb95e2 100644 > --- a/util/qht.c > +++ b/util/qht.c > @@ -783,11 +783,16 @@ void qht_statistics_init(struct qht *ht, struct qht_stats *stats) > > map = atomic_rcu_read(&ht->map); > > - stats->head_buckets = map->n_buckets; > 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)) { > + stats->head_buckets = 0; > + return; > + } > + stats->head_buckets = map->n_buckets; > > for (i = 0; i < map->n_buckets; i++) { > struct qht_bucket *head = &map->buckets[i]; > Queued, thanks. Paolo
On 22 July 2016 at 17:36, Emilio G. Cota <cota@braap.org> wrote: > So far, QHT functions assume that the passed qht has previously been > initialized--otherwise they segfault. > > This patch makes an exception for qht_statistics_init, with the goal > of simplifying calling code. For instance, qht_statistics_init is > called from the 'info jit' dump, and given that under KVM the TB qht > is never initialized, we get a segfault. Thus, instead of complicating > the 'info jit' code with additional checks, let's allow passing an > uninitialized qht to qht_statistics_init. > > While at it, add a test for this to test-qht. > > Before the patch (for $ qemu -enable-kvm [...]): > (qemu) info jit > [...] > direct jump count 0 (0%) (2 jumps=0 0%) > Program received signal SIGSEGV, Segmentation fault. > > After the patch: > (qemu) info jit > [...] > direct jump count 0 (0%) (2 jumps=0 0%) > TB hash buckets 0/0 (-nan% head buckets used) > TB hash occupancy nan% avg chain occ. Histogram: (null) > TB hash avg chain nan buckets. Histogram: (null) This looks like we're passing NULL pointers to printf %s specifiers. This is undefined behaviour at least for POSIX printf, and I can't see anything in the glib printf-alike function documentation that gives an extra guarantee for this, so it's probably a bad idea. Printing 'nan' also looks a bit odd, though it's not UB. thanks -- PMM
diff --git a/tests/test-qht.c b/tests/test-qht.c index c8eb930..e2f9e14 100644 --- a/tests/test-qht.c +++ b/tests/test-qht.c @@ -96,8 +96,12 @@ static void iter_check(unsigned int count) 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); + qht_init(&ht, 0, mode); + check_n(0); insert(0, N); check(0, N, true); check_n(N); diff --git a/util/qht.c b/util/qht.c index 6f74909..0cb95e2 100644 --- a/util/qht.c +++ b/util/qht.c @@ -783,11 +783,16 @@ void qht_statistics_init(struct qht *ht, struct qht_stats *stats) map = atomic_rcu_read(&ht->map); - stats->head_buckets = map->n_buckets; 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)) { + stats->head_buckets = 0; + return; + } + stats->head_buckets = map->n_buckets; for (i = 0; i < map->n_buckets; i++) { struct qht_bucket *head = &map->buckets[i];
So far, QHT functions assume that the passed qht has previously been initialized--otherwise they segfault. This patch makes an exception for qht_statistics_init, with the goal of simplifying calling code. For instance, qht_statistics_init is called from the 'info jit' dump, and given that under KVM the TB qht is never initialized, we get a segfault. Thus, instead of complicating the 'info jit' code with additional checks, let's allow passing an uninitialized qht to qht_statistics_init. While at it, add a test for this to test-qht. Before the patch (for $ qemu -enable-kvm [...]): (qemu) info jit [...] direct jump count 0 (0%) (2 jumps=0 0%) Program received signal SIGSEGV, Segmentation fault. After the patch: (qemu) info jit [...] direct jump count 0 (0%) (2 jumps=0 0%) TB hash buckets 0/0 (-nan% head buckets used) TB hash occupancy nan% avg chain occ. Histogram: (null) TB hash avg chain nan buckets. Histogram: (null) [...] Reported by: Changlong Xie <xiecl.fnst@cn.fujitsu.com> Signed-off-by: Emilio G. Cota <cota@braap.org> --- tests/test-qht.c | 4 ++++ util/qht.c | 7 ++++++- 2 files changed, 10 insertions(+), 1 deletion(-)