[PATCH 0/2] remoteDispatchAuthPolkit: Fix lock ordering deadlock if client closes connection during auth
11:13 p.m.
Peter Krempa (2):
reproducer
remoteDispatchAuthPolkit: Fix lock ordering deadlock if client closes
connection during auth
src/remote/remote_daemon_dispatch.c | 76 +++++++++++++++--------------
src/rpc/virnetserver.c | 26 +++++++++-
src/rpc/virnetserverclient.c | 2 +
3 files changed, 65 insertions(+), 39 deletions(-)
--
2.43.0
11:13 p.m.
New subject: [PATCH 1/2] reproducer
To reproduce the issue, start virtqemud with polkit auth enabled while
looking at the logs. Run a virsh command which will ask for
authentication.
After providing the password CTRL+C the virsh instance.
Any subsequent connection will get stuck
---
src/rpc/virnetserver.c | 26 ++++++++++++++++++++++++--
src/rpc/virnetserverclient.c | 2 ++
2 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index a6c6443c55..f5f50464a2 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -821,8 +821,30 @@ void
virNetServerSetClientAuthenticated(virNetServer *srv,
virNetServerClient *client)
{
- VIR_LOCK_GUARD server_lock = virObjectLockGuard(srv);
- VIR_LOCK_GUARD client_lock = virObjectLockGuard(client);
+ VIR_LOCK_GUARD server_lock;
+ VIR_LOCK_GUARD client_lock;
+
+ printf("\n\n\n!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n");
+ printf(" kill virsh now; waiting 10 s\n");
+ printf("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n");
+
+ sleep(10);
+
+ printf("\n\n\n!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n");
+ printf(" continuing\n");
+ printf("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n");
+
+ server_lock = virObjectLockGuard(srv);
+
+ printf("\n\n\n!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n");
+ printf(" srv lokced\n");
+ printf("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n");
+
+ client_lock = virObjectLockGuard(client);
+
+ printf("\n\n\n!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n");
+ printf(" client lokced\n");
+ printf("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n");
virNetServerClientSetAuthLocked(client, VIR_NET_SERVER_SERVICE_AUTH_NONE);
virNetServerSetClientAuthCompletedLocked(srv, client);
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 355aab4b04..bfdf121243 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -1013,6 +1013,8 @@ virNetServerClientCloseLocked(virNetServerClient *client)
if (client->sock) {
g_clear_pointer(&client->sock, virObjectUnref);
}
+
+ VIR_DEBUG("client closed");
}
--
2.43.0
11:13 p.m.
New subject: [PATCH 2/2] remoteDispatchAuthPolkit: Fix lock ordering deadlock if client closes connection during auth
Locks in following text:
A: virNetServer
B: virNetServerClient
C: daemonClientPrivate
'virNetServerSetClientAuthenticated' locks A then B
'remoteDispatchAuthPolkit' calls 'virNetServerSetClientAuthenticated'
while holding C.
If a client closes its connection 'virNetServerProcessClients' with the
lock A and B locked will call 'virNetServerClientCloseLocked' which will
try to dispose of the 'client' private data by:
ref(b);
unlock(b);
remoteClientFreePrivateCallbacks();
lock(b);
unref(b);
Unfortunately remoteClientFreePrivateCallbacks() tries lock C.
Thus the locks are held in the following order:
polkit auth: C -> A
connection close: A -> C
causing a textbook-example deadlock. To resolve it we can simply drop
lock 'C' before calling 'virNetServerSetClientAuthenticated' as the lock
is not needed any more.
Resolves: https://issues.redhat.com/browse/RHEL-20337
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/remote/remote_daemon_dispatch.c | 76 +++++++++++++++--------------
1 file changed, 39 insertions(+), 37 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index 7daf503b51..aaabd1e56c 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -3979,50 +3979,52 @@ remoteDispatchAuthPolkit(virNetServer *server,
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
int rv;
- VIR_LOCK_GUARD lock = virLockGuardLock(&priv->lock);
-
- action = virNetServerClientGetReadonly(client) ?
- "org.libvirt.unix.monitor" :
- "org.libvirt.unix.manage";
- VIR_DEBUG("Start PolicyKit auth %d", virNetServerClientGetFD(client));
- if (virNetServerClientGetAuth(client) != VIR_NET_SERVER_SERVICE_AUTH_POLKIT) {
- VIR_ERROR(_("client tried invalid PolicyKit init request"));
- goto authfail;
- }
+ VIR_WITH_MUTEX_LOCK_GUARD(&priv->lock) {
+ action = virNetServerClientGetReadonly(client) ?
+ "org.libvirt.unix.monitor" :
+ "org.libvirt.unix.manage";
- if (virNetServerClientGetUNIXIdentity(client, &callerUid, &callerGid,
- &callerPid, ×tamp) < 0) {
- goto authfail;
- }
+ VIR_DEBUG("Start PolicyKit auth %d", virNetServerClientGetFD(client));
+ if (virNetServerClientGetAuth(client) != VIR_NET_SERVER_SERVICE_AUTH_POLKIT) {
+ VIR_ERROR(_("client tried invalid PolicyKit init request"));
+ goto authfail;
+ }
- if (timestamp == 0) {
- VIR_WARN("Failing polkit auth due to missing client (pid=%lld) start
time",
- (long long)callerPid);
- goto authfail;
- }
+ if (virNetServerClientGetUNIXIdentity(client, &callerUid, &callerGid,
+ &callerPid, ×tamp) < 0) {
+ goto authfail;
+ }
- VIR_INFO("Checking PID %lld running as %d",
- (long long) callerPid, callerUid);
+ if (timestamp == 0) {
+ VIR_WARN("Failing polkit auth due to missing client (pid=%lld) start
time",
+ (long long)callerPid);
+ goto authfail;
+ }
- rv = virPolkitCheckAuth(action,
- callerPid,
- timestamp,
- callerUid,
- NULL,
- true);
- if (rv == -1)
- goto authfail;
- else if (rv == -2)
- goto authdeny;
+ VIR_INFO("Checking PID %lld running as %d",
+ (long long) callerPid, callerUid);
- PROBE(RPC_SERVER_CLIENT_AUTH_ALLOW,
- "client=%p auth=%d identity=%s",
- client, REMOTE_AUTH_POLKIT, ident);
- VIR_INFO("Policy allowed action %s from pid %lld, uid %d",
- action, (long long) callerPid, callerUid);
- ret->complete = 1;
+ rv = virPolkitCheckAuth(action,
+ callerPid,
+ timestamp,
+ callerUid,
+ NULL,
+ true);
+ if (rv == -1)
+ goto authfail;
+ else if (rv == -2)
+ goto authdeny;
+
+ PROBE(RPC_SERVER_CLIENT_AUTH_ALLOW,
+ "client=%p auth=%d identity=%s",
+ client, REMOTE_AUTH_POLKIT, ident);
+ VIR_INFO("Policy allowed action %s from pid %lld, uid %d",
+ action, (long long) callerPid, callerUid);
+ ret->complete = 1;
+ }
+ /* this must be called with the private data mutex unlocked */
virNetServerSetClientAuthenticated(server, client);
return 0;
--
2.43.0
5:17 p.m.
New subject: [PATCH 2/2] remoteDispatchAuthPolkit: Fix lock ordering deadlock
if client closes connection during auth
On Wed, Jan 17, 2024 at 04:13:30PM +0100, Peter Krempa wrote:
Locks in following text:
A: virNetServer
B: virNetServerClient
C: daemonClientPrivate
'virNetServerSetClientAuthenticated' locks A then B
'remoteDispatchAuthPolkit' calls 'virNetServerSetClientAuthenticated'
while holding C.
If a client closes its connection 'virNetServerProcessClients' with the
lock A and B locked will call 'virNetServerClientCloseLocked' which will
try to dispose of the 'client' private data by:
ref(b);
unlock(b);
remoteClientFreePrivateCallbacks();
lock(b);
unref(b);
Unfortunately remoteClientFreePrivateCallbacks() tries lock C.
Thus the locks are held in the following order:
polkit auth: C -> A
connection close: A -> C
causing a textbook-example deadlock. To resolve it we can simply drop
lock 'C' before calling 'virNetServerSetClientAuthenticated' as the lock
is not needed any more.
Resolves: https://issues.redhat.com/browse/RHEL-20337
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
Reviewed-by: Martin Kletzander <mkletzan(a)redhat.com>
227
days inactive
234
days old
3 comments
2 participants
participants (2)
-
Martin Kletzander -
Peter Krempa