<div dir="ltr"><div>Hi, Michal:</div> 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-><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Lance Liu <<a href="mailto:liu.lance.89@gmail.com" target="_blank">liu.lance.89@gmail.com</a>> 于2019年11月26日周二 下午1:54写道：<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Yeah, your patch is perfectly fine for stream freed problem.<div>But I have reviewed code in daemonStreamEvent() and daemonStreamFilter() again, and I think there still two issue need to be reconsidered:</div><div>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,</div><div>but it is not good code:<br>/* If we got HANGUP, we need to only send an empty<br> * packet so the client sees an EOF and cleans up<br> */<br> if (!stream->closed && !stream->recvEOF &&<br> (events & VIR_STREAM_EVENT_HANGUP)) {<br> virNetMessagePtr msg;<br> events &= ~(VIR_STREAM_EVENT_HANGUP);<br> stream->tx = false;<br> stream->recvEOF = true;<br> if (!(msg = virNetMessageNew(false))) {<br> daemonRemoveClientStream(client, stream);<br> virNetServerClientClose(client);<br> goto cleanup;<br> }<br> msg->cb = daemonStreamMessageFinished;<br> msg->opaque = stream;<br> <font color="#ff0000">stream->refs++;</font><br> if (virNetServerProgramSendStreamData(stream->prog,<br> client,<br> msg,<br> stream->procedure,<br> stream->serial,<br> "", 0) < 0) {<br> virNetMessageFree(msg);<br> daemonRemoveClientStream(client, stream);<br> virNetServerClientClose(client);<br> goto cleanup;<br> }<br> }<br></div><div>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,</div><div>the daemon can process client resource release unified</div><div>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</div><div>'s client close the session simutaneously, but remoteClientCloseFunc not lock(priv->lock), so the resource maybe released when daemonStreamEvent ref)</div><div><br></div><div>How do you see it?</div><div><br></div><div><br></div><div>Best Regards!</div><div><br></div><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Michal Privoznik <<a href="mailto:mprivozn@redhat.com" target="_blank">mprivozn@redhat.com</a>> 于2019年11月26日周二 上午12:49写道：<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">In v5.9.0-273-g8ecab214de I've tried to fix a lock ordering<br>
problem, but introduced a crasher. Problem is that because the<br>
client lock is unlocked (in order to honour lock ordering) the<br>
stream we are currently checking in daemonStreamFilter() might be<br>
freed and thus stream->priv might not even exist when the control<br>
get to virMutexLock() call.<br>
<br>
To resolve this, grab an extra reference to the stream and handle<br>
its cleanup should the refcounter reach zero after the deref.<br>
If that's the case and we are the only ones holding a reference<br>
to the stream, we MUST return a positive value to make<br>
virNetServerClientDispatchRead() break its loop where it iterates<br>
over filters. The problem is, if we did not do so, then<br>
"filter = filter->next" line will read from a memory that was<br>
just freed (freeing a stream also unregisters its filter).<br>
<br>
Signed-off-by: Michal Privoznik <<a href="mailto:mprivozn@redhat.com" target="_blank">mprivozn@redhat.com</a>><br>
---<br>
<br>
Reproducing this issue is very easy:<br>
<br>
1) put sleep(5) right after virObjectUnlock(client) in the fist hunk,<br>
2) virsh console --force $dom and type something so that the stream<br>
has some data to process<br>
3) while 2) is still running, run the same command from another terminal<br>
4) observe libvirtd crash<br>
<br>
src/remote/remote_daemon_stream.c | 19 +++++++++++++++++++<br>
1 file changed, 19 insertions(+)<br>
<br>
diff --git a/src/remote/remote_daemon_stream.c b/src/remote/remote_daemon_stream.c<br>
index 82cadb67ac..73e4d7befb 100644<br>
--- a/src/remote/remote_daemon_stream.c<br>
+++ b/src/remote/remote_daemon_stream.c<br>
@@ -293,10 +293,25 @@ daemonStreamFilter(virNetServerClientPtr client,<br>
daemonClientStream *stream = opaque;<br>
int ret = 0;<br>
<br>
+ /* We must honour lock ordering here. Client private data lock must<br>
+ * be acquired before client lock. Bu we are already called with<br>
+ * client locked. To avoid stream disappearing while we unlock<br>
+ * everything, let's increase its refcounter. This has some<br>
+ * implications though. */<br>
+ stream->refs++;<br>
virObjectUnlock(client);<br>
virMutexLock(&stream->priv->lock);<br>
virObjectLock(client);<br>
<br>
+ if (stream->refs == 1) {<br>
+ /* So we are the only ones holding the reference to the stream.<br>
+ * Return 1 to signal to the caller that we've processed the<br>
+ * message. And to "process" means free. */<br>
+ virNetMessageFree(msg);<br>
+ ret = 1;<br>
+ goto cleanup;<br>
+ }<br>
+<br>
if (msg->header.type != VIR_NET_STREAM &&<br>
msg->header.type != VIR_NET_STREAM_HOLE)<br>
goto cleanup;<br>
@@ -318,6 +333,10 @@ daemonStreamFilter(virNetServerClientPtr client,<br>
<br>
cleanup:<br>
virMutexUnlock(&stream->priv->lock);<br>
+ /* Don't pass client here, because client is locked here and this<br>
+ * function might try to lock it again which would result in a<br>
+ * deadlock. */<br>
+ daemonFreeClientStream(NULL, stream);<br>
return ret;<br>
}<br>
<br>
-- <br>
2.23.0<br>
<br>
</blockquote></div>
</blockquote></div></div>