Re: [libvirt] [PATCH] add lock priv->lock protection in remoteClientCloseFunc()
Hi:
Need any more informations about my patch?
LanceLiu 于2019年12月2日周一 上午11:16写道：
> sometimes, virsh console session may closed by virsh console existed
> session(when virsh console client
> exit) and new virsh console --force session simutaneously. So in one
> thread(Job worker), it calls
> daemonStreamEvent() and referencing stream, and in another thread(main
> thread), virNetServerClientClose()
> was called, and the callback remoteClientCloseFunc() was called, without
> protect by priv->lock in
> remoteClientCloseFunc(), it may lead to daemonStreamEvent reference stream
> released by remoteClientCloseFunc(),
> and also need change virNetServerClientClose() to be
> virNetServerClientImmediateClose() in daemonStreamEvent(),
> or it lead to libvirt daemon deadlock by double lock priv->lock in
> daemonStreamEvent()
> ---
> src/remote/remote_daemon_dispatch.c | 2 ++
> src/remote/remote_daemon_stream.c | 14 +++---
> 2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/src/remote/remote_daemon_dispatch.c
> b/src/remote/remote_daemon_dispatch.c
> index f369ffb..b0982b7 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -1724,7 +1724,9 @@ static void
> remoteClientCloseFunc(virNetServerClientPtr client)
> {
> struct daemonClientPrivate *priv =
> virNetServerClientGetPrivateData(client);
>
> +virMutexLock(>lock);
> daemonRemoveAllClientStreams(priv->streams);
> +virMutexUnlock(>lock);
>
> remoteClientFreePrivateCallbacks(priv);
> }
> diff --git a/src/remote/remote_daemon_stream.c
> b/src/remote/remote_daemon_stream.c
> index 73e4d7b..6a84fdf 100644
> --- a/src/remote/remote_daemon_stream.c
> +++ b/src/remote/remote_daemon_stream.c
> @@ -141,7 +141,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
> (events & VIR_STREAM_EVENT_WRITABLE)) {
> if (daemonStreamHandleWrite(client, stream) < 0) {
> daemonRemoveClientStream(client, stream);
> -virNetServerClientClose(client);
> +virNetServerClientImmediateClose(client);
> goto cleanup;
> }
> }
> @@ -151,7 +151,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
> events = events & ~(VIR_STREAM_EVENT_READABLE);
> if (daemonStreamHandleRead(client, stream) < 0) {
> daemonRemoveClientStream(client, stream);
> -virNetServerClientClose(client);
> +virNetServerClientImmediateClose(client);
> goto cleanup;
> }
> /* If we detected EOF during read processing,
> @@ -176,7 +176,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
> if (daemonStreamHandleFinish(client, stream, msg) < 0) {
> virNetMessageFree(msg);
> daemonRemoveClientStream(client, stream);
> -virNetServerClientClose(client);
> +virNetServerClientImmediateClose(client);
> goto cleanup;
> }
> break;
> @@ -186,7 +186,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
> if (daemonStreamHandleAbort(client, stream, msg) < 0) {
> virNetMessageFree(msg);
> daemonRemoveClientStream(client, stream);
> -virNetServerClientClose(client);
> +virNetServerClientImmediateClose(client);
> goto cleanup;
> }
> break;
> @@ -205,7 +205,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
> stream->recvEOF = true;
> if (!(msg = virNetMessageNew(false))) {
> daemonRemoveClientStream(client, stream);
> -virNetServerClientClose(client);
> +virNetServerClientImmediateClose(client);
> goto cleanup;
> }
> msg->cb = daemonStreamMessageFinished;
> @@ -219,7 +219,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
>"", 0) < 0) {
> virNetMessageFree(msg);
> daemonRemoveClientStream(client, stream);
> -virNetServerClientClose(client);
> +virNetServerClientImmediateClose(client);
> goto cleanup;
> }
> }
> @@ -262,7 +262,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
> }
> daemonRemoveClientStream(client, stream);
> if (ret < 0)
> -virNetServerClientClose(client);
> + virNetServerClientImmediateClose(client);
> goto cleanup;
> }
>
> --
> 1.8.3.1
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] remote_daemon_stream: Hold an extra reference to stream in daemonStreamFilter
Michal Privoznik
下午5:58 (1小时前)
发送至 我、 libvir-list
On 11/28/19 4:09 AM, Lance Liu wrote:
> Hi, Michal:
> I think the daemonFreeClientStream(NULL, stream); in your patch
> should line above virMutexUnlock(>priv->lock)， or else there
> is issue WAW.
I'm sorry, I don't know what WAW is.
write after write, daemonFreeClientStream() outside stream->priv->lock
may lead to two threads call daemonFreeClientStream() simultaneously
> That is two threads may sub stream->
>
> Lance Liu mailto:liu.lance...@gmail.com>> 于
> 2019年11月26日周二 下午1:54写道：
>
> Yeah, your patch is perfectly fine for stream freed problem.
> But I have reviewed code in daemonStreamEvent() and
> daemonStreamFilter() again, and I think there still two issue need
> to be reconsidered:
> 1. stream->ref only ++ in daemonStreamFilter, except for error
> occurred call daemonRemoveClientStream() lead to ref--, like code as
> follow. Though code won't be executed because of
> no VIR_STREAM_EVENT_HANGUP event taken place,
> but it is not good code:
Do you perhaps mean daemonStreamEvent() instead of daemonStreamFilter()?
Because in daemonStreamFilter() we do both ref & unref - the unref is
hidden in daemonFreeClientStream().
Again, the unref is hidden in daemonStreamMessageFinished() which calls
daemonFreeClientStream(). The msg->cb is guaranteed to be called
eventually upon successful return from
virNetServerProgramSendStreamData(). That's why you don't see the direct
unref in the success path.
Yeah, got it. I missed that!
> }
> 2. call virNetServerClientClose() is still inappropriate in because
> the it may free the client resource which other threads need ref,
> may be replace it with virNetServerClientImmediateClose() is better,
> the daemon can process client resource release unified
I've seen the patch you send, but it was without any commit message so
no one had any idea why the change is needed.
I understand your argument here, but I'm having hard time finding
practical example in the code. I mean, have you seen such issue in real?
> 3. Segmentfault may still exists when another thread
> call remoteClientCloseFunc in virNetServerClientClose()(for example,
> when new force console session break down existed console session,
> and existed console session
> 's client close the session simutaneously, but remoteClientCloseFunc
> not lock(priv->lock), so the resource maybe released when
> daemonStreamEvent ref)
Again, sounds plausible, but this is a subset of the previous problem
since remoteClientCloseFunc() is called from
virNetServerClientCloseLocked() which is called from
virNetServerClientClose().
I'll show you how to get the bug。Use gdb attach the libvirt daemon
and break in the middle of daemonStreamEvent()(with event == 4,
called by new force console break down existed console session),
then exit from the previous virsh console client, so libvirt daemon can
percent console client connection fd closed, then call the
virNetServerClientClose()
in main thread, because the callback remote remoteClientCloseFunc() downes
not
lock priv->lock, so it will release stream directly, regardless anther
thread reference it
in daemonStreamEvent()
Michal Privoznik 于2019年11月28日周四 下午5:58写道：
> On 11/28/19 4:09 AM, Lance Liu wrote:
> > Hi, Michal:
> > I think the daemonFreeClientStream(NULL, stream); in your patch
> > should line above virMutexUnlock(>priv->lock)， or else there
> > is issue WAW.
>
> I'm sorry, I don't know what WAW is.
>
> > That is two threads may sub stream->
> >
> > Lance Liu mailto:liu.lance...@gmail.com>> 于
> > 2019年11月26日周二 下午1:54写道：
> >
> > Yeah, your patch is perfectly fine for stream freed problem.
> > But I have reviewed code in daemonStreamEvent() and
> > daemonStreamFilter() again, and I think there still two issue need
> > to be reconsidered:
> > 1. stream->ref only ++ in daemonStreamFilter, except for error
> > occurred call daemonRemoveClientStream() lead to ref--, like code as
> > follow. Though code won't be executed because of
> > no VIR_STREAM_EVENT_HANGUP event taken place,
> > but it is not good code:
>
> Do you perhaps mean daemonStreamEvent() instead of daemonStreamFilter()?
> Because in daemonStreamFilter() we do both ref & unref - the unref is
> hidden in daemonFreeClientStream().
>
> > /* If we got HANGUP, we need to only send an empty
> > * packet so the client sees an EOF and cleans up
> > */
> > if (!stream->closed && !stream->recvEOF &&
> > (events & VIR_STREAM_EVENT_HANGUP)) {
>
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 cons
Ok, thanks. Michal Privoznik 于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
Re: [libvirt] [PATCH] remote_daemon_stream: Hold an extra reference to stream in daemonStreamFilter
Hi, Michal:
I think the daemonFreeClientStream(NULL, stream); in your patch
should line above virMutexUnlock(>priv->lock)， or else there is
issue WAW. That is two threads may sub stream->
Lance Liu 于2019年11月26日周二 下午1:54写道：
> Yeah, your patch is perfectly fine for stream freed problem.
> But I have reviewed code in daemonStreamEvent() and daemonStreamFilter()
> again, and I think there still two issue need to be reconsidered:
> 1. stream->ref only ++ in daemonStreamFilter, except for error
> occurred call daemonRemoveClientStream() lead to ref--, like code as
> follow. Though code won't be executed because of no VIR_STREAM_EVENT_HANGUP
> event taken place,
> but it is not good code:
> /* If we got HANGUP, we need to only send an empty
> * packet so the client sees an EOF and cleans up
> */
> if (!stream->closed && !stream->recvEOF &&
> (events & VIR_STREAM_EVENT_HANGUP)) {
> virNetMessagePtr msg;
> events &= ~(VIR_STREAM_EVENT_HANGUP);
> stream->tx = false;
> stream->recvEOF = true;
> if (!(msg = virNetMessageNew(false))) {
> daemonRemoveClientStream(client, stream);
> virNetServerClientClose(client);
> goto cleanup;
> }
> msg->cb = daemonStreamMessageFinished;
> msg->opaque = stream;
> stream->refs++;
> if (virNetServerProgramSendStreamData(stream->prog,
> client,
> msg,
> stream->procedure,
> stream->serial,
> "", 0) < 0) {
> virNetMessageFree(msg);
> daemonRemoveClientStream(client, stream);
> virNetServerClientClose(client);
> goto cleanup;
> }
> }
> 2. call virNetServerClientClose() is still inappropriate in because the
> it may free the client resource which other threads need ref, may be
> replace it with virNetServerClientImmediateClose() is better,
> the daemon can process client resource release unified
> 3. Segmentfault may still exists when another thread
> call remoteClientCloseFunc in virNetServerClientClose()(for example, when
> new force console session break down existed console session, and existed
> console session
> 's client close the session simutaneously, but remoteClientCloseFunc not
> lock(priv->lock), so the resource maybe released when daemonStreamEvent ref)
>
> How do you see it?
>
>
> Best Regards!
>
>
>
>
> Michal Privoznik 于2019年11月26日周二 上午12:49写道：
>
>> In v5.9.0-273-g8ecab214de I've tried to fix a lock ordering
>> problem, but introduced a crasher. Problem is that because the
>> client lock is unlocked (in order to honour lock ordering) the
>> stream we are currently checking in daemonStreamFilter() might be
>> freed and thus stream->priv might not even exist when the control
>> get to virMutexLock() call.
>>
>> To resolve this, grab an extra reference to the stream and handle
>> its cleanup should the refcounter reach zero after the deref.
>> If that's the case and we are the only ones holding a reference
>> to the stream, we MUST return a positive value to make
>> virNetServerClientDispatchRead() break its loop where it iterates
>> over filters. The problem is, if we did not do so, then
>> "filter = filter->next" line will read from a memory that was
>> just freed (freeing a stream also unregisters its filter).
>>
>> Signed-off-by: Michal Privoznik
>> ---
>>
>> Reproducing this issue is very easy:
>>
>> 1) put sleep(5) right after virObjectUnlock(client) in the fist hunk,
>> 2) virsh console --force $dom and type something so that the stream
>>has some data to process
>> 3) while 2) is still running, run the same command from another terminal
>> 4) observe libvirtd crash
>>
>> src/remote/remote_daemon_stream.c | 19 +++
>> 1 file changed, 19 insertions(+)
>>
>> diff --git a/src/remote/remote_daemon_stream.c
>> b/src/remote/remote_daemon_stream.c
>> index 82cadb67ac..73e4d7befb 100644
>> --- a/src/remote/remote_daemon_stream.c
>> +++ b/src/remote/remote_daemon_stream.c
>> @@ -293,10 +293,25 @@ daemonStreamFilter(virNetServerClientPtr client,
>> daemonClientStream *stream = opaque;
>> int ret = 0;
>>
>> +/* We must honour lock ordering here. Client private data lock must
>> + * be acquired
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 cons
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 于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(>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 0x7feacd1d6f6c in __lll_lock_wait () at /lib64/libpthread.so.0 > #1 0x7feacd1cf194 in pthread_mutex_lock () at /lib64/libpthread.so.0 > #2 0x7feacdc78a9e in virMutexLock (m=0x565030303700) at > ../../src/util/virthread.c:79 > #3 0x7feacdc488a2 in virObjectLock (anyobj=0x5650303036f0) at > ../../src/util/virobject.c:433 > #4 0x7feacdd9f8bc in virNetServerClientSendMessage > (client=0x5650303036f0, msg=0x7feab40281e0) at > ../../src/rpc/virnetserverclient.c:1532 > #5 0x7feacdd9ac27 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 0x7feacdd9ad52 in virNetServerProgramSendStreamError > (prog=0x5650302ccb70, client=0x5650303036f0, msg=0x7feab40281e0, > rerr=0x7feac8935730, procedure=201, serial=16) at > ../../src/rpc/virnetserverprogram.c:217 > #7 0x56502e9bbd24 in daemonStreamEvent (st=0x7feac40022a0, > events=4, opaque=0x5650303036f0) at > ../../src/remote/remote_daemon_stream.c:256 > #8 0x7feacdc01648 in virFDStreamCloseInt (st=0x7feac40022a0, > streamAbort=true) at ../../src/util/virfdstream.c:715 > #9 0x7feacdc017d1 in virFDStreamAbort (st=0x7feac40022a0) at > ../../src/util/virfdstream.c:759 > #10 0x7feacded9e1d in virStreamAbort (stream=0x7feac40022a0) at > ../../src/libvirt-stream.c:1237 > #11 0x7feacdd456e3 in virChrdevOpen (devs=0x7fea740cae00, > source=0x7fea740d26a0, st=0x7feab402f220, force=true) at > ../../src/conf/virchrdev.c:387 > #12 0x7feab3f722e7 in qemuDomainOpenConsole (dom=0x7feab40329b0, > dev_name=0x0, st=0x7feab402f220, flags=1) at > ../../src/qemu/qemu_driver.c:17309 > #13 0x7feacdeb375f in virDomainOpenConsole (dom=0x7feab40329b0, > dev_name=0x0, st=0x7feab402f220, flags=1) at > ../../src/libvirt-domain.c:9662 > #14 0x56502e999894 in remoteDispatchDomainOpenConsole > (server=0x56503027fc30, client=0x56503031ad70, msg=0x565030304eb0, > rerr=0x7feac8935ad0, args=0x7feab402cd70) at > ./remote/remote_daemon_dispatch_stubs.h:9211 > #15 0x56502e99976f in remoteDispatchDomainOpenConsoleHelper > (server=0x56503027fc30, client=0x56503031ad70, msg=0x565030304eb0, > rerr=0x7feac8935ad0, args=0x7feab402cd70, ret=0x0) at > ./remote/remote_daemon_dispatch_stubs.h:9178 > #16 0x7feacdd9b4b1 in virNetServerProgramDispatchCall > (prog=0x5650302ccb70, server=0x56503027fc30, client=0x56503031ad70, > msg=0x565030304eb0) at ../../src/rpc/virnetserverprogram.c:430 > #17 0x7feacdd9b026 in virNetServerProgramDispatch > (prog=0x5650302ccb70, server=0x56503027fc30, client=0x56503031ad70, > msg=0x565030304eb0) at ../../src/rpc/virnetserverprogram.c:302 > #18 0x7feacdda1e34 in virNetServerProcessMsg (srv=0x56503027fc30, > client=0x56503031ad70, prog=0x5650302ccb70, msg=0x565030304eb0) at > ../../src/rpc/virnetserver.c:136 > #19 0x7feacdda1ef4 in virNetServerHandleJob > (jobOpaque=0x565030336ea0, opaque=0x56503027fc30) at > ../../src/rpc/virnetserver.c:153 > #20 0x7feacdc79800 in virThreadPoolWorker (opaque=0x56503028a1f0) at > ../../src/util/virthreadpool.c:163 >
Re: [libvirt] [PATCH] remote_daemon_stream: Hold an extra reference to stream in daemonStreamFilter
Yeah, your patch is perfectly fine for stream freed problem.
But I have reviewed code in daemonStreamEvent() and daemonStreamFilter()
again, and I think there still two issue need to be reconsidered:
1. stream->ref only ++ in daemonStreamFilter, except for error
occurred call daemonRemoveClientStream() lead to ref--, like code as
follow. Though code won't be executed because of no VIR_STREAM_EVENT_HANGUP
event taken place,
but it is not good code:
/* If we got HANGUP, we need to only send an empty
* packet so the client sees an EOF and cleans up
*/
if (!stream->closed && !stream->recvEOF &&
(events & VIR_STREAM_EVENT_HANGUP)) {
virNetMessagePtr msg;
events &= ~(VIR_STREAM_EVENT_HANGUP);
stream->tx = false;
stream->recvEOF = true;
if (!(msg = virNetMessageNew(false))) {
daemonRemoveClientStream(client, stream);
virNetServerClientClose(client);
goto cleanup;
}
msg->cb = daemonStreamMessageFinished;
msg->opaque = stream;
stream->refs++;
if (virNetServerProgramSendStreamData(stream->prog,
client,
msg,
stream->procedure,
stream->serial,
"", 0) < 0) {
virNetMessageFree(msg);
daemonRemoveClientStream(client, stream);
virNetServerClientClose(client);
goto cleanup;
}
}
2. call virNetServerClientClose() is still inappropriate in because the it
may free the client resource which other threads need ref, may be replace
it with virNetServerClientImmediateClose() is better,
the daemon can process client resource release unified
3. Segmentfault may still exists when another thread
call remoteClientCloseFunc in virNetServerClientClose()(for example, when
new force console session break down existed console session, and existed
console session
's client close the session simutaneously, but remoteClientCloseFunc not
lock(priv->lock), so the resource maybe released when daemonStreamEvent ref)
How do you see it?
Best Regards!
Michal Privoznik 于2019年11月26日周二 上午12:49写道：
> In v5.9.0-273-g8ecab214de I've tried to fix a lock ordering
> problem, but introduced a crasher. Problem is that because the
> client lock is unlocked (in order to honour lock ordering) the
> stream we are currently checking in daemonStreamFilter() might be
> freed and thus stream->priv might not even exist when the control
> get to virMutexLock() call.
>
> To resolve this, grab an extra reference to the stream and handle
> its cleanup should the refcounter reach zero after the deref.
> If that's the case and we are the only ones holding a reference
> to the stream, we MUST return a positive value to make
> virNetServerClientDispatchRead() break its loop where it iterates
> over filters. The problem is, if we did not do so, then
> "filter = filter->next" line will read from a memory that was
> just freed (freeing a stream also unregisters its filter).
>
> Signed-off-by: Michal Privoznik
> ---
>
> Reproducing this issue is very easy:
>
> 1) put sleep(5) right after virObjectUnlock(client) in the fist hunk,
> 2) virsh console --force $dom and type something so that the stream
>has some data to process
> 3) while 2) is still running, run the same command from another terminal
> 4) observe libvirtd crash
>
> src/remote/remote_daemon_stream.c | 19 +++
> 1 file changed, 19 insertions(+)
>
> diff --git a/src/remote/remote_daemon_stream.c
> b/src/remote/remote_daemon_stream.c
> index 82cadb67ac..73e4d7befb 100644
> --- a/src/remote/remote_daemon_stream.c
> +++ b/src/remote/remote_daemon_stream.c
> @@ -293,10 +293,25 @@ daemonStreamFilter(virNetServerClientPtr client,
> daemonClientStream *stream = opaque;
> int ret = 0;
>
> +/* We must honour lock ordering here. Client private data lock must
> + * be acquired before client lock. Bu we are already called with
> + * client locked. To avoid stream disappearing while we unlock
> + * everything, let's increase its refcounter. This has some
> + * implications though. */
> +stream->refs++;
> virObjectUnlock(client);
> virMutexLock(>priv->lock);
> virObjectLock(client);
>
> +if (stream->refs == 1) {
> +/* So we are the only ones holding the reference to the stream.
> + * Return 1 to signal to the caller that we've processed the
> + * message. And to "process" means free. */
> +virNetMessageFree(msg);
> +ret = 1;
> +goto cleanup;
> +}
> +
> if (msg->header.type != VIR_NET_STREAM &&
> msg->header.type != VIR_NET_STREAM_HOLE)
> goto cleanup;
> @@ -318,6 +333,10 @@ daemonStreamFilter(virNetServerClientPtr client,
>
> cleanup:
>
Re: [libvirt] [PATCH] Use virNetServerClientImmediateClose() rather than virNetServerClientClose()
As I mentioned, I think add this patch will solve the problem you worried
about
LanceLiu 于2019年11月25日周一 下午7:23写道：
> ---
> src/remote/remote_daemon_stream.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/src/remote/remote_daemon_stream.c
> b/src/remote/remote_daemon_stream.c
> index de0dca3..d206d12 100644
> --- a/src/remote/remote_daemon_stream.c
> +++ b/src/remote/remote_daemon_stream.c
> @@ -141,7 +141,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
> (events & VIR_STREAM_EVENT_WRITABLE)) {
> if (daemonStreamHandleWrite(client, stream) < 0) {
> daemonRemoveClientStream(client, stream);
> -virNetServerClientClose(client);
> +virNetServerClientImmediateClose(client);
> goto cleanup;
> }
> }
> @@ -151,7 +151,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
> events = events & ~(VIR_STREAM_EVENT_READABLE);
> if (daemonStreamHandleRead(client, stream) < 0) {
> daemonRemoveClientStream(client, stream);
> -virNetServerClientClose(client);
> +virNetServerClientImmediateClose(client);
> goto cleanup;
> }
> /* If we detected EOF during read processing,
> @@ -176,7 +176,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
> if (daemonStreamHandleFinish(client, stream, msg) < 0) {
> virNetMessageFree(msg);
> daemonRemoveClientStream(client, stream);
> -virNetServerClientClose(client);
> +virNetServerClientImmediateClose(client);
> goto cleanup;
> }
> break;
> @@ -186,7 +186,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
> if (daemonStreamHandleAbort(client, stream, msg) < 0) {
> virNetMessageFree(msg);
> daemonRemoveClientStream(client, stream);
> -virNetServerClientClose(client);
> +virNetServerClientImmediateClose(client);
> goto cleanup;
> }
> break;
> @@ -205,7 +205,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
> stream->recvEOF = true;
> if (!(msg = virNetMessageNew(false))) {
> daemonRemoveClientStream(client, stream);
> -virNetServerClientClose(client);
> +virNetServerClientImmediateClose(client);
> goto cleanup;
> }
> msg->cb = daemonStreamMessageFinished;
> @@ -219,7 +219,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
>"", 0) < 0) {
> virNetMessageFree(msg);
> daemonRemoveClientStream(client, stream);
> -virNetServerClientClose(client);
> +virNetServerClientImmediateClose(client);
> goto cleanup;
> }
> }
> @@ -262,7 +262,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
> }
> daemonRemoveClientStream(client, stream);
> if (ret < 0)
> -virNetServerClientClose(client);
> + virNetServerClientImmediateClose(client);
> goto cleanup;
> }
>
> --
> 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 cons
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(>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 于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(>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(>priv->lock);
> > +virMutexUnlock(>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(>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 cons
Previous patch can not pass build because of
missing virNetServerClientCheckFilterExist stated in libvirt_remote.syms
LanceLiu 于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(>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(>priv->lock);
> +virMutexUnlock(>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 deadlock when another force console break down an existed console client when this deadlock hanppened, libvirtd backtrace as follow, a typical ABBA deadloc
Thank you!
But it looks like libvirtd daemon stream module still has bug lead to
process segfault if applied this patch(if not, it will lead libvirtd
deadlock).I have got the cause, but it seems difficult to fix it with
present codes。
Bug reproduce:
1. add sleep(3) in daemonStreamFilter() so it is easily to get the point,
as follows
static int
daemonStreamFilter(virNetServerClientPtr client,
virNetMessagePtr msg,
void *opaque)
{
daemonClientStream *stream = opaque;
int ret = 0;
static int first = 1;
//if (first) {
//notify_force_console();
//VIR_ERROR("notify force console to break this client down!client:
%p", client);
//sleep(30);
//VIR_ERROR("sleep 10 finished");
//}
VIR_ERROR("stream: %p, filter_id: %d", stream, stream->filterID);
virObjectUnlock(client);
sleep(3);
virMutexLock(>priv->lock);
virObjectLock(client);
2. build the libvirtd with new code, then replace the libvirtd
3. use virsh console to open a vm's console tty, then just input many chars
in the console
4. open new terminal, then use virsh console --force to break the previous
client, then you'll got libvirtd segfault
it looks because when daemonStreamEvent() with para events
as VIR_STREAM_EVENT_ERROR is called, it will remove stream and free stream,
but daemonStreamFilter still waiting stream->priv->lock mutex, thus cause
invalid memory access
To fix this, I think the most effective and clean way is to drop
client->priv->lock elements, just use client object lock。But for current
code, it's not easy to do that。
also， a workaroud scheme is to add a big lock, but is ugly and low
efficient。
Any good ideas?
Michal Privoznik 于2019年11月19日周二 下午11:41写道：
> On 11/19/19 12:39 PM, LanceLiu wrote:
> > (gdb) thread 23
> > [Switching to thread 23 (Thread 0x7fbb54810700 (LWP 296966))]
> > (gdb) bt
> > at rpc/virnetserverclient.c:1503
> > client=client@entry=0x5641d20e20a0, msg=0x7fbb08003ab0,
> rerr=rerr@entry=0x7fbb5480f7b0, procedure=201,
> > type=type@entry=3, serial=9) at rpc/virnetserverprogram.c:173
> > msg=, rerr=rerr@entry=0x7fbb5480f7b0,
> procedure=, serial=)
> > at rpc/virnetserverprogram.c:222
> > opaque=opaque@entry=0x5641d20e20a0) at stream.c:246
> > force=force@entry=true) at conf/virchrdev.c:386
> > at qemu/qemu_driver.c:16188
> > st=st@entry=0x7fbb08008fc0, flags=1) at libvirt-domain.c:9363
> > rerr=0x7fbb5480fc10, msg=, client=0x5641d20e4e80) at
> remote_dispatch.h:8540
> > rerr=0x7fbb5480fc10, args=0x7fbb08005eb0, ret=) at
> remote_dispatch.h:8505
> > server=0x5641d20be420, prog=0x5641d20da580) at
> rpc/virnetserverprogram.c:437
> > msg=0x5641d20e9a20) at rpc/virnetserverprogram.c:307
> > srv=0x5641d20be420) at rpc/virnetserver.c:148
> > ---Type to continue, or q to quit---
> >
> > (gdb) thread 26
> > [Switching to thread 26 (Thread 0x7fbb87a378c0 (LWP 295636))]
> > (gdb) bt
> > at stream.c:286
> > at rpc/virnetserverclient.c:1247
> > at rpc/virnetserverclient.c:1457
> > at util/vireventpoll.c:508
> >
> > Signed-off-by: LanceLiu
> > ---
> > src/remote/remote_daemon_stream.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/remote/remote_daemon_stream.c
> b/src/remote/remote_daemon_stream.c
> > index 1f6e783..b27348f 100644
> > --- a/src/remote/remote_daemon_stream.c
> > +++ b/src/remote/remote_daemon_stream.c
> > @@ -284,14 +284,16 @@ daemonStreamEvent(virStreamPtr st, int events,
> void *opaque)
> >* -1 on fatal client error
> >*/
> > static int
> > -daemonStreamFilter(virNetServerClientPtr client ATTRIBUTE_UNUSED,
> > +daemonStreamFilter(virNetServerClientPtr client,
>
> This does not look rebased onto current git HEAD ;-) "Luckily", the
> problem you're fixing is still there.
>
> > virNetMessagePtr msg,
> > void *opaque)
> > {
> > daemonClientStream *stream = opaque;
> > int ret = 0;
> >
> > +virObjectUnlock(client);
> > virMutexLock(>priv->lock);
> > +virObjectLock(client);
> >
> > if (msg->header.type != VIR_NET_STREAM &&
> > msg->header.type != VIR_NET_STREAM_HOLE)
> >
>
> I've reworded the commit message, put a comment to
> virNetServerClientFilterFunc() typedef, ACKed and pushed.
>
> Congratulations on your first libvirt contribution!
>
> Michal
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
