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

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

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~