[v1] daemon: Fix a crash during virNetlinkEventServiceStopAll

[libvirt] [PATCH]daemon: Fix a crash during virNetlinkEventServiceStopAll

Posted by Liu Haitao 4 years, 10 months ago

The virNetlinkEventServiceStopAll() should be executed behind virStateCleanup(),
for some important resources like(static virNetlinkEventSrvPrivatePtr server)
are freed unexpected. However virStateCleanup() need to use this
variable(server).

The call trace of virNetlinkEventServiceStopAll:

virNetlinkEventServiceStopAll()
	--> virNetlinkEventServiceStop()
	  --> server[protocol] = NULL;   // set server to null 

The call trace of virStateCleanup():
virStateCleanup()
	-->qemuStateCleanup()
	  -->qemuProcessStop()
	    -->virNetDevMacVLanDeleteWithVPortProfile()
	     -->virNetlinkEventRemoveClient()
	       --> srv = server[protocol] 

In virNetlinkEventRemoveClient() the variable server is used again, but now it
is null that is freed by virNetlinkEventServiceStopAll().So it would case a crash .

The call trace of crash:

(gdb) bt
0  __GI___pthread_mutex_lock (mutex=0x0) at /usr/src/debug/glibc/2.24-r0/git/nptl/pthread_mutex_lock.c:67
1  0x00007fb0d555d0f9 in virNetlinkEventRemoveClient () from /usr/lib64/libvirt.so.0
2  0x00007fb0d55551df in virNetDevMacVLanDeleteWithVPortProfile () from /usr/lib64/libvirt.so.0
3  0x00007fb0c1131251 in qemuProcessStop () from /usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so
4  0x00007fb0c11995ea in ?? () from /usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so
5  0x00007fb0d5588c5b in ?? () from /usr/lib64/libvirt.so.0
6  0x00007fb0d5587fe8 in ?? () from /usr/lib64/libvirt.so.0
7  0x00007fb0d19533f4 in start_thread (arg=0x7fb0be17b700) at /usr/src/debug/glibc/2.24-r0/git/nptl/pthread_create.c:456
8  0x00007fb0d128f10f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:105

Signed-off-by: Liu Haitao <haitao.liu@windriver.com>
---
 src/remote/remote_daemon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index c3782971f1..7da20a6644 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -1464,8 +1464,6 @@ int main(int argc, char **argv) {
     /* Keep cleanup order in inverse order of startup */
     virNetDaemonClose(dmn);
 
-    virNetlinkEventServiceStopAll();
-
     if (driversInitialized) {
         /* NB: Possible issue with timing window between driversInitialized
          * setting if virNetlinkEventServerStart fails */
@@ -1473,6 +1471,8 @@ int main(int argc, char **argv) {
         virStateCleanup();
     }
 
+    virNetlinkEventServiceStopAll();
+
     virObjectUnref(adminProgram);
     virObjectUnref(srvAdm);
     virObjectUnref(qemuProgram);
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH]daemon: Fix a crash during virNetlinkEventServiceStopAll

Posted by Laine Stump 4 years, 10 months ago

On 6/10/19 2:15 AM, Liu Haitao wrote:
> The virNetlinkEventServiceStopAll() should be executed behind virStateCleanup(),
> for some important resources like(static virNetlinkEventSrvPrivatePtr server)
> are freed unexpected. However virStateCleanup() need to use this
> variable(server).
>
> The call trace of virNetlinkEventServiceStopAll:
>
> virNetlinkEventServiceStopAll()
> 	--> virNetlinkEventServiceStop()
> 	  --> server[protocol] = NULL;   // set server to null
>
> The call trace of virStateCleanup():
> virStateCleanup()
> 	-->qemuStateCleanup()
> 	  -->qemuProcessStop()


Where does qemuStateCleanup() call qemuProcessStop()?


(If you're basing this on the comment at the top of qemuStateCleanup 
that says "it will stop all active domains and networks", note that that 
comment was added to the code in commit b4c282a79 on June 29, 2007. That 
was before my time, so I don't know if it was true at that time that 
shutting down libvirtd would stop all active domains and networks, but 
it definitely is not true now (and hasn't been for at least 10 years).


What were the exact steps to cause this crash?


> 	    -->virNetDevMacVLanDeleteWithVPortProfile()
> 	     -->virNetlinkEventRemoveClient()
> 	       --> srv = server[protocol]
>
> In virNetlinkEventRemoveClient() the variable server is used again, but now it
> is null that is freed by virNetlinkEventServiceStopAll().So it would case a crash .
>
> The call trace of crash:
>
> (gdb) bt
> 0  __GI___pthread_mutex_lock (mutex=0x0) at /usr/src/debug/glibc/2.24-r0/git/nptl/pthread_mutex_lock.c:67
> 1  0x00007fb0d555d0f9 in virNetlinkEventRemoveClient () from /usr/lib64/libvirt.so.0
> 2  0x00007fb0d55551df in virNetDevMacVLanDeleteWithVPortProfile () from /usr/lib64/libvirt.so.0
> 3  0x00007fb0c1131251 in qemuProcessStop () from /usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so
> 4  0x00007fb0c11995ea in ?? () from /usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so
> 5  0x00007fb0d5588c5b in ?? () from /usr/lib64/libvirt.so.0
> 6  0x00007fb0d5587fe8 in ?? () from /usr/lib64/libvirt.so.0
> 7  0x00007fb0d19533f4 in start_thread (arg=0x7fb0be17b700) at /usr/src/debug/glibc/2.24-r0/git/nptl/pthread_create.c:456
> 8  0x00007fb0d128f10f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:105
>
> Signed-off-by: Liu Haitao <haitao.liu@windriver.com>
> ---
>   src/remote/remote_daemon.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> index c3782971f1..7da20a6644 100644
> --- a/src/remote/remote_daemon.c
> +++ b/src/remote/remote_daemon.c
> @@ -1464,8 +1464,6 @@ int main(int argc, char **argv) {
>       /* Keep cleanup order in inverse order of startup */
>       virNetDaemonClose(dmn);
>   
> -    virNetlinkEventServiceStopAll();
> -
>       if (driversInitialized) {
>           /* NB: Possible issue with timing window between driversInitialized
>            * setting if virNetlinkEventServerStart fails */
> @@ -1473,6 +1471,8 @@ int main(int argc, char **argv) {
>           virStateCleanup();
>       }
>   
> +    virNetlinkEventServiceStopAll();
> +
>       virObjectUnref(adminProgram);
>       virObjectUnref(srvAdm);
>       virObjectUnref(qemuProgram);


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH]daemon: Fix a crash during virNetlinkEventServiceStopAll

Posted by Haitaoliu 4 years, 10 months ago

Thank you for reminding me of that comment.  Please ignore this patch, 
for the comments are inaccurate.

I will send it again with  steps to reproduce  the bug.

On 2019/6/11 09:39, Laine Stump wrote:
> On 6/10/19 2:15 AM, Liu Haitao wrote:
>> The virNetlinkEventServiceStopAll() should be executed behind 
>> virStateCleanup(),
>> for some important resources like(static virNetlinkEventSrvPrivatePtr 
>> server)
>> are freed unexpected. However virStateCleanup() need to use this
>> variable(server).
>>
>> The call trace of virNetlinkEventServiceStopAll:
>>
>> virNetlinkEventServiceStopAll()
>>     --> virNetlinkEventServiceStop()
>>       --> server[protocol] = NULL;   // set server to null
>>
>> The call trace of virStateCleanup():
>> virStateCleanup()
>>     -->qemuStateCleanup()
>>       -->qemuProcessStop()
>
>
> Where does qemuStateCleanup() call qemuProcessStop()?
>
>
> (If you're basing this on the comment at the top of qemuStateCleanup 
> that says "it will stop all active domains and networks", note that 
> that comment was added to the code in commit b4c282a79 on June 29, 
> 2007. That was before my time, so I don't know if it was true at that 
> time that shutting down libvirtd would stop all active domains and 
> networks, but it definitely is not true now (and hasn't been for at 
> least 10 years).
>
>
> What were the exact steps to cause this crash?
>
>
>> -->virNetDevMacVLanDeleteWithVPortProfile()
>>          -->virNetlinkEventRemoveClient()
>>            --> srv = server[protocol]
>>
>> In virNetlinkEventRemoveClient() the variable server is used again, 
>> but now it
>> is null that is freed by virNetlinkEventServiceStopAll().So it would 
>> case a crash .
>>
>> The call trace of crash:
>>
>> (gdb) bt
>> 0  __GI___pthread_mutex_lock (mutex=0x0) at 
>> /usr/src/debug/glibc/2.24-r0/git/nptl/pthread_mutex_lock.c:67
>> 1  0x00007fb0d555d0f9 in virNetlinkEventRemoveClient () from 
>> /usr/lib64/libvirt.so.0
>> 2  0x00007fb0d55551df in virNetDevMacVLanDeleteWithVPortProfile () 
>> from /usr/lib64/libvirt.so.0
>> 3  0x00007fb0c1131251 in qemuProcessStop () from 
>> /usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so
>> 4  0x00007fb0c11995ea in ?? () from 
>> /usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so
>> 5  0x00007fb0d5588c5b in ?? () from /usr/lib64/libvirt.so.0
>> 6  0x00007fb0d5587fe8 in ?? () from /usr/lib64/libvirt.so.0
>> 7  0x00007fb0d19533f4 in start_thread (arg=0x7fb0be17b700) at 
>> /usr/src/debug/glibc/2.24-r0/git/nptl/pthread_create.c:456
>> 8  0x00007fb0d128f10f in clone () at 
>> ../sysdeps/unix/sysv/linux/x86_64/clone.S:105
>>
>> Signed-off-by: Liu Haitao <haitao.liu@windriver.com>
>> ---
>>   src/remote/remote_daemon.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
>> index c3782971f1..7da20a6644 100644
>> --- a/src/remote/remote_daemon.c
>> +++ b/src/remote/remote_daemon.c
>> @@ -1464,8 +1464,6 @@ int main(int argc, char **argv) {
>>       /* Keep cleanup order in inverse order of startup */
>>       virNetDaemonClose(dmn);
>>   -    virNetlinkEventServiceStopAll();
>> -
>>       if (driversInitialized) {
>>           /* NB: Possible issue with timing window between 
>> driversInitialized
>>            * setting if virNetlinkEventServerStart fails */
>> @@ -1473,6 +1471,8 @@ int main(int argc, char **argv) {
>>           virStateCleanup();
>>       }
>>   +    virNetlinkEventServiceStopAll();
>> +
>>       virObjectUnref(adminProgram);
>>       virObjectUnref(srvAdm);
>>       virObjectUnref(qemuProgram);
>
>
>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list