[libvirt] [PATCH] Fix bug libvirt daemon segfault when new force console vm session break down existed console session. When force console vm command arrived, libvirtd will break down existed console session, and in this procedure, it will call daemonStreamEvent() to release resources , so daemonStreamFilter() need to check if stream filter existed when get client object lock and client->privData's lock, if not existed, just return -1

LanceLiu posted 1 patch 4 years, 5 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1574667279-80696-1-git-send-email-liu.lance.89@gmail.com
src/libvirt_remote.syms           |  1 +
src/remote/remote_daemon_stream.c | 10 +++++++++-
src/rpc/virnetserverclient.c      | 12 ++++++++++++
src/rpc/virnetserverclient.h      |  2 ++
4 files changed, 24 insertions(+), 1 deletion(-)
[libvirt] [PATCH] Fix bug libvirt daemon segfault when new force console vm session break down existed console session. When force console vm command arrived, libvirtd will break down existed console session, and in this procedure, it will call daemonStreamEvent() to release resources , so daemonStreamFilter() need to check if stream filter existed when get client object lock and client->privData's lock, if not existed, just return -1
Posted by LanceLiu 4 years, 5 months ago
---
 src/libvirt_remote.syms           |  1 +
 src/remote/remote_daemon_stream.c | 10 +++++++++-
 src/rpc/virnetserverclient.c      | 12 ++++++++++++
 src/rpc/virnetserverclient.h      |  2 ++
 4 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 0493467..c32e234 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -173,6 +173,7 @@ virNetServerClientPreExecRestart;
 virNetServerClientRemoteAddrStringSASL;
 virNetServerClientRemoteAddrStringURI;
 virNetServerClientRemoveFilter;
+virNetServerClientCheckFilterExist;
 virNetServerClientSendMessage;
 virNetServerClientSetAuthLocked;
 virNetServerClientSetAuthPendingLocked;
diff --git a/src/remote/remote_daemon_stream.c b/src/remote/remote_daemon_stream.c
index 82cadb6..de0dca3 100644
--- a/src/remote/remote_daemon_stream.c
+++ b/src/remote/remote_daemon_stream.c
@@ -292,10 +292,18 @@ daemonStreamFilter(virNetServerClientPtr client,
 {
     daemonClientStream *stream = opaque;
     int ret = 0;
+    daemonClientPrivatePtr priv = NULL;
+    int filter_id = stream->filterID;
 
     virObjectUnlock(client);
+    priv = virNetServerClientGetPrivateData(client);
     virMutexLock(&stream->priv->lock);
     virObjectLock(client);
+    if (!virNetServerClientCheckFilterExist(client, filter_id)) {
+        VIR_WARN("this daemon stream filter: %d have been deleted!", filter_id);
+        ret = -1;
+        goto cleanup;
+    }
 
     if (msg->header.type != VIR_NET_STREAM &&
         msg->header.type != VIR_NET_STREAM_HOLE)
@@ -317,7 +325,7 @@ daemonStreamFilter(virNetServerClientPtr client,
     ret = 1;
 
  cleanup:
-    virMutexUnlock(&stream->priv->lock);
+    virMutexUnlock(&priv->lock);
     return ret;
 }
 
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 67b3bf9..f80f493 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -287,6 +287,18 @@ void virNetServerClientRemoveFilter(virNetServerClientPtr client,
     virObjectUnlock(client);
 }
 
+int virNetServerClientCheckFilterExist(virNetServerClientPtr client,
+                                       int filterID)
+{
+    virNetServerClientFilterPtr tmp;
+
+    tmp = client->filters;
+    while(tmp && tmp->id != filterID) {
+        tmp = tmp->next;
+    }
+
+    return (tmp != NULL);
+}
 
 /* Check the client's access. */
 static int
diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
index 7a3061d..85fda39 100644
--- a/src/rpc/virnetserverclient.h
+++ b/src/rpc/virnetserverclient.h
@@ -93,6 +93,8 @@ int virNetServerClientAddFilter(virNetServerClientPtr client,
 
 void virNetServerClientRemoveFilter(virNetServerClientPtr client,
                                     int filterID);
+int virNetServerClientCheckFilterExist(virNetServerClientPtr client,
+                                       int filterID);
 
 int virNetServerClientGetAuth(virNetServerClientPtr client);
 void virNetServerClientSetAuthLocked(virNetServerClientPtr client, int auth);
-- 
1.8.3.1


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

Re: [libvirt] [PATCH] Fix bug libvirt daemon segfault when new force console vm session break down existed console session. When force console vm command arrived, libvirtd will break down existed console session, and in this procedure, it will call daemonStreamEvent() to release resources , so daemonStreamFilter() need to check if stream filter existed when get client object lock and client->privData's lock, if not existed, just return -1
Posted by Lance Liu 4 years, 5 months ago
Previous patch can not pass build because of
missing virNetServerClientCheckFilterExist stated in libvirt_remote.syms

LanceLiu <liu.lance.89@gmail.com> 于2019年11月25日周一 下午3:34写道:

> ---
>  src/libvirt_remote.syms           |  1 +
>  src/remote/remote_daemon_stream.c | 10 +++++++++-
>  src/rpc/virnetserverclient.c      | 12 ++++++++++++
>  src/rpc/virnetserverclient.h      |  2 ++
>  4 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
> index 0493467..c32e234 100644
> --- a/src/libvirt_remote.syms
> +++ b/src/libvirt_remote.syms
> @@ -173,6 +173,7 @@ virNetServerClientPreExecRestart;
>  virNetServerClientRemoteAddrStringSASL;
>  virNetServerClientRemoteAddrStringURI;
>  virNetServerClientRemoveFilter;
> +virNetServerClientCheckFilterExist;
>  virNetServerClientSendMessage;
>  virNetServerClientSetAuthLocked;
>  virNetServerClientSetAuthPendingLocked;
> diff --git a/src/remote/remote_daemon_stream.c
> b/src/remote/remote_daemon_stream.c
> index 82cadb6..de0dca3 100644
> --- a/src/remote/remote_daemon_stream.c
> +++ b/src/remote/remote_daemon_stream.c
> @@ -292,10 +292,18 @@ daemonStreamFilter(virNetServerClientPtr client,
>  {
>      daemonClientStream *stream = opaque;
>      int ret = 0;
> +    daemonClientPrivatePtr priv = NULL;
> +    int filter_id = stream->filterID;
>
>      virObjectUnlock(client);
> +    priv = virNetServerClientGetPrivateData(client);
>      virMutexLock(&stream->priv->lock);
>      virObjectLock(client);
> +    if (!virNetServerClientCheckFilterExist(client, filter_id)) {
> +        VIR_WARN("this daemon stream filter: %d have been deleted!",
> filter_id);
> +        ret = -1;
> +        goto cleanup;
> +    }
>
>      if (msg->header.type != VIR_NET_STREAM &&
>          msg->header.type != VIR_NET_STREAM_HOLE)
> @@ -317,7 +325,7 @@ daemonStreamFilter(virNetServerClientPtr client,
>      ret = 1;
>
>   cleanup:
> -    virMutexUnlock(&stream->priv->lock);
> +    virMutexUnlock(&priv->lock);
>      return ret;
>  }
>
> diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
> index 67b3bf9..f80f493 100644
> --- a/src/rpc/virnetserverclient.c
> +++ b/src/rpc/virnetserverclient.c
> @@ -287,6 +287,18 @@ void
> virNetServerClientRemoveFilter(virNetServerClientPtr client,
>      virObjectUnlock(client);
>  }
>
> +int virNetServerClientCheckFilterExist(virNetServerClientPtr client,
> +                                       int filterID)
> +{
> +    virNetServerClientFilterPtr tmp;
> +
> +    tmp = client->filters;
> +    while(tmp && tmp->id != filterID) {
> +        tmp = tmp->next;
> +    }
> +
> +    return (tmp != NULL);
> +}
>
>  /* Check the client's access. */
>  static int
> diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
> index 7a3061d..85fda39 100644
> --- a/src/rpc/virnetserverclient.h
> +++ b/src/rpc/virnetserverclient.h
> @@ -93,6 +93,8 @@ int virNetServerClientAddFilter(virNetServerClientPtr
> client,
>
>  void virNetServerClientRemoveFilter(virNetServerClientPtr client,
>                                      int filterID);
> +int virNetServerClientCheckFilterExist(virNetServerClientPtr client,
> +                                       int filterID);
>
>  int virNetServerClientGetAuth(virNetServerClientPtr client);
>  void virNetServerClientSetAuthLocked(virNetServerClientPtr client, int
> auth);
> --
> 1.8.3.1
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix bug libvirt daemon segfault when new force console vm session break down existed console session. When force console vm command arrived, libvirtd will break down existed console session, and in this procedure, it will call daemonStreamEvent() to release resources , so daemonStreamFilter() need to check if stream filter existed when get client object lock and client->privData's lock, if not existed, just return -1
Posted by Michal Privoznik 4 years, 5 months ago
On 11/25/19 8:34 AM, LanceLiu wrote:
> ---
>   src/libvirt_remote.syms           |  1 +
>   src/remote/remote_daemon_stream.c | 10 +++++++++-
>   src/rpc/virnetserverclient.c      | 12 ++++++++++++
>   src/rpc/virnetserverclient.h      |  2 ++
>   4 files changed, 24 insertions(+), 1 deletion(-)

Please format commit messages following title + message format (look at 
git log how other messages are formatted).

> 
> diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
> index 0493467..c32e234 100644
> --- a/src/libvirt_remote.syms
> +++ b/src/libvirt_remote.syms
> @@ -173,6 +173,7 @@ virNetServerClientPreExecRestart;
>   virNetServerClientRemoteAddrStringSASL;
>   virNetServerClientRemoteAddrStringURI;
>   virNetServerClientRemoveFilter;
> +virNetServerClientCheckFilterExist;
>   virNetServerClientSendMessage;
>   virNetServerClientSetAuthLocked;
>   virNetServerClientSetAuthPendingLocked;
> diff --git a/src/remote/remote_daemon_stream.c b/src/remote/remote_daemon_stream.c
> index 82cadb6..de0dca3 100644
> --- a/src/remote/remote_daemon_stream.c
> +++ b/src/remote/remote_daemon_stream.c
> @@ -292,10 +292,18 @@ daemonStreamFilter(virNetServerClientPtr client,
>   {
>       daemonClientStream *stream = opaque;
>       int ret = 0;
> +    daemonClientPrivatePtr priv = NULL;
> +    int filter_id = stream->filterID;
>   
>       virObjectUnlock(client);
> +    priv = virNetServerClientGetPrivateData(client);

This is not needed.

>       virMutexLock(&stream->priv->lock);
>       virObjectLock(client);
> +    if (!virNetServerClientCheckFilterExist(client, filter_id)) {
> +        VIR_WARN("this daemon stream filter: %d have been deleted!", filter_id);
> +        ret = -1;
> +        goto cleanup;
> +    }
>   
>       if (msg->header.type != VIR_NET_STREAM &&
>           msg->header.type != VIR_NET_STREAM_HOLE)
> @@ -317,7 +325,7 @@ daemonStreamFilter(virNetServerClientPtr client,
>       ret = 1;
>   
>    cleanup:
> -    virMutexUnlock(&stream->priv->lock);
> +    virMutexUnlock(&priv->lock);

This is not needed: stream->priv and priv are the same structure.

>       return ret;
>   }
>   

Anyway, this still doesn't work. Problem is, that if a stream is 
removed, the private data might be removed too and hence 
virMutexLock(&stream->priv->lock) will do something undefined (besides 
accessing freed memory). In my testing the daemon deadlocks because it's 
trying to lock stream-priv->lock which is locked.

As I said in the other thread - we need to re-evaluate the first commit. 
Do you have a reproducer for the original problem please?

Michal

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

Re: [libvirt] [PATCH] Fix bug libvirt daemon segfault when new force console vm session break down existed console session. When force console vm command arrived, libvirtd will break down existed console session, and in this procedure, it will call daemonStreamEvent() to release resources , so daemonStreamFilter() need to check if stream filter existed when get client object lock and client->privData's lock, if not existed, just return -1
Posted by Lance Liu 4 years, 5 months ago
We first produce this bug in rhel7.4's libvir daemon。For easily produce the
bug, the step can be as follows:
1. add sleep(3)  in daemonStreamFilter() pre
virMutexLock(&stream->priv->lock), then build libvirtd bin executable, then
restart libvirtd
2. use virsh console open one vm's console, for this console(the vm's
kernel need console=ttyS0 boot parameter,then just  input from keyboard on
and on
3. use virsh console --force to break the previous console session,  than
you will get libvirt daemon deadlock。

And for the problem client->privData to be released problem, only
virNetServerClientClose() will free client->privData and client, I think
this can be fixed by remove virNetServerClientClose()
from daemonStreamEvent(),
and replace with virNetServerClientImmediateClose(),
so virNetServerProcessClients() will test the session would be closed。



Michal Privoznik <mprivozn@redhat.com> 于2019年11月25日周一 下午5:38写道:

> On 11/25/19 8:34 AM, LanceLiu wrote:
> > ---
> >   src/libvirt_remote.syms           |  1 +
> >   src/remote/remote_daemon_stream.c | 10 +++++++++-
> >   src/rpc/virnetserverclient.c      | 12 ++++++++++++
> >   src/rpc/virnetserverclient.h      |  2 ++
> >   4 files changed, 24 insertions(+), 1 deletion(-)
>
> Please format commit messages following title + message format (look at
> git log how other messages are formatted).
>
> >
> > diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
> > index 0493467..c32e234 100644
> > --- a/src/libvirt_remote.syms
> > +++ b/src/libvirt_remote.syms
> > @@ -173,6 +173,7 @@ virNetServerClientPreExecRestart;
> >   virNetServerClientRemoteAddrStringSASL;
> >   virNetServerClientRemoteAddrStringURI;
> >   virNetServerClientRemoveFilter;
> > +virNetServerClientCheckFilterExist;
> >   virNetServerClientSendMessage;
> >   virNetServerClientSetAuthLocked;
> >   virNetServerClientSetAuthPendingLocked;
> > diff --git a/src/remote/remote_daemon_stream.c
> b/src/remote/remote_daemon_stream.c
> > index 82cadb6..de0dca3 100644
> > --- a/src/remote/remote_daemon_stream.c
> > +++ b/src/remote/remote_daemon_stream.c
> > @@ -292,10 +292,18 @@ daemonStreamFilter(virNetServerClientPtr client,
> >   {
> >       daemonClientStream *stream = opaque;
> >       int ret = 0;
> > +    daemonClientPrivatePtr priv = NULL;
> > +    int filter_id = stream->filterID;
> >
> >       virObjectUnlock(client);
> > +    priv = virNetServerClientGetPrivateData(client);
>
> This is not needed.
>
> >       virMutexLock(&stream->priv->lock);
> >       virObjectLock(client);
> > +    if (!virNetServerClientCheckFilterExist(client, filter_id)) {
> > +        VIR_WARN("this daemon stream filter: %d have been deleted!",
> filter_id);
> > +        ret = -1;
> > +        goto cleanup;
> > +    }
> >
> >       if (msg->header.type != VIR_NET_STREAM &&
> >           msg->header.type != VIR_NET_STREAM_HOLE)
> > @@ -317,7 +325,7 @@ daemonStreamFilter(virNetServerClientPtr client,
> >       ret = 1;
> >
> >    cleanup:
> > -    virMutexUnlock(&stream->priv->lock);
> > +    virMutexUnlock(&priv->lock);
>
> This is not needed: stream->priv and priv are the same structure.
>
> >       return ret;
> >   }
> >
>
> Anyway, this still doesn't work. Problem is, that if a stream is
> removed, the private data might be removed too and hence
> virMutexLock(&stream->priv->lock) will do something undefined (besides
> accessing freed memory). In my testing the daemon deadlocks because it's
> trying to lock stream-priv->lock which is locked.
>
> As I said in the other thread - we need to re-evaluate the first commit.
> Do you have a reproducer for the original problem please?
>
> Michal
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix bug libvirt daemon segfault when new force console vm session break down existed console session. When force console vm command arrived, libvirtd will break down existed console session, and in this procedure, it will call daemonStreamEvent() to release resources , so daemonStreamFilter() need to check if stream filter existed when get client object lock and client->privData's lock, if not existed, just return -1
Posted by Michal Privoznik 4 years, 5 months ago
On 11/25/19 11:53 AM, Lance Liu wrote:
> We first produce this bug in rhel7.4's libvir daemon。For easily 
> produce the bug, the step can be as follows: 1. add sleep(3)  in 
> daemonStreamFilter() pre virMutexLock(&stream->priv->lock), then 
> build libvirtd bin executable, then restart libvirtd 2. use virsh 
> console open one vm's console, for this console(the vm's kernel need 
> console=ttyS0 boot parameter,then just  input from keyboard on and on
> 3. use virsh console --force to break the previous console session,
> than you will get libvirt daemon deadlock。
> 
> And for the problem client->privData to be released problem, only 
> virNetServerClientClose() will free client->privData and client, I 
> think this can be fixed by remove virNetServerClientClose() from 
> daemonStreamEvent(), and replace with 
> virNetServerClientImmediateClose(), so virNetServerProcessClients() 
> will test the session would be closed。

RHEL-7.4 you say? That's libvirt-3.2.0 which is far from what we have on
the master. Also, RHEL-7.4 itself is kind of old as RHEL-7.7 was
released during summer. At least that explains why your original patch
was against older libvirt.

When I revert your patch I can see the deadlock still happening:

Thread 5 (Thread 0x7feac8936700 (LWP 162328)):
#0  0x00007feacd1d6f6c in __lll_lock_wait () at /lib64/libpthread.so.0
#1  0x00007feacd1cf194 in pthread_mutex_lock () at /lib64/libpthread.so.0
#2  0x00007feacdc78a9e in virMutexLock (m=0x565030303700) at 
../../src/util/virthread.c:79
#3  0x00007feacdc488a2 in virObjectLock (anyobj=0x5650303036f0) at 
../../src/util/virobject.c:433
#4  0x00007feacdd9f8bc in virNetServerClientSendMessage 
(client=0x5650303036f0, msg=0x7feab40281e0) at 
../../src/rpc/virnetserverclient.c:1532
#5  0x00007feacdd9ac27 in virNetServerProgramSendError 
(program=536903814, version=1, client=0x5650303036f0, 
msg=0x7feab40281e0, rerr=0x7feac8935730, procedure=201, type=3, 
serial=16) at ../../src/rpc/virnetserverprogram.c:168
#6  0x00007feacdd9ad52 in virNetServerProgramSendStreamError 
(prog=0x5650302ccb70, client=0x5650303036f0, msg=0x7feab40281e0, 
rerr=0x7feac8935730, procedure=201, serial=16) at 
../../src/rpc/virnetserverprogram.c:217
#7  0x000056502e9bbd24 in daemonStreamEvent (st=0x7feac40022a0, 
events=4, opaque=0x5650303036f0) at 
../../src/remote/remote_daemon_stream.c:256
#8  0x00007feacdc01648 in virFDStreamCloseInt (st=0x7feac40022a0, 
streamAbort=true) at ../../src/util/virfdstream.c:715
#9  0x00007feacdc017d1 in virFDStreamAbort (st=0x7feac40022a0) at 
../../src/util/virfdstream.c:759
#10 0x00007feacded9e1d in virStreamAbort (stream=0x7feac40022a0) at 
../../src/libvirt-stream.c:1237
#11 0x00007feacdd456e3 in virChrdevOpen (devs=0x7fea740cae00, 
source=0x7fea740d26a0, st=0x7feab402f220, force=true) at 
../../src/conf/virchrdev.c:387
#12 0x00007feab3f722e7 in qemuDomainOpenConsole (dom=0x7feab40329b0, 
dev_name=0x0, st=0x7feab402f220, flags=1) at 
../../src/qemu/qemu_driver.c:17309
#13 0x00007feacdeb375f in virDomainOpenConsole (dom=0x7feab40329b0, 
dev_name=0x0, st=0x7feab402f220, flags=1) at ../../src/libvirt-domain.c:9662
#14 0x000056502e999894 in remoteDispatchDomainOpenConsole 
(server=0x56503027fc30, client=0x56503031ad70, msg=0x565030304eb0, 
rerr=0x7feac8935ad0, args=0x7feab402cd70) at 
./remote/remote_daemon_dispatch_stubs.h:9211
#15 0x000056502e99976f in remoteDispatchDomainOpenConsoleHelper 
(server=0x56503027fc30, client=0x56503031ad70, msg=0x565030304eb0, 
rerr=0x7feac8935ad0, args=0x7feab402cd70, ret=0x0) at 
./remote/remote_daemon_dispatch_stubs.h:9178
#16 0x00007feacdd9b4b1 in virNetServerProgramDispatchCall 
(prog=0x5650302ccb70, server=0x56503027fc30, client=0x56503031ad70, 
msg=0x565030304eb0) at ../../src/rpc/virnetserverprogram.c:430
#17 0x00007feacdd9b026 in virNetServerProgramDispatch 
(prog=0x5650302ccb70, server=0x56503027fc30, client=0x56503031ad70, 
msg=0x565030304eb0) at ../../src/rpc/virnetserverprogram.c:302
#18 0x00007feacdda1e34 in virNetServerProcessMsg (srv=0x56503027fc30, 
client=0x56503031ad70, prog=0x5650302ccb70, msg=0x565030304eb0) at 
../../src/rpc/virnetserver.c:136
#19 0x00007feacdda1ef4 in virNetServerHandleJob 
(jobOpaque=0x565030336ea0, opaque=0x56503027fc30) at 
../../src/rpc/virnetserver.c:153
#20 0x00007feacdc79800 in virThreadPoolWorker (opaque=0x56503028a1f0) at 
../../src/util/virthreadpool.c:163
#21 0x00007feacdc78dad in virThreadHelper (data=0x56503028afd0) at 
../../src/util/virthread.c:196
#22 0x00007feacd1cc458 in start_thread () at /lib64/libpthread.so.0
#23 0x00007feacd0fa6ef in clone () at /lib64/libc.so.6


Thread 1 (Thread 0x7feaca8a7dc0 (LWP 162311)):
#0  0x00007feacd1d6f6c in __lll_lock_wait () at /lib64/libpthread.so.0
#1  0x00007feacd1cf194 in pthread_mutex_lock () at /lib64/libpthread.so.0
#2  0x00007feacdc78a9e in virMutexLock (m=0x5650302ff110) at 
../../src/util/virthread.c:79
#3  0x000056502e9bbde3 in daemonStreamFilter (client=0x5650303036f0, 
msg=0x5650303097d0, opaque=0x7feac4003020) at 
../../src/remote/remote_daemon_stream.c:297
#4  0x00007feacdd9f03b in virNetServerClientDispatchRead 
(client=0x5650303036f0) at ../../src/rpc/virnetserverclient.c:1288
#5  0x00007feacdd9f6ab in virNetServerClientDispatchEvent 
(sock=0x565030309a90, events=1, opaque=0x5650303036f0) at 
../../src/rpc/virnetserverclient.c:1485
#6  0x00007feacdd907f9 in virNetSocketEventHandle (watch=19, fd=28, 
events=1, opaque=0x565030309a90) at ../../src/rpc/virnetsocket.c:2177
#7  0x00007feacdbff32d in virEventPollDispatchHandles (nfds=16, 
fds=0x5650302bf200) at ../../src/util/vireventpoll.c:503
#8  0x00007feacdbffb86 in virEventPollRunOnce () at 
../../src/util/vireventpoll.c:658
#9  0x00007feacdbfdafb in virEventRunDefaultImpl () at 
../../src/util/virevent.c:322
#10 0x00007feacdda1a0d in virNetDaemonRun (dmn=0x56503027f880) at 
../../src/rpc/virnetdaemon.c:836
#11 0x000056502e98843f in main (argc=2, argv=0x7ffc308703a8) at 
../../src/remote/remote_daemon.c:1427


But I think I have an idea how to fix this. Will post patches later today.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix bug libvirt daemon segfault when new force console vm session break down existed console session. When force console vm command arrived, libvirtd will break down existed console session, and in this procedure, it will call daemonStreamEvent() to release resources , so daemonStreamFilter() need to check if stream filter existed when get client object lock and client->privData's lock, if not existed, just return -1
Posted by Lance Liu 4 years, 4 months ago
Ok, I'll check it out.

Look like your patch is still not in master branch。May you commit it? I'll
check out it and test for force console case

Michal Privoznik <mprivozn@redhat.com> 于2019年11月25日周一 下午10:12写道:

> On 11/25/19 11:53 AM, Lance Liu wrote:
> > We first produce this bug in rhel7.4's libvir daemon。For easily
> > produce the bug, the step can be as follows: 1. add sleep(3)  in
> > daemonStreamFilter() pre virMutexLock(&stream->priv->lock), then
> > build libvirtd bin executable, then restart libvirtd 2. use virsh
> > console open one vm's console, for this console(the vm's kernel need
> > console=ttyS0 boot parameter,then just  input from keyboard on and on
> > 3. use virsh console --force to break the previous console session,
> > than you will get libvirt daemon deadlock。
> >
> > And for the problem client->privData to be released problem, only
> > virNetServerClientClose() will free client->privData and client, I
> > think this can be fixed by remove virNetServerClientClose() from
> > daemonStreamEvent(), and replace with
> > virNetServerClientImmediateClose(), so virNetServerProcessClients()
> > will test the session would be closed。
>
> RHEL-7.4 you say? That's libvirt-3.2.0 which is far from what we have on
> the master. Also, RHEL-7.4 itself is kind of old as RHEL-7.7 was
> released during summer. At least that explains why your original patch
> was against older libvirt.
>
> When I revert your patch I can see the deadlock still happening:
>
> Thread 5 (Thread 0x7feac8936700 (LWP 162328)):
> #0  0x00007feacd1d6f6c in __lll_lock_wait () at /lib64/libpthread.so.0
> #1  0x00007feacd1cf194 in pthread_mutex_lock () at /lib64/libpthread.so.0
> #2  0x00007feacdc78a9e in virMutexLock (m=0x565030303700) at
> ../../src/util/virthread.c:79
> #3  0x00007feacdc488a2 in virObjectLock (anyobj=0x5650303036f0) at
> ../../src/util/virobject.c:433
> #4  0x00007feacdd9f8bc in virNetServerClientSendMessage
> (client=0x5650303036f0, msg=0x7feab40281e0) at
> ../../src/rpc/virnetserverclient.c:1532
> #5  0x00007feacdd9ac27 in virNetServerProgramSendError
> (program=536903814, version=1, client=0x5650303036f0,
> msg=0x7feab40281e0, rerr=0x7feac8935730, procedure=201, type=3,
> serial=16) at ../../src/rpc/virnetserverprogram.c:168
> #6  0x00007feacdd9ad52 in virNetServerProgramSendStreamError
> (prog=0x5650302ccb70, client=0x5650303036f0, msg=0x7feab40281e0,
> rerr=0x7feac8935730, procedure=201, serial=16) at
> ../../src/rpc/virnetserverprogram.c:217
> #7  0x000056502e9bbd24 in daemonStreamEvent (st=0x7feac40022a0,
> events=4, opaque=0x5650303036f0) at
> ../../src/remote/remote_daemon_stream.c:256
> #8  0x00007feacdc01648 in virFDStreamCloseInt (st=0x7feac40022a0,
> streamAbort=true) at ../../src/util/virfdstream.c:715
> #9  0x00007feacdc017d1 in virFDStreamAbort (st=0x7feac40022a0) at
> ../../src/util/virfdstream.c:759
> #10 0x00007feacded9e1d in virStreamAbort (stream=0x7feac40022a0) at
> ../../src/libvirt-stream.c:1237
> #11 0x00007feacdd456e3 in virChrdevOpen (devs=0x7fea740cae00,
> source=0x7fea740d26a0, st=0x7feab402f220, force=true) at
> ../../src/conf/virchrdev.c:387
> #12 0x00007feab3f722e7 in qemuDomainOpenConsole (dom=0x7feab40329b0,
> dev_name=0x0, st=0x7feab402f220, flags=1) at
> ../../src/qemu/qemu_driver.c:17309
> #13 0x00007feacdeb375f in virDomainOpenConsole (dom=0x7feab40329b0,
> dev_name=0x0, st=0x7feab402f220, flags=1) at
> ../../src/libvirt-domain.c:9662
> #14 0x000056502e999894 in remoteDispatchDomainOpenConsole
> (server=0x56503027fc30, client=0x56503031ad70, msg=0x565030304eb0,
> rerr=0x7feac8935ad0, args=0x7feab402cd70) at
> ./remote/remote_daemon_dispatch_stubs.h:9211
> #15 0x000056502e99976f in remoteDispatchDomainOpenConsoleHelper
> (server=0x56503027fc30, client=0x56503031ad70, msg=0x565030304eb0,
> rerr=0x7feac8935ad0, args=0x7feab402cd70, ret=0x0) at
> ./remote/remote_daemon_dispatch_stubs.h:9178
> #16 0x00007feacdd9b4b1 in virNetServerProgramDispatchCall
> (prog=0x5650302ccb70, server=0x56503027fc30, client=0x56503031ad70,
> msg=0x565030304eb0) at ../../src/rpc/virnetserverprogram.c:430
> #17 0x00007feacdd9b026 in virNetServerProgramDispatch
> (prog=0x5650302ccb70, server=0x56503027fc30, client=0x56503031ad70,
> msg=0x565030304eb0) at ../../src/rpc/virnetserverprogram.c:302
> #18 0x00007feacdda1e34 in virNetServerProcessMsg (srv=0x56503027fc30,
> client=0x56503031ad70, prog=0x5650302ccb70, msg=0x565030304eb0) at
> ../../src/rpc/virnetserver.c:136
> #19 0x00007feacdda1ef4 in virNetServerHandleJob
> (jobOpaque=0x565030336ea0, opaque=0x56503027fc30) at
> ../../src/rpc/virnetserver.c:153
> #20 0x00007feacdc79800 in virThreadPoolWorker (opaque=0x56503028a1f0) at
> ../../src/util/virthreadpool.c:163
> #21 0x00007feacdc78dad in virThreadHelper (data=0x56503028afd0) at
> ../../src/util/virthread.c:196
> #22 0x00007feacd1cc458 in start_thread () at /lib64/libpthread.so.0
> #23 0x00007feacd0fa6ef in clone () at /lib64/libc.so.6
>
>
> Thread 1 (Thread 0x7feaca8a7dc0 (LWP 162311)):
> #0  0x00007feacd1d6f6c in __lll_lock_wait () at /lib64/libpthread.so.0
> #1  0x00007feacd1cf194 in pthread_mutex_lock () at /lib64/libpthread.so.0
> #2  0x00007feacdc78a9e in virMutexLock (m=0x5650302ff110) at
> ../../src/util/virthread.c:79
> #3  0x000056502e9bbde3 in daemonStreamFilter (client=0x5650303036f0,
> msg=0x5650303097d0, opaque=0x7feac4003020) at
> ../../src/remote/remote_daemon_stream.c:297
> #4  0x00007feacdd9f03b in virNetServerClientDispatchRead
> (client=0x5650303036f0) at ../../src/rpc/virnetserverclient.c:1288
> #5  0x00007feacdd9f6ab in virNetServerClientDispatchEvent
> (sock=0x565030309a90, events=1, opaque=0x5650303036f0) at
> ../../src/rpc/virnetserverclient.c:1485
> #6  0x00007feacdd907f9 in virNetSocketEventHandle (watch=19, fd=28,
> events=1, opaque=0x565030309a90) at ../../src/rpc/virnetsocket.c:2177
> #7  0x00007feacdbff32d in virEventPollDispatchHandles (nfds=16,
> fds=0x5650302bf200) at ../../src/util/vireventpoll.c:503
> #8  0x00007feacdbffb86 in virEventPollRunOnce () at
> ../../src/util/vireventpoll.c:658
> #9  0x00007feacdbfdafb in virEventRunDefaultImpl () at
> ../../src/util/virevent.c:322
> #10 0x00007feacdda1a0d in virNetDaemonRun (dmn=0x56503027f880) at
> ../../src/rpc/virnetdaemon.c:836
> #11 0x000056502e98843f in main (argc=2, argv=0x7ffc308703a8) at
> ../../src/remote/remote_daemon.c:1427
>
>
> But I think I have an idea how to fix this. Will post patches later today.
>
> Michal
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix bug libvirt daemon segfault when new force console vm session break down existed console session. When force console vm command arrived, libvirtd will break down existed console session, and in this procedure, it will call daemonStreamEvent() to release resources , so daemonStreamFilter() need to check if stream filter existed when get client object lock and client->privData's lock, if not existed, just return -1
Posted by Michal Privoznik 4 years, 4 months ago
On 11/28/19 4:03 AM, Lance Liu wrote:
> Ok, I'll check it out.
> 
> Look like your patch is still not in master branch。May you commit it? 
> I'll check out it and test for force console case

Unfortunately, it did not receive any ACK from a contributor with commit 
rights (which is required for any patch that we want to merge). I will 
try to get somebody to review it.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix bug libvirt daemon segfault when new force console vm session break down existed console session. When force console vm command arrived, libvirtd will break down existed console session, and in this procedure, it will call daemonStreamEvent() to release resources , so daemonStreamFilter() need to check if stream filter existed when get client object lock and client->privData's lock, if not existed, just return -1
Posted by Lance Liu 4 years, 4 months ago
Ok, thanks.

Michal Privoznik <mprivozn@redhat.com> 于2019年11月28日周四 下午5:21写道:

> On 11/28/19 4:03 AM, Lance Liu wrote:
> > Ok, I'll check it out.
> >
> > Look like your patch is still not in master branch。May you commit it?
> > I'll check out it and test for force console case
>
> Unfortunately, it did not receive any ACK from a contributor with commit
> rights (which is required for any patch that we want to merge). I will
> try to get somebody to review it.
>
> Michal
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list