[PATCH] plugins: fix race condition with scoreboards

[PATCH] plugins: fix race condition with scoreboards

[Pierrick Bouvier]

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);
     struct qemu_plugin_scoreboard *score;
     QLIST_FOREACH(score, &plugin.scoreboards, entry) {
-        g_array_set_size(score->data, plugin.scoreboard_alloc_size);
+        g_array_set_size(score->data, scoreboard_size);
     }
+    plugin.scoreboard_alloc_size = scoreboard_size;
     /* force all tb to be flushed, as scoreboard pointers were changed. */
     tb_flush(cpu);
     end_exclusive();
-- 
2.39.2

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

[Pierrick Bouvier]

@Alex: If possible, this patch should be included for the release to come.

On 7/22/24 16: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);
>       struct qemu_plugin_scoreboard *score;
>       QLIST_FOREACH(score, &plugin.scoreboards, entry) {
> -        g_array_set_size(score->data, plugin.scoreboard_alloc_size);
> +        g_array_set_size(score->data, scoreboard_size);
>       }
> +    plugin.scoreboard_alloc_size = scoreboard_size;
>       /* force all tb to be flushed, as scoreboard pointers were changed. */
>       tb_flush(cpu);
>       end_exclusive();

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~

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

[Pierrick Bouvier]

On 7/28/24 20:30, Richard Henderson wrote:
> 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.
> 

Added this check (it's size > alloc_size instead).

> 
> r~