Re: [PATCH 3/6] plugins/cache: Fixed a use-after-free bug with multithreaded usermode

[Alex Bennée]

Mahmoud Mandour <ma.mandourr@gmail.com> writes:

> Since callbacks may be interleaved because of multithreaded execution,
> we should not make assumptions about `plugin_exit` either. The problem
> with `plugin_exit` is that it frees shared data structures (caches and
> `miss_ht` hash table). It should not be assumed that callbacks will not
> be called after it and hence use already-freed data structures.

What was your test case for this because I wonder if it would be worth
coming up with one for check-tcg? From what I remember the race is
in between preexit_cleanup and the eventual _exit/exit_group which nixes
all other child threads. Maybe we should be triggering a more graceful
unload here?

> This is mitigated in this commit by synchronizing the call to
> `plugin_exit` through locking to ensure execlusive access to data
> structures, and NULL-ifying those data structures so that subsequent
> callbacks can check whether the data strucutres are freed, and if so,
> immediately exit.
>
> It's okay to immediately exit and don't account for those callbacks
> since they won't be accounted for anyway since `plugin_exit` is already
> called once and reported the statistics.
>
> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> ---
>  contrib/plugins/cache.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c
> index 695fb969dc..a452aba01c 100644
> --- a/contrib/plugins/cache.c
> +++ b/contrib/plugins/cache.c
> @@ -363,6 +363,11 @@ static void vcpu_mem_access(unsigned int vcpu_index, 
> qemu_plugin_meminfo_t info,
>      effective_addr = hwaddr ? qemu_plugin_hwaddr_phys_addr(hwaddr) : vaddr;
>  
>      g_mutex_lock(&mtx);
> +    if (dcache == NULL) {
> +        g_mutex_unlock(&mtx);
> +        return;
> +    }
> +
>      if (!access_cache(dcache, effective_addr)) {
>          insn = (InsnData *) userdata;
>          insn->dmisses++;
> @@ -380,6 +385,11 @@ static void vcpu_insn_exec(unsigned int vcpu_index, void 
> *userdata)
>      g_mutex_lock(&mtx);
>      insn_addr = ((InsnData *) userdata)->addr;
>  
> +    if (icache == NULL) {
> +        g_mutex_unlock(&mtx);
> +        return;
> +    }
> +
>      if (!access_cache(icache, insn_addr)) {
>          insn = (InsnData *) userdata;
>          insn->imisses++;
> @@ -406,12 +416,24 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct 
> qemu_plugin_tb *tb)
>              effective_addr = (uint64_t) qemu_plugin_insn_vaddr(insn);
>          }
>  
> +        g_mutex_lock(&mtx);
> +
> +        /*
> +         * is the plugin_exit callback called? If so, any further callback
> +         * registration is useless as it won't get accounted for after 
> calling
> +         * plugin_exit once already, and also will use miss_ht after it's 
> freed
> +         */
> +        if (miss_ht == NULL) {
> +            g_mutex_unlock(&mtx);
> +            return;
> +        }
> +
>          /*
>           * Instructions might get translated multiple times, we do not create
>           * new entries for those instructions. Instead, we fetch the same
>           * entry from the hash table and register it for the callback again.
>           */
> -        g_mutex_lock(&mtx);
> +
>          data = g_hash_table_lookup(miss_ht, 
> GUINT_TO_POINTER(effective_addr));
>          if (data == NULL) {
>              data = g_new0(InsnData, 1);
> @@ -527,13 +549,20 @@ static void log_top_insns()
>  
>  static void plugin_exit(qemu_plugin_id_t id, void *p)
>  {
> +    g_mutex_lock(&mtx);
>      log_stats();
>      log_top_insns();
>  
>      cache_free(dcache);
> +    dcache = NULL;
> +
>      cache_free(icache);
> +    icache = NULL;
>  
>      g_hash_table_destroy(miss_ht);
> +    miss_ht = NULL;
> +
> +    g_mutex_unlock(&mtx);
>  }
>  
>  static void policy_init()


-- 
Alex Bennée