Re: [Qemu-devel] [PATCH] qht: do not segfault when gathering stats from an uninitialized qht

[Paolo Bonzini]

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 <address@hidden>
> Signed-off-by: Emilio G. Cota <address@hidden>
> ---
>  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