Hi, Michal: I think the daemonFreeClientStream(NULL, stream); in your patch should line above virMutexUnlock(&stream->priv->lock), or else there is issue WAW. That is two threads may sub stream->
Lance Liu <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: > /* 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 <mpriv...@redhat.com> 于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 <mpriv...@redhat.com> >> --- >> >> 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(&stream->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: >> virMutexUnlock(&stream->priv->lock); >> + /* Don't pass client here, because client is locked here and this >> + * function might try to lock it again which would result in a >> + * deadlock. */ >> + daemonFreeClientStream(NULL, stream); >> return ret; >> } >> >> -- >> 2.23.0 >> >>
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list