[libvirt] [PATCH] rpc: Avoid deadlock when closing client connection
We need to drop the server lock before calling virObjectUnlock(client) since in case we had the last reference to the client, its dispose callback would be called and that could possibly try to lock the server and cause a deadlock. This is exactly what happens when there is only one QEMU domain running and it is marked to be autodestroyed when the connection dies. This results in qemuProcessAutoDestroy -> qemuProcessStop -> virNetServerRemoveShutdownInhibition call sequence, where the last function locks the server. --- src/rpc/virnetserver.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 95333d0..e536cc3 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -1120,7 +1120,7 @@ void virNetServerRun(virNetServerPtr srv) if (virNetServerClientWantClose(srv->clients[i])) virNetServerClientClose(srv->clients[i]); if (virNetServerClientIsClosed(srv->clients[i])) { - virObjectUnref(srv->clients[i]); + virNetServerClientPtr client = srv->clients[i]; if (srv->nclients > 1) { memmove(srv->clients + i, srv->clients + i + 1, @@ -1131,6 +1131,10 @@ void virNetServerRun(virNetServerPtr srv) srv->nclients = 0; } + virObjectUnlock(srv); + virObjectUnref(client); + virObjectLock(srv); + goto reprocess; } } -- 1.8.1.2
On Mon, Feb 18, 2013 at 03:52:40PM +0100, Jiri Denemark wrote:
We need to drop the server lock before calling virObjectUnlock(client) since in case we had the last reference to the client, its dispose callback would be called and that could possibly try to lock the server and cause a deadlock. This is exactly what happens when there is only one QEMU domain running and it is marked to be autodestroyed when the connection dies. This results in qemuProcessAutoDestroy -> qemuProcessStop -> virNetServerRemoveShutdownInhibition call sequence, where the last function locks the server. --- src/rpc/virnetserver.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 95333d0..e536cc3 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -1120,7 +1120,7 @@ void virNetServerRun(virNetServerPtr srv) if (virNetServerClientWantClose(srv->clients[i])) virNetServerClientClose(srv->clients[i]); if (virNetServerClientIsClosed(srv->clients[i])) { - virObjectUnref(srv->clients[i]); + virNetServerClientPtr client = srv->clients[i]; if (srv->nclients > 1) { memmove(srv->clients + i, srv->clients + i + 1, @@ -1131,6 +1131,10 @@ void virNetServerRun(virNetServerPtr srv) srv->nclients = 0; }
+ virObjectUnlock(srv); + virObjectUnref(client); + virObjectLock(srv); + goto reprocess; } }
Ordinarily unlocking in the middle of aloop is unsafe, but the 'goto reprocess' restarts the entire loop, so it is fine in this scenario. ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Feb 19, 2013 at 14:21:42 +0000, Daniel P. Berrange wrote:
On Mon, Feb 18, 2013 at 03:52:40PM +0100, Jiri Denemark wrote:
We need to drop the server lock before calling virObjectUnlock(client) since in case we had the last reference to the client, its dispose callback would be called and that could possibly try to lock the server and cause a deadlock. This is exactly what happens when there is only one QEMU domain running and it is marked to be autodestroyed when the connection dies. This results in qemuProcessAutoDestroy -> qemuProcessStop -> virNetServerRemoveShutdownInhibition call sequence, where the last function locks the server. ---
ACK
Thanks, pushed. Jirka
participants (2)
-
Daniel P. Berrange -
Jiri Denemark