Re: [PATCH] plugins: fix race condition with scoreboards

[Richard Henderson]

On 7/23/24 09:23, Pierrick Bouvier wrote:
> A deadlock can be created if a new vcpu (a) triggers a scoreboard
> reallocation, and another vcpu (b) wants to create a new scoreboard at
> the same time.
>
> In this case, (a) holds the plugin lock, and starts an exclusive
> section, waiting for (b). But at the same time, (b) is waiting for
> plugin lock.
>
> The solution is to drop the lock before entering the exclusive section.
>
> This bug can be easily reproduced by creating a callback for any tb
> exec, that allocates a new scoreboard. In this case, as soon as we reach
> more than 16 vcpus, the deadlock occurs.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2344
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   plugins/core.c | 29 +++++++++++++++++++++++------
>   1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/plugins/core.c b/plugins/core.c
> index 12c67b4b4eb..e31a5c1c9cc 100644
> --- a/plugins/core.c
> +++ b/plugins/core.c
> @@ -214,28 +214,45 @@ CPUPluginState *qemu_plugin_create_vcpu_state(void)
>   
>   static void plugin_grow_scoreboards__locked(CPUState *cpu)
>   {
> -    if (cpu->cpu_index < plugin.scoreboard_alloc_size) {
> +    size_t scoreboard_size = plugin.scoreboard_alloc_size;
> +    if (cpu->cpu_index < scoreboard_size) {
>           return;
>       }
>   
>       bool need_realloc = FALSE;
> -    while (cpu->cpu_index >= plugin.scoreboard_alloc_size) {
> -        plugin.scoreboard_alloc_size *= 2;
> +    while (cpu->cpu_index >= scoreboard_size) {
> +        scoreboard_size *= 2;
>           need_realloc = TRUE;
>       }
>   
> +    if (!need_realloc) {
> +        return;
> +    }
>   
> -    if (!need_realloc || QLIST_EMPTY(&plugin.scoreboards)) {
> -        /* nothing to do, we just updated sizes for future scoreboards */
> +    if (QLIST_EMPTY(&plugin.scoreboards)) {
> +        /* just update size for future scoreboards */
> +        plugin.scoreboard_alloc_size = scoreboard_size;
>           return;
>       }
>   
> +    /*
> +     * A scoreboard creation/deletion might be in progress. If a new vcpu is
> +     * initialized at the same time, we are safe, as the new
> +     * plugin.scoreboard_alloc_size was not yet written.
> +     */
> +    qemu_rec_mutex_unlock(&plugin.lock);
> +
>       /* cpus must be stopped, as tb might still use an existing scoreboard. */
>       start_exclusive();
> +    /* re-acquire lock */
> +    qemu_rec_mutex_lock(&plugin.lock);
> +    /* in case another vcpu is created between unlock and exclusive section. */
> +    scoreboard_size = MAX(scoreboard_size, plugin.scoreboard_alloc_size);

Rather than MAX, if a concurrent resize just completed, we don't need to resize 
again.

So:

  start_exclusive
  lock
  if (size < alloc_size) {
     foreach
     flush
  }
  unlock
  end_exclusive.


r~