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

[Lance Liu]

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(&stream->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 <liu.lance.89 at gmail.com <mailto: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:

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 <mprivozn at redhat.com> 于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(&stream->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 <liu.lance.89 at gmail.com <mailto: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:
>
> 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)) {
> >              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;
> >              }
>
> 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.
>
> >          }
> >     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().
>
> Michal
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20191128/fba15f99/attachment-0001.htm>