Add judgement in compress_threads_save_cleanup() to check whether the
static CompressParam *comp_param has been allocated. If not, just
return; or else Segmentation fault will occur when using the
NULL comp_param's parameters in terminate_compression_threads().
One test case can reproduce this error is: set the compression on
and migrate to a wrong nonexistent host IP address.
Add judgement before handling comp_param[idx]'s quit and cond in
terminate_compression_threads(), in case they are not initialized.
Or else "qemu_mutex_lock_impl: Assertion `mutex->initialized' failed."
will occur. One test case can reproduce this error is: set the
compression on and fail to fully setup the eight compression thread
in compress_threads_save_setup().
Signed-off-by: Fei Li <fli@suse.com>
---
migration/ram.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/migration/ram.c b/migration/ram.c
index 79c89425a3..522a5550b4 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -427,6 +427,9 @@ static inline void terminate_compression_threads(void)
thread_count = migrate_compress_threads();
for (idx = 0; idx < thread_count; idx++) {
+ if (!comp_param[idx].mutex.initialized) {
+ break;
+ }
qemu_mutex_lock(&comp_param[idx].mutex);
comp_param[idx].quit = true;
qemu_cond_signal(&comp_param[idx].cond);
@@ -438,7 +441,7 @@ static void compress_threads_save_cleanup(void)
{
int i, thread_count;
- if (!migrate_use_compression()) {
+ if (!migrate_use_compression() || !comp_param) {
return;
}
terminate_compression_threads();
--
2.13.7
On Wed, Sep 19, 2018 at 09:35:20PM +0800, Fei Li wrote:
> Add judgement in compress_threads_save_cleanup() to check whether the
> static CompressParam *comp_param has been allocated. If not, just
> return; or else Segmentation fault will occur when using the
> NULL comp_param's parameters in terminate_compression_threads().
> One test case can reproduce this error is: set the compression on
> and migrate to a wrong nonexistent host IP address.
>
> Add judgement before handling comp_param[idx]'s quit and cond in
> terminate_compression_threads(), in case they are not initialized.
> Or else "qemu_mutex_lock_impl: Assertion `mutex->initialized' failed."
> will occur. One test case can reproduce this error is: set the
> compression on and fail to fully setup the eight compression thread
> in compress_threads_save_setup().
>
> Signed-off-by: Fei Li <fli@suse.com>
> ---
> migration/ram.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 79c89425a3..522a5550b4 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -427,6 +427,9 @@ static inline void terminate_compression_threads(void)
> thread_count = migrate_compress_threads();
>
> for (idx = 0; idx < thread_count; idx++) {
> + if (!comp_param[idx].mutex.initialized) {
> + break;
> + }
Instead of accessing the mutex internal variable "initialized", how
about we just squash the terminate_compression_threads() into existing
compress_threads_save_cleanup()? Note that we have this in
compress_threads_save_cleanup() already:
for (i = 0; i < thread_count; i++) {
/*
* we use it as a indicator which shows if the thread is
* properly init'd or not
*/
if (!comp_param[i].file) {
break;
}
qemu_thread_join(compress_threads + i);
qemu_mutex_destroy(&comp_param[i].mutex);
qemu_cond_destroy(&comp_param[i].cond);
deflateEnd(&comp_param[i].stream);
g_free(comp_param[i].originbuf);
qemu_fclose(comp_param[i].file);
comp_param[i].file = NULL;
}
So we already try to detect this using the comp_param[i].file, which
IMO is better (the exposure of the mutex.init var is just "an
accident" - logically we could hide that from mutex impl).
> qemu_mutex_lock(&comp_param[idx].mutex);
> comp_param[idx].quit = true;
> qemu_cond_signal(&comp_param[idx].cond);
> @@ -438,7 +441,7 @@ static void compress_threads_save_cleanup(void)
> {
> int i, thread_count;
>
> - if (!migrate_use_compression()) {
> + if (!migrate_use_compression() || !comp_param) {
This should be a valid fix for a crash (since we might call the
cleanup code even without setup when connect() never worked, so we'd
better handle it well).
Considering that this seems to fix a migration crash on source, I'm
CCing Dave and Juan in case this (or a new version) could even be a
good candidate as part of a migration pull.
Thanks,
> return;
> }
> terminate_compression_threads();
> --
> 2.13.7
>
>
--
Peter Xu
On 09/20/2018 12:31 PM, Peter Xu wrote:
> On Wed, Sep 19, 2018 at 09:35:20PM +0800, Fei Li wrote:
>> Add judgement in compress_threads_save_cleanup() to check whether the
>> static CompressParam *comp_param has been allocated. If not, just
>> return; or else Segmentation fault will occur when using the
>> NULL comp_param's parameters in terminate_compression_threads().
>> One test case can reproduce this error is: set the compression on
>> and migrate to a wrong nonexistent host IP address.
>>
>> Add judgement before handling comp_param[idx]'s quit and cond in
>> terminate_compression_threads(), in case they are not initialized.
>> Or else "qemu_mutex_lock_impl: Assertion `mutex->initialized' failed."
>> will occur. One test case can reproduce this error is: set the
>> compression on and fail to fully setup the eight compression thread
>> in compress_threads_save_setup().
>>
>> Signed-off-by: Fei Li <fli@suse.com>
>> ---
>> migration/ram.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 79c89425a3..522a5550b4 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -427,6 +427,9 @@ static inline void terminate_compression_threads(void)
>> thread_count = migrate_compress_threads();
>>
>> for (idx = 0; idx < thread_count; idx++) {
>> + if (!comp_param[idx].mutex.initialized) {
>> + break;
>> + }
> Instead of accessing the mutex internal variable "initialized", how
> about we just squash the terminate_compression_threads() into existing
> compress_threads_save_cleanup()? Note that we have this in
> compress_threads_save_cleanup() already:
>
> for (i = 0; i < thread_count; i++) {
> /*
> * we use it as a indicator which shows if the thread is
> * properly init'd or not
> */
> if (!comp_param[i].file) {
> break;
> }
> qemu_thread_join(compress_threads + i);
> qemu_mutex_destroy(&comp_param[i].mutex);
> qemu_cond_destroy(&comp_param[i].cond);
> deflateEnd(&comp_param[i].stream);
> g_free(comp_param[i].originbuf);
> qemu_fclose(comp_param[i].file);
> comp_param[i].file = NULL;
> }
>
> So we already try to detect this using the comp_param[i].file, which
> IMO is better (the exposure of the mutex.init var is just "an
> accident" - logically we could hide that from mutex impl).
Actually, I firstly use the comp_param[i].file as the judging condition, but
I am not sure whether the comp_param[i].file is also needed to be protected
by the mutex lock..
And I have no objection to the squash.
>
>> qemu_mutex_lock(&comp_param[idx].mutex);
>> comp_param[idx].quit = true;
>> qemu_cond_signal(&comp_param[idx].cond);
>> @@ -438,7 +441,7 @@ static void compress_threads_save_cleanup(void)
>> {
>> int i, thread_count;
>>
>> - if (!migrate_use_compression()) {
>> + if (!migrate_use_compression() || !comp_param) {
> This should be a valid fix for a crash (since we might call the
> cleanup code even without setup when connect() never worked, so we'd
> better handle it well).
>
> Considering that this seems to fix a migration crash on source, I'm
> CCing Dave and Juan in case this (or a new version) could even be a
> good candidate as part of a migration pull.
>
> Thanks,
Thanks for helping cc. ;)
Have a nice day
Fei
>
>> return;
>> }
>> terminate_compression_threads();
>> --
>> 2.13.7
>>
>>
On Thu, Sep 20, 2018 at 01:06:21PM +0800, Fei Li wrote:
>
>
> On 09/20/2018 12:31 PM, Peter Xu wrote:
> > On Wed, Sep 19, 2018 at 09:35:20PM +0800, Fei Li wrote:
> > > Add judgement in compress_threads_save_cleanup() to check whether the
> > > static CompressParam *comp_param has been allocated. If not, just
> > > return; or else Segmentation fault will occur when using the
> > > NULL comp_param's parameters in terminate_compression_threads().
> > > One test case can reproduce this error is: set the compression on
> > > and migrate to a wrong nonexistent host IP address.
> > >
> > > Add judgement before handling comp_param[idx]'s quit and cond in
> > > terminate_compression_threads(), in case they are not initialized.
> > > Or else "qemu_mutex_lock_impl: Assertion `mutex->initialized' failed."
> > > will occur. One test case can reproduce this error is: set the
> > > compression on and fail to fully setup the eight compression thread
> > > in compress_threads_save_setup().
> > >
> > > Signed-off-by: Fei Li <fli@suse.com>
> > > ---
> > > migration/ram.c | 5 ++++-
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/migration/ram.c b/migration/ram.c
> > > index 79c89425a3..522a5550b4 100644
> > > --- a/migration/ram.c
> > > +++ b/migration/ram.c
> > > @@ -427,6 +427,9 @@ static inline void terminate_compression_threads(void)
> > > thread_count = migrate_compress_threads();
> > > for (idx = 0; idx < thread_count; idx++) {
> > > + if (!comp_param[idx].mutex.initialized) {
> > > + break;
> > > + }
> > Instead of accessing the mutex internal variable "initialized", how
> > about we just squash the terminate_compression_threads() into existing
> > compress_threads_save_cleanup()? Note that we have this in
> > compress_threads_save_cleanup() already:
> >
> > for (i = 0; i < thread_count; i++) {
> > /*
> > * we use it as a indicator which shows if the thread is
> > * properly init'd or not
> > */
> > if (!comp_param[i].file) {
> > break;
> > }
> > qemu_thread_join(compress_threads + i);
> > qemu_mutex_destroy(&comp_param[i].mutex);
> > qemu_cond_destroy(&comp_param[i].cond);
> > deflateEnd(&comp_param[i].stream);
> > g_free(comp_param[i].originbuf);
> > qemu_fclose(comp_param[i].file);
> > comp_param[i].file = NULL;
> > }
> >
> > So we already try to detect this using the comp_param[i].file, which
> > IMO is better (the exposure of the mutex.init var is just "an
> > accident" - logically we could hide that from mutex impl).
> Actually, I firstly use the comp_param[i].file as the judging condition, but
> I am not sure whether the comp_param[i].file is also needed to be protected
> by the mutex lock..
> And I have no objection to the squash.
It should be fine to only check non-zero. This is slightly tricky so
we have had some comment there which clarifies it a bit.
> >
> > > qemu_mutex_lock(&comp_param[idx].mutex);
> > > comp_param[idx].quit = true;
> > > qemu_cond_signal(&comp_param[idx].cond);
> > > @@ -438,7 +441,7 @@ static void compress_threads_save_cleanup(void)
> > > {
> > > int i, thread_count;
> > > - if (!migrate_use_compression()) {
> > > + if (!migrate_use_compression() || !comp_param) {
> > This should be a valid fix for a crash (since we might call the
> > cleanup code even without setup when connect() never worked, so we'd
> > better handle it well).
> >
> > Considering that this seems to fix a migration crash on source, I'm
> > CCing Dave and Juan in case this (or a new version) could even be a
> > good candidate as part of a migration pull.
> >
> > Thanks,
> Thanks for helping cc. ;)
No problem. If you're gonna post another version, feel free to CC
Dave and Juan on this patch, and you can post this as a standalone one
since it's not directly related with the series and it fixes an even
more severe issue on migration side (source VM will data-loss if this
is triggerred; and it triggers easily).
Regards,
--
Peter Xu
Sorry for the late reply.
On 09/20/2018 01:33 PM, Peter Xu wrote:
> On Thu, Sep 20, 2018 at 01:06:21PM +0800, Fei Li wrote:
>>
>> On 09/20/2018 12:31 PM, Peter Xu wrote:
>>> On Wed, Sep 19, 2018 at 09:35:20PM +0800, Fei Li wrote:
>>>> Add judgement in compress_threads_save_cleanup() to check whether the
>>>> static CompressParam *comp_param has been allocated. If not, just
>>>> return; or else Segmentation fault will occur when using the
>>>> NULL comp_param's parameters in terminate_compression_threads().
>>>> One test case can reproduce this error is: set the compression on
>>>> and migrate to a wrong nonexistent host IP address.
>>>>
>>>> Add judgement before handling comp_param[idx]'s quit and cond in
>>>> terminate_compression_threads(), in case they are not initialized.
>>>> Or else "qemu_mutex_lock_impl: Assertion `mutex->initialized' failed."
>>>> will occur. One test case can reproduce this error is: set the
>>>> compression on and fail to fully setup the eight compression thread
>>>> in compress_threads_save_setup().
>>>>
>>>> Signed-off-by: Fei Li <fli@suse.com>
>>>> ---
>>>> migration/ram.c | 5 ++++-
>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/migration/ram.c b/migration/ram.c
>>>> index 79c89425a3..522a5550b4 100644
>>>> --- a/migration/ram.c
>>>> +++ b/migration/ram.c
>>>> @@ -427,6 +427,9 @@ static inline void terminate_compression_threads(void)
>>>> thread_count = migrate_compress_threads();
>>>> for (idx = 0; idx < thread_count; idx++) {
>>>> + if (!comp_param[idx].mutex.initialized) {
>>>> + break;
>>>> + }
>>> Instead of accessing the mutex internal variable "initialized", how
>>> about we just squash the terminate_compression_threads() into existing
>>> compress_threads_save_cleanup()? Note that we have this in
>>> compress_threads_save_cleanup() already:
>>>
>>> for (i = 0; i < thread_count; i++) {
>>> /*
>>> * we use it as a indicator which shows if the thread is
>>> * properly init'd or not
>>> */
>>> if (!comp_param[i].file) {
>>> break;
>>> }
>>> qemu_thread_join(compress_threads + i);
>>> qemu_mutex_destroy(&comp_param[i].mutex);
>>> qemu_cond_destroy(&comp_param[i].cond);
>>> deflateEnd(&comp_param[i].stream);
>>> g_free(comp_param[i].originbuf);
>>> qemu_fclose(comp_param[i].file);
>>> comp_param[i].file = NULL;
>>> }
>>>
>>> So we already try to detect this using the comp_param[i].file, which
>>> IMO is better (the exposure of the mutex.init var is just "an
>>> accident" - logically we could hide that from mutex impl).
>> Actually, I firstly use the comp_param[i].file as the judging condition, but
>> I am not sure whether the comp_param[i].file is also needed to be protected
>> by the mutex lock..
>> And I have no objection to the squash.
> It should be fine to only check non-zero. This is slightly tricky so
> we have had some comment there which clarifies it a bit.
Ok, I will do the squash.
>
>>>> qemu_mutex_lock(&comp_param[idx].mutex);
>>>> comp_param[idx].quit = true;
>>>> qemu_cond_signal(&comp_param[idx].cond);
>>>> @@ -438,7 +441,7 @@ static void compress_threads_save_cleanup(void)
>>>> {
>>>> int i, thread_count;
>>>> - if (!migrate_use_compression()) {
>>>> + if (!migrate_use_compression() || !comp_param) {
>>> This should be a valid fix for a crash (since we might call the
>>> cleanup code even without setup when connect() never worked, so we'd
>>> better handle it well).
>>>
>>> Considering that this seems to fix a migration crash on source, I'm
>>> CCing Dave and Juan in case this (or a new version) could even be a
>>> good candidate as part of a migration pull.
>>>
>>> Thanks,
>> Thanks for helping cc. ;)
> No problem. If you're gonna post another version, feel free to CC
> Dave and Juan on this patch, and you can post this as a standalone one
> since it's not directly related with the series and it fixes an even
> more severe issue on migration side (source VM will data-loss if this
> is triggerred; and it triggers easily).
>
> Regards,
>
Thanks for the suggestion! I'd like to post a standalone patch then. :)
Have a nice day
Fei
© 2016 - 2026 Red Hat, Inc.