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

[Michal Privoznik]

On 11/22/19 8:00 AM, Lance Liu wrote:
> 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

Yes, this is exactly what is happening.

> 
> 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?

I think we need to get back to drawing board. In the original problem 
you observed ABBA, but also was patching older libvirt. I convinced 
myself that the problem still occurs even with (at that point) current 
master:

1) virNetServerClientDispatchEvent() locks @client,
    virNetServerClientDispatchRead()
    daemonStreamFilter() which locks stream->priv->lock;

on the other hand:

2) daemonStreamEvent() locks stream->priv->lock,
    virNetServerClientClose() which locks @client; /* there are other 
functions called over @client which also lock @client, I've just picked 
one */


So there is ABBA pattern. However, there might be another lock that is 
breaking the pattern (although I don't see it now): CAB and another 
thread then does CBA.

Do you have a reproducer for your original problem? Does it reproduce on 
the current master (after you revert the patch I've merged of course).

Michal