[v2] net/colo-compare.c: Fix a crash in COLO Primary.

[Qemu-devel] [PATCH v2] net/colo-compare.c: Fix a crash in COLO Primary.

Posted by Lukas Straub 4 years, 12 months ago

From: Lukas Straub <lukasstraub2@web.de>
Because event_unhandled_count may be accessed concurrently, it needs
to be protected by taking the lock. However the assert is outside the
lock, probably causing it to read garbage and aborting Qemu erroneously.

The Bug only happens when running Qemu in COLO mode.

This Patch fixes the following bug: https://bugs.launchpad.net/qemu/+bug/1824622

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 net/colo-compare.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index bf10526f05..fcb491121b 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -813,9 +813,8 @@ static void colo_compare_handle_event(void *opaque)
         break;
     }

-    assert(event_unhandled_count > 0);
-
     qemu_mutex_lock(&event_mtx);
+    assert(event_unhandled_count > 0);
     event_unhandled_count--;
     qemu_cond_broadcast(&event_complete_cond);
     qemu_mutex_unlock(&event_mtx);
--
2.20.1

Re: [Qemu-devel] [PATCH v2] net/colo-compare.c: Fix a crash in COLO Primary.

Posted by Philippe Mathieu-Daudé 4 years, 12 months ago

On 4/20/19 7:14 PM, Lukas Straub wrote:
> From: Lukas Straub <lukasstraub2@web.de>
> Because event_unhandled_count may be accessed concurrently, it needs
> to be protected by taking the lock. However the assert is outside the
> lock, probably causing it to read garbage and aborting Qemu erroneously.
> 
> The Bug only happens when running Qemu in COLO mode.
> 
> This Patch fixes the following bug: https://bugs.launchpad.net/qemu/+bug/1824622
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>  net/colo-compare.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index bf10526f05..fcb491121b 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -813,9 +813,8 @@ static void colo_compare_handle_event(void *opaque)
>          break;
>      }
> 
> -    assert(event_unhandled_count > 0);
> -
>      qemu_mutex_lock(&event_mtx);
> +    assert(event_unhandled_count > 0);
>      event_unhandled_count--;
>      qemu_cond_broadcast(&event_complete_cond);
>      qemu_mutex_unlock(&event_mtx);
> --
> 2.20.1
> 
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [Qemu-devel] [PATCH v2] net/colo-compare.c: Fix a crash in COLO Primary.

Posted by Philippe Mathieu-Daudé 4 years, 11 months ago

Cc'ing Paolo & Stefan

On 4/20/19 7:14 PM, Lukas Straub wrote:
> From: Lukas Straub <lukasstraub2@web.de>
> Because event_unhandled_count may be accessed concurrently, it needs
> to be protected by taking the lock. However the assert is outside the
> lock, probably causing it to read garbage and aborting Qemu erroneously.
> 
> The Bug only happens when running Qemu in COLO mode.
> 
> This Patch fixes the following bug: https://bugs.launchpad.net/qemu/+bug/1824622
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>  net/colo-compare.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index bf10526f05..fcb491121b 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -813,9 +813,8 @@ static void colo_compare_handle_event(void *opaque)
>          break;
>      }
> 
> -    assert(event_unhandled_count > 0);
> -
>      qemu_mutex_lock(&event_mtx);
> +    assert(event_unhandled_count > 0);
>      event_unhandled_count--;
>      qemu_cond_broadcast(&event_complete_cond);
>      qemu_mutex_unlock(&event_mtx);
> --
> 2.20.1
> 
>

Re: [Qemu-devel] [PATCH v2] net/colo-compare.c: Fix a crash in COLO Primary.

Posted by Lukas Straub 4 years, 11 months ago

On Sat, 20 Apr 2019 19:14:25 +0200
Lukas Straub <lukasstraub2@web.de> wrote:

> From: Lukas Straub <lukasstraub2@web.de>
> Because event_unhandled_count may be accessed concurrently, it needs
> to be protected by taking the lock. However the assert is outside the
> lock, probably causing it to read garbage and aborting Qemu
> erroneously.
>
> The Bug only happens when running Qemu in COLO mode.
>
> This Patch fixes the following bug:
> https://bugs.launchpad.net/qemu/+bug/1824622
>
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>  net/colo-compare.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index bf10526f05..fcb491121b 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -813,9 +813,8 @@ static void colo_compare_handle_event(void
> *opaque) break;
>      }
>
> -    assert(event_unhandled_count > 0);
> -
>      qemu_mutex_lock(&event_mtx);
> +    assert(event_unhandled_count > 0);
>      event_unhandled_count--;
>      qemu_cond_broadcast(&event_complete_cond);
>      qemu_mutex_unlock(&event_mtx);

Ping.

Regards,
Lukas Straub

Re: [Qemu-devel] [PATCH v2] net/colo-compare.c: Fix a crash in COLO Primary.

Posted by Jason Wang 4 years, 11 months ago

On 2019/5/6 下午6:32, Lukas Straub wrote:
> On Sat, 20 Apr 2019 19:14:25 +0200
> Lukas Straub <lukasstraub2@web.de> wrote:
>
>> From: Lukas Straub <lukasstraub2@web.de>
>> Because event_unhandled_count may be accessed concurrently, it needs
>> to be protected by taking the lock. However the assert is outside the
>> lock, probably causing it to read garbage and aborting Qemu
>> erroneously.
>>
>> The Bug only happens when running Qemu in COLO mode.
>>
>> This Patch fixes the following bug:
>> https://bugs.launchpad.net/qemu/+bug/1824622
>>
>> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
>> ---
>>   net/colo-compare.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>> index bf10526f05..fcb491121b 100644
>> --- a/net/colo-compare.c
>> +++ b/net/colo-compare.c
>> @@ -813,9 +813,8 @@ static void colo_compare_handle_event(void
>> *opaque) break;
>>       }
>>
>> -    assert(event_unhandled_count > 0);
>> -
>>       qemu_mutex_lock(&event_mtx);
>> +    assert(event_unhandled_count > 0);
>>       event_unhandled_count--;
>>       qemu_cond_broadcast(&event_complete_cond);
>>       qemu_mutex_unlock(&event_mtx);
> Ping.
>
> Regards,
> Lukas Straub


Applied.

Thanks

>

Re: [Qemu-devel] [PATCH v2] net/colo-compare.c: Fix a crash in COLO Primary.

Posted by Zhang, Chen 4 years, 12 months ago

> -----Original Message-----
> From: Lukas Straub [mailto:lukasstraub2@web.de]
> Sent: Sunday, April 21, 2019 1:14 AM
> To: qemu-devel@nongnu.org
> Cc: Zhang, Chen <chen.zhang@intel.com>
> Subject: [PATCH v2] net/colo-compare.c: Fix a crash in COLO Primary.
> 
> From: Lukas Straub <lukasstraub2@web.de> Because event_unhandled_count
> may be accessed concurrently, it needs to be protected by taking the lock.
> However the assert is outside the lock, probably causing it to read garbage and
> aborting Qemu erroneously.
> 
> The Bug only happens when running Qemu in COLO mode.
> 
> This Patch fixes the following bug:
> https://bugs.launchpad.net/qemu/+bug/1824622
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>

Looks good for me.

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

Thanks
Zhang Chen

> ---
>  net/colo-compare.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> bf10526f05..fcb491121b 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -813,9 +813,8 @@ static void colo_compare_handle_event(void *opaque)
>          break;
>      }
> 
> -    assert(event_unhandled_count > 0);
> -
>      qemu_mutex_lock(&event_mtx);
> +    assert(event_unhandled_count > 0);
>      event_unhandled_count--;
>      qemu_cond_broadcast(&event_complete_cond);
>      qemu_mutex_unlock(&event_mtx);
> --
> 2.20.1