[libvirt] [PATCH] remote_daemon_stream: Hold an extra reference to stream in daemonStreamFilter

[Lance Liu]

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.89 at 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 <mprivozn at 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 <mprivozn at 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
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20191128/3c771bc1/attachment-0001.htm>