[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 deadlock circumstance:

[Lance Liu]

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(&stream->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 <mprivozn at redhat.com> 于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 at entry=0x5641d20e20a0, msg=0x7fbb08003ab0,
> rerr=rerr at entry=0x7fbb5480f7b0, procedure=201,
> >      type=type at entry=3, serial=9) at rpc/virnetserverprogram.c:173
> >      msg=<optimized out>, rerr=rerr at entry=0x7fbb5480f7b0,
> procedure=<optimized out>, serial=<optimized out>)
> >      at rpc/virnetserverprogram.c:222
> >      opaque=opaque at entry=0x5641d20e20a0) at stream.c:246
> >      force=force at entry=true) at conf/virchrdev.c:386
> >      at qemu/qemu_driver.c:16188
> >      st=st at entry=0x7fbb08008fc0, flags=1) at libvirt-domain.c:9363
> >      rerr=0x7fbb5480fc10, msg=<optimized out>, client=0x5641d20e4e80) at
> remote_dispatch.h:8540
> >      rerr=0x7fbb5480fc10, args=0x7fbb08005eb0, ret=<optimized out>) 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 <return> to continue, or q <return> 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 <liu.lance.89 at gmail.com>
> > ---
> >   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(&stream->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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20191122/54343398/attachment-0001.htm>