qemu: Add missing lock in qemuProcessHandleMonitorEOF

[PATCH v2] qemu: Add missing lock in qemuProcessHandleMonitorEOF

Posted by Peng Liang 3 years, 1 month ago

qemuMonitorUnregister will be called in multiple threads (e.g. threads
in rpc worker pool and the vm event thread).  In some cases, it isn't
protected by the monitor lock, which may lead to call g_source_unref
more than one time and a use-after-free problem eventually.

Add the missing lock in qemuProcessHandleMonitorEOF (which is the only
position missing lock of monitor I found).

Suggested-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
This patch is v2 of https://listman.redhat.com/archives/libvir-list/2021-February/msg00945.html.

v1 -> v2:
Locking monitor in qemuProcessHandleMonitorEOF instead of using aotmic
function in qemuMonitorUnregister.

 src/qemu/qemu_monitor.h | 1 +
 src/qemu/qemu_process.c | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index d25c26343a7f..14e6b1fe9626 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -427,6 +427,7 @@ bool qemuMonitorWasDisposed(void);
 
 void qemuMonitorRegister(qemuMonitorPtr mon)
     ATTRIBUTE_NONNULL(1);
+/* Must be called with monitor locked. */
 void qemuMonitorUnregister(qemuMonitorPtr mon)
     ATTRIBUTE_NONNULL(1);
 void qemuMonitorClose(qemuMonitorPtr mon);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index d930ff9a74f6..bfa742577f32 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -318,7 +318,9 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon,
     /* We don't want this EOF handler to be called over and over while the
      * thread is waiting for a job.
      */
+    virObjectLock(mon);
     qemuMonitorUnregister(mon);
+    virObjectUnlock(mon);
 
     /* We don't want any cleanup from EOF handler (or any other
      * thread) to enter qemu namespace. */
-- 
2.29.2

Re: [PATCH v2] qemu: Add missing lock in qemuProcessHandleMonitorEOF

Posted by Michal Privoznik 3 years, 1 month ago

On 2/24/21 12:28 PM, Peng Liang wrote:
> qemuMonitorUnregister will be called in multiple threads (e.g. threads
> in rpc worker pool and the vm event thread).  In some cases, it isn't
> protected by the monitor lock, which may lead to call g_source_unref
> more than one time and a use-after-free problem eventually.
> 
> Add the missing lock in qemuProcessHandleMonitorEOF (which is the only
> position missing lock of monitor I found).
> 
> Suggested-by: Michal Privoznik <mprivozn@redhat.com>
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
> This patch is v2 of https://listman.redhat.com/archives/libvir-list/2021-February/msg00945.html.
> 
> v1 -> v2:
> Locking monitor in qemuProcessHandleMonitorEOF instead of using aotmic
> function in qemuMonitorUnregister.
> 
>   src/qemu/qemu_monitor.h | 1 +
>   src/qemu/qemu_process.c | 2 ++
>   2 files changed, 3 insertions(+)
> 
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index d25c26343a7f..14e6b1fe9626 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -427,6 +427,7 @@ bool qemuMonitorWasDisposed(void);
>   
>   void qemuMonitorRegister(qemuMonitorPtr mon)
>       ATTRIBUTE_NONNULL(1);
> +/* Must be called with monitor locked. */
>   void qemuMonitorUnregister(qemuMonitorPtr mon)

 From this it's not very clear which function the comment belongs to. We 
tend to put comments into .c because that's where tags usually take you 
first. So you get the memo first.

>       ATTRIBUTE_NONNULL(1);
>   void qemuMonitorClose(qemuMonitorPtr mon);
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index d930ff9a74f6..bfa742577f32 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -318,7 +318,9 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon,
>       /* We don't want this EOF handler to be called over and over while the
>        * thread is waiting for a job.
>        */
> +    virObjectLock(mon);
>       qemuMonitorUnregister(mon);
> +    virObjectUnlock(mon);
>   
>       /* We don't want any cleanup from EOF handler (or any other
>        * thread) to enter qemu namespace. */
> 

ACK to this hunk. And I'll be pushing it in a few moments.

Michal

Re: [PATCH v2] qemu: Add missing lock in qemuProcessHandleMonitorEOF

Posted by Peng Liang 3 years, 1 month ago

On 2/24/2021 9:59 PM, Michal Privoznik wrote:
> On 2/24/21 12:28 PM, Peng Liang wrote:
>> qemuMonitorUnregister will be called in multiple threads (e.g. threads
>> in rpc worker pool and the vm event thread).  In some cases, it isn't
>> protected by the monitor lock, which may lead to call g_source_unref
>> more than one time and a use-after-free problem eventually.
>>
>> Add the missing lock in qemuProcessHandleMonitorEOF (which is the only
>> position missing lock of monitor I found).
>>
>> Suggested-by: Michal Privoznik <mprivozn@redhat.com>
>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>> ---
>> This patch is v2 of
>> https://listman.redhat.com/archives/libvir-list/2021-February/msg00945.html.
>>
>>
>> v1 -> v2:
>> Locking monitor in qemuProcessHandleMonitorEOF instead of using aotmic
>> function in qemuMonitorUnregister.
>>
>>   src/qemu/qemu_monitor.h | 1 +
>>   src/qemu/qemu_process.c | 2 ++
>>   2 files changed, 3 insertions(+)
>>
>> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
>> index d25c26343a7f..14e6b1fe9626 100644
>> --- a/src/qemu/qemu_monitor.h
>> +++ b/src/qemu/qemu_monitor.h
>> @@ -427,6 +427,7 @@ bool qemuMonitorWasDisposed(void);
>>     void qemuMonitorRegister(qemuMonitorPtr mon)
>>       ATTRIBUTE_NONNULL(1);
>> +/* Must be called with monitor locked. */
>>   void qemuMonitorUnregister(qemuMonitorPtr mon)
> 
> From this it's not very clear which function the comment belongs to. We
> tend to put comments into .c because that's where tags usually take you
> first. So you get the memo first.

Hi Michal,

So, do I need to send another patch to document it in
src/qemu/qemu_monitor.c?

Thanks,
Peng

> 
>>       ATTRIBUTE_NONNULL(1);
>>   void qemuMonitorClose(qemuMonitorPtr mon);
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index d930ff9a74f6..bfa742577f32 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -318,7 +318,9 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon,
>>       /* We don't want this EOF handler to be called over and over
>> while the
>>        * thread is waiting for a job.
>>        */
>> +    virObjectLock(mon);
>>       qemuMonitorUnregister(mon);
>> +    virObjectUnlock(mon);
>>         /* We don't want any cleanup from EOF handler (or any other
>>        * thread) to enter qemu namespace. */
>>
> 
> ACK to this hunk. And I'll be pushing it in a few moments.
> 
> Michal
> 
> .

Re: [PATCH v2] qemu: Add missing lock in qemuProcessHandleMonitorEOF

Posted by Michal Privoznik 3 years, 1 month ago

On 2/25/21 2:39 AM, Peng Liang wrote:
> On 2/24/2021 9:59 PM, Michal Privoznik wrote:
>> On 2/24/21 12:28 PM, Peng Liang wrote:
>>> qemuMonitorUnregister will be called in multiple threads (e.g. threads
>>> in rpc worker pool and the vm event thread).  In some cases, it isn't
>>> protected by the monitor lock, which may lead to call g_source_unref
>>> more than one time and a use-after-free problem eventually.
>>>
>>> Add the missing lock in qemuProcessHandleMonitorEOF (which is the only
>>> position missing lock of monitor I found).
>>>
>>> Suggested-by: Michal Privoznik <mprivozn@redhat.com>
>>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>>> ---
>>> This patch is v2 of
>>> https://listman.redhat.com/archives/libvir-list/2021-February/msg00945.html.
>>>
>>>
>>> v1 -> v2:
>>> Locking monitor in qemuProcessHandleMonitorEOF instead of using aotmic
>>> function in qemuMonitorUnregister.
>>>
>>>    src/qemu/qemu_monitor.h | 1 +
>>>    src/qemu/qemu_process.c | 2 ++
>>>    2 files changed, 3 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
>>> index d25c26343a7f..14e6b1fe9626 100644
>>> --- a/src/qemu/qemu_monitor.h
>>> +++ b/src/qemu/qemu_monitor.h
>>> @@ -427,6 +427,7 @@ bool qemuMonitorWasDisposed(void);
>>>      void qemuMonitorRegister(qemuMonitorPtr mon)
>>>        ATTRIBUTE_NONNULL(1);
>>> +/* Must be called with monitor locked. */
>>>    void qemuMonitorUnregister(qemuMonitorPtr mon)
>>
>>  From this it's not very clear which function the comment belongs to. We
>> tend to put comments into .c because that's where tags usually take you
>> first. So you get the memo first.
> 
> Hi Michal,
> 
> So, do I need to send another patch to document it in
> src/qemu/qemu_monitor.c?

No need I just did:

https://listman.redhat.com/archives/libvir-list/2021-February/msg01202.html

I'll merge it later today.

Michal