There is a race between virNetServerProcessClients (main thread) and
remoteDispatchAuthList/remoteDispatchAuthPolkit/remoteSASLFinish (worker
thread) that can lead to decrementing srv->nclients_unauth when it's
zero. Since virNetServerCheckLimits relies on the value
srv->nclients_unauth the underrun causes libvirtd to stop accepting
new connections forever.
Example race scenario (assuming libvirtd is using policykit and the
client is privileged):
1. The client calls the RPC remoteDispatchAuthList =>
remoteDispatchAuthList is executed on a worker thread (Thread
T1). We're assuming now the execution stops for some time before
the line 'virNetServerClientSetAuth(client, 0)'
2. The client closes the connection irregularly. This causes the
event loop to wake up and virNetServerProcessClient to be
called (on the main thread T0). During the
virNetServerProcessClients the srv lock is hold. The condition
virNetServerClientNeedAuth(client) will be checked and as the
authentication is not finished right now
virNetServerTrackCompletedAuthLocked(srv) will be called =>
--srv->nclients_unauth => 0
3. The Thread T1 continues, marks the client as authenticated, and
calls virNetServerTrackCompletedAuthLocked(srv) =>
--srv->nclients_unauth => --0 => wrap around as nclient_unauth is
unsigned
4. virNetServerCheckLimits(srv) will disable the services forever
To fix it, add an auth_pending field to the client struct so that it
is now possible to determine if the authentication process has already
been handled for this client.
Setting the authentication method to none for the client in
virNetServerProcessClients is not a proper way to indicate that the
counter has been decremented, as this would imply that the client is
authenticated.
Additionally, adjust the existing test cases for this new field.
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
---
src/libvirt_remote.syms | 2 +
src/rpc/virnetserver.c | 28 ++++++++++++--
src/rpc/virnetserverclient.c | 44 +++++++++++++++++++++-
src/rpc/virnetserverclient.h | 2 +
.../virnetdaemondata/output-data-admin-nomdns.json | 4 ++
.../output-data-admin-server-names.json | 4 ++
.../virnetdaemondata/output-data-anon-clients.json | 2 +
tests/virnetdaemondata/output-data-client-ids.json | 2 +
.../output-data-client-timestamp.json | 2 +
.../output-data-initial-nomdns.json | 2 +
tests/virnetdaemondata/output-data-initial.json | 2 +
.../output-data-no-keepalive-required.json | 4 ++
12 files changed, 92 insertions(+), 6 deletions(-)
diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 7fcae970484d..cef2794e1122 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -138,6 +138,7 @@ virNetServerClientGetUNIXIdentity;
virNetServerClientImmediateClose;
virNetServerClientInit;
virNetServerClientInitKeepAlive;
+virNetServerClientIsAuthPendingLocked;
virNetServerClientIsClosedLocked;
virNetServerClientIsLocal;
virNetServerClientIsSecure;
@@ -152,6 +153,7 @@ virNetServerClientRemoteAddrStringURI;
virNetServerClientRemoveFilter;
virNetServerClientSendMessage;
virNetServerClientSetAuthLocked;
+virNetServerClientSetAuthPendingLocked;
virNetServerClientSetCloseHook;
virNetServerClientSetDispatcher;
virNetServerClientStartKeepAlive;
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 72105cd9318f..6dd673ff3b23 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -286,7 +286,7 @@ int virNetServerAddClient(virNetServerPtr srv,
srv->clients[srv->nclients-1] = virObjectRef(client);
virObjectLock(client);
- if (virNetServerClientNeedAuthLocked(client))
+ if (virNetServerClientIsAuthPendingLocked(client))
virNetServerTrackPendingAuthLocked(srv);
virObjectUnlock(client);
@@ -738,6 +738,25 @@ int virNetServerSetTLSContext(virNetServerPtr srv,
/**
+ * virNetServerSetClientAuthCompletedLocked:
+ * @srv: server must be locked by the caller
+ * @client: client must be locked by the caller
+ *
+ * Sets on the client that the authentication is no longer pending and
+ * tracks on @srv that the authentication of this @client has been
+ * completed.
+ */
+static void
+virNetServerSetClientAuthCompletedLocked(virNetServerPtr srv, virNetServerClientPtr client)
+{
+ if (virNetServerClientIsAuthPendingLocked(client)) {
+ virNetServerClientSetAuthPendingLocked(client, false);
+ virNetServerTrackCompletedAuthLocked(srv);
+ }
+}
+
+
+/**
* virNetServerSetClientAuthenticated:
* @srv: server must be unlocked
* @client: client must be unlocked
@@ -752,7 +771,7 @@ virNetServerSetClientAuthenticated(virNetServerPtr srv, virNetServerClientPtr cl
virObjectLock(srv);
virObjectLock(client);
virNetServerClientSetAuthLocked(client, VIR_NET_SERVER_SERVICE_AUTH_NONE);
- virNetServerTrackCompletedAuthLocked(srv);
+ virNetServerSetClientAuthCompletedLocked(srv, client);
virNetServerCheckLimits(srv);
virObjectUnlock(client);
virObjectUnlock(srv);
@@ -870,8 +889,9 @@ virNetServerProcessClients(virNetServerPtr srv)
if (virNetServerClientIsClosedLocked(client)) {
VIR_DELETE_ELEMENT(srv->clients, i, srv->nclients);
- if (virNetServerClientNeedAuthLocked(client))
- virNetServerTrackCompletedAuthLocked(srv);
+ /* Mark the authentication for this client as no longer
+ * pending */
+ virNetServerSetClientAuthCompletedLocked(srv, client);
virObjectUnlock(client);
virNetServerCheckLimits(srv);
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 6adc3ec3ec01..5224bc7de1d5 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -71,6 +71,7 @@ struct _virNetServerClient
bool delayedClose;
virNetSocketPtr sock;
int auth;
+ bool auth_pending;
bool readonly;
#if WITH_GNUTLS
virNetTLSContextPtr tlsCtxt;
@@ -375,6 +376,7 @@ static virNetServerClientPtr
virNetServerClientNewInternal(unsigned long long id,
virNetSocketPtr sock,
int auth,
+ bool auth_pending,
#ifdef WITH_GNUTLS
virNetTLSContextPtr tls,
#endif
@@ -384,6 +386,12 @@ virNetServerClientNewInternal(unsigned long long id,
{
virNetServerClientPtr client;
+ /* If the used authentication method implies that the new client
+ * is automatically authenticated, the authentication cannot be
+ * pending */
+ if (auth_pending && virNetServerClientAuthMethodImpliesAuthenticated(auth))
+ return NULL;
+
if (virNetServerClientInitialize() < 0)
return NULL;
@@ -393,6 +401,7 @@ virNetServerClientNewInternal(unsigned long long id,
client->id = id;
client->sock = virObjectRef(sock);
client->auth = auth;
+ client->auth_pending = auth_pending;
client->readonly = readonly;
#ifdef WITH_GNUTLS
client->tlsCtxt = virObjectRef(tls);
@@ -440,6 +449,7 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id,
{
virNetServerClientPtr client;
time_t now;
+ bool auth_pending = !virNetServerClientAuthMethodImpliesAuthenticated(auth);
VIR_DEBUG("sock=%p auth=%d tls=%p", sock, auth,
#ifdef WITH_GNUTLS
@@ -454,7 +464,7 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id,
return NULL;
}
- if (!(client = virNetServerClientNewInternal(id, sock, auth,
+ if (!(client = virNetServerClientNewInternal(id, sock, auth, auth_pending,
#ifdef WITH_GNUTLS
tls,
#endif
@@ -486,7 +496,7 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec
virNetServerClientPtr client = NULL;
virNetSocketPtr sock;
int auth;
- bool readonly;
+ bool readonly, auth_pending;
unsigned int nrequests_max;
unsigned long long id;
long long timestamp;
@@ -496,6 +506,17 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec
_("Missing auth field in JSON state document"));
return NULL;
}
+
+ if (!virJSONValueObjectHasKey(object, "auth_pending")) {
+ auth_pending = !virNetServerClientAuthMethodImpliesAuthenticated(auth);
+ } else {
+ if (virJSONValueObjectGetBoolean(object, "auth_pending", &auth_pending) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Malformed auth_pending field in JSON state document"));
+ return NULL;
+ }
+ }
+
if (virJSONValueObjectGetBoolean(object, "readonly", &readonly) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Missing readonly field in JSON state document"));
@@ -544,6 +565,7 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec
if (!(client = virNetServerClientNewInternal(id,
sock,
auth,
+ auth_pending,
#ifdef WITH_GNUTLS
NULL,
#endif
@@ -592,6 +614,8 @@ virJSONValuePtr virNetServerClientPreExecRestart(virNetServerClientPtr client)
if (virJSONValueObjectAppendNumberInt(object, "auth", client->auth) < 0)
goto error;
+ if (virJSONValueObjectAppendBoolean(object, "auth_pending", client->auth_pending) < 0)
+ goto error;
if (virJSONValueObjectAppendBoolean(object, "readonly", client->readonly) < 0)
goto error;
if (virJSONValueObjectAppendNumberUint(object, "nrequests_max", client->nrequests_max) < 0)
@@ -1545,6 +1569,22 @@ virNetServerClientNeedAuth(virNetServerClientPtr client)
}
+/* The caller must hold the lock for @client */
+void
+virNetServerClientSetAuthPendingLocked(virNetServerClientPtr client, bool auth_pending)
+{
+ client->auth_pending = auth_pending;
+}
+
+
+/* The caller must hold the lock for @client */
+bool
+virNetServerClientIsAuthPendingLocked(virNetServerClientPtr client)
+{
+ return client->auth_pending;
+}
+
+
static void
virNetServerClientKeepAliveDeadCB(void *opaque)
{
diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
index 19f20b808284..f608682dc600 100644
--- a/src/rpc/virnetserverclient.h
+++ b/src/rpc/virnetserverclient.h
@@ -148,6 +148,8 @@ int virNetServerClientSendMessage(virNetServerClientPtr client,
bool virNetServerClientNeedAuth(virNetServerClientPtr client);
bool virNetServerClientNeedAuthLocked(virNetServerClientPtr client);
+bool virNetServerClientIsAuthPendingLocked(virNetServerClientPtr client);
+void virNetServerClientSetAuthPendingLocked(virNetServerClientPtr client, bool auth_pending);
int virNetServerClientGetTransport(virNetServerClientPtr client);
int virNetServerClientGetInfo(virNetServerClientPtr client,
diff --git a/tests/virnetdaemondata/output-data-admin-nomdns.json b/tests/virnetdaemondata/output-data-admin-nomdns.json
index fc960f02cee7..d6d02163e282 100644
--- a/tests/virnetdaemondata/output-data-admin-nomdns.json
+++ b/tests/virnetdaemondata/output-data-admin-nomdns.json
@@ -41,6 +41,7 @@
{
"id": 1,
"auth": 1,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 15,
"sock": {
@@ -53,6 +54,7 @@
{
"id": 2,
"auth": 2,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 66,
"sock": {
@@ -105,6 +107,7 @@
{
"id": 1,
"auth": 1,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 15,
"sock": {
@@ -117,6 +120,7 @@
{
"id": 2,
"auth": 2,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 66,
"sock": {
diff --git a/tests/virnetdaemondata/output-data-admin-server-names.json b/tests/virnetdaemondata/output-data-admin-server-names.json
index fc960f02cee7..d6d02163e282 100644
--- a/tests/virnetdaemondata/output-data-admin-server-names.json
+++ b/tests/virnetdaemondata/output-data-admin-server-names.json
@@ -41,6 +41,7 @@
{
"id": 1,
"auth": 1,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 15,
"sock": {
@@ -53,6 +54,7 @@
{
"id": 2,
"auth": 2,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 66,
"sock": {
@@ -105,6 +107,7 @@
{
"id": 1,
"auth": 1,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 15,
"sock": {
@@ -117,6 +120,7 @@
{
"id": 2,
"auth": 2,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 66,
"sock": {
diff --git a/tests/virnetdaemondata/output-data-anon-clients.json b/tests/virnetdaemondata/output-data-anon-clients.json
index 9f1c63504a1b..cb6005bfe68c 100644
--- a/tests/virnetdaemondata/output-data-anon-clients.json
+++ b/tests/virnetdaemondata/output-data-anon-clients.json
@@ -41,6 +41,7 @@
{
"id": 1,
"auth": 1,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 15,
"sock": {
@@ -53,6 +54,7 @@
{
"id": 2,
"auth": 2,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 66,
"sock": {
diff --git a/tests/virnetdaemondata/output-data-client-ids.json b/tests/virnetdaemondata/output-data-client-ids.json
index 43c61cc46ec7..2b1663d4f8c6 100644
--- a/tests/virnetdaemondata/output-data-client-ids.json
+++ b/tests/virnetdaemondata/output-data-client-ids.json
@@ -41,6 +41,7 @@
{
"id": 2,
"auth": 1,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 15,
"sock": {
@@ -53,6 +54,7 @@
{
"id": 3,
"auth": 2,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 66,
"sock": {
diff --git a/tests/virnetdaemondata/output-data-client-timestamp.json b/tests/virnetdaemondata/output-data-client-timestamp.json
index b706c147ed0e..e8c8e9a991d9 100644
--- a/tests/virnetdaemondata/output-data-client-timestamp.json
+++ b/tests/virnetdaemondata/output-data-client-timestamp.json
@@ -41,6 +41,7 @@
{
"id": 1,
"auth": 1,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 15,
"conn_time": 1234567890,
@@ -54,6 +55,7 @@
{
"id": 2,
"auth": 2,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 66,
"conn_time": 1234567890,
diff --git a/tests/virnetdaemondata/output-data-initial-nomdns.json b/tests/virnetdaemondata/output-data-initial-nomdns.json
index ca6b99553754..167aef8d295e 100644
--- a/tests/virnetdaemondata/output-data-initial-nomdns.json
+++ b/tests/virnetdaemondata/output-data-initial-nomdns.json
@@ -41,6 +41,7 @@
{
"id": 1,
"auth": 1,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 15,
"sock": {
@@ -53,6 +54,7 @@
{
"id": 2,
"auth": 2,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 66,
"sock": {
diff --git a/tests/virnetdaemondata/output-data-initial.json b/tests/virnetdaemondata/output-data-initial.json
index a8df6336c316..3281e868d7aa 100644
--- a/tests/virnetdaemondata/output-data-initial.json
+++ b/tests/virnetdaemondata/output-data-initial.json
@@ -42,6 +42,7 @@
{
"id": 1,
"auth": 1,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 15,
"sock": {
@@ -54,6 +55,7 @@
{
"id": 2,
"auth": 2,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 66,
"sock": {
diff --git a/tests/virnetdaemondata/output-data-no-keepalive-required.json b/tests/virnetdaemondata/output-data-no-keepalive-required.json
index fc960f02cee7..d6d02163e282 100644
--- a/tests/virnetdaemondata/output-data-no-keepalive-required.json
+++ b/tests/virnetdaemondata/output-data-no-keepalive-required.json
@@ -41,6 +41,7 @@
{
"id": 1,
"auth": 1,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 15,
"sock": {
@@ -53,6 +54,7 @@
{
"id": 2,
"auth": 2,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 66,
"sock": {
@@ -105,6 +107,7 @@
{
"id": 1,
"auth": 1,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 15,
"sock": {
@@ -117,6 +120,7 @@
{
"id": 2,
"auth": 2,
+ "auth_pending": true,
"readonly": true,
"nrequests_max": 66,
"sock": {
--
2.13.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/12/2017 06:36 AM, Marc Hartmayer wrote:
> There is a race between virNetServerProcessClients (main thread) and
> remoteDispatchAuthList/remoteDispatchAuthPolkit/remoteSASLFinish (worker
> thread) that can lead to decrementing srv->nclients_unauth when it's
> zero. Since virNetServerCheckLimits relies on the value
> srv->nclients_unauth the underrun causes libvirtd to stop accepting
> new connections forever.
>
> Example race scenario (assuming libvirtd is using policykit and the
> client is privileged):
> 1. The client calls the RPC remoteDispatchAuthList =>
> remoteDispatchAuthList is executed on a worker thread (Thread
> T1). We're assuming now the execution stops for some time before
> the line 'virNetServerClientSetAuth(client, 0)'
> 2. The client closes the connection irregularly. This causes the
> event loop to wake up and virNetServerProcessClient to be
> called (on the main thread T0). During the
> virNetServerProcessClients the srv lock is hold. The condition
> virNetServerClientNeedAuth(client) will be checked and as the
> authentication is not finished right now
> virNetServerTrackCompletedAuthLocked(srv) will be called =>
> --srv->nclients_unauth => 0
> 3. The Thread T1 continues, marks the client as authenticated, and
> calls virNetServerTrackCompletedAuthLocked(srv) =>
> --srv->nclients_unauth => --0 => wrap around as nclient_unauth is
> unsigned
> 4. virNetServerCheckLimits(srv) will disable the services forever
>
> To fix it, add an auth_pending field to the client struct so that it
> is now possible to determine if the authentication process has already
> been handled for this client.
>
> Setting the authentication method to none for the client in
> virNetServerProcessClients is not a proper way to indicate that the
> counter has been decremented, as this would imply that the client is
> authenticated.
>
> Additionally, adjust the existing test cases for this new field.
>
> Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
> ---
> src/libvirt_remote.syms | 2 +
> src/rpc/virnetserver.c | 28 ++++++++++++--
> src/rpc/virnetserverclient.c | 44 +++++++++++++++++++++-
> src/rpc/virnetserverclient.h | 2 +
> .../virnetdaemondata/output-data-admin-nomdns.json | 4 ++
> .../output-data-admin-server-names.json | 4 ++
> .../virnetdaemondata/output-data-anon-clients.json | 2 +
> tests/virnetdaemondata/output-data-client-ids.json | 2 +
> .../output-data-client-timestamp.json | 2 +
> .../output-data-initial-nomdns.json | 2 +
> tests/virnetdaemondata/output-data-initial.json | 2 +
> .../output-data-no-keepalive-required.json | 4 ++
> 12 files changed, 92 insertions(+), 6 deletions(-)
>
Ah... the patch I've been waiting for ;-)... What caused me to
initially look at this series is because I have bz related to some
possible sort of timing problem with TLS authentication, so I figured
reviewing a series related to this code would help me understand the
flow a bit better.
In any case, I think perhaps this can be split into two patches - one
that just sets/clears the auth_pending and then what's leftover as the fix.
> diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
> index 7fcae970484d..cef2794e1122 100644
> --- a/src/libvirt_remote.syms
> +++ b/src/libvirt_remote.syms
> @@ -138,6 +138,7 @@ virNetServerClientGetUNIXIdentity;
> virNetServerClientImmediateClose;
> virNetServerClientInit;
> virNetServerClientInitKeepAlive;
> +virNetServerClientIsAuthPendingLocked;
> virNetServerClientIsClosedLocked;
> virNetServerClientIsLocal;
> virNetServerClientIsSecure;
> @@ -152,6 +153,7 @@ virNetServerClientRemoteAddrStringURI;
> virNetServerClientRemoveFilter;
> virNetServerClientSendMessage;
> virNetServerClientSetAuthLocked;
> +virNetServerClientSetAuthPendingLocked;
> virNetServerClientSetCloseHook;
> virNetServerClientSetDispatcher;
> virNetServerClientStartKeepAlive;
> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> index 72105cd9318f..6dd673ff3b23 100644
> --- a/src/rpc/virnetserver.c
> +++ b/src/rpc/virnetserver.c
> @@ -286,7 +286,7 @@ int virNetServerAddClient(virNetServerPtr srv,
> srv->clients[srv->nclients-1] = virObjectRef(client);
>
> virObjectLock(client);
> - if (virNetServerClientNeedAuthLocked(client))
> + if (virNetServerClientIsAuthPendingLocked(client))
> virNetServerTrackPendingAuthLocked(srv);
> virObjectUnlock(client);
>
> @@ -738,6 +738,25 @@ int virNetServerSetTLSContext(virNetServerPtr srv,
>
>
> /**
> + * virNetServerSetClientAuthCompletedLocked:
> + * @srv: server must be locked by the caller
> + * @client: client must be locked by the caller
> + *
> + * Sets on the client that the authentication is no longer pending and
> + * tracks on @srv that the authentication of this @client has been
> + * completed.
If the client authentication was pending, clear that pending and update
the server tracking.
> + */
> +static void
> +virNetServerSetClientAuthCompletedLocked(virNetServerPtr srv, virNetServerClientPtr client)
One line for each argument.
> +{
> + if (virNetServerClientIsAuthPendingLocked(client)) {
> + virNetServerClientSetAuthPendingLocked(client, false);
> + virNetServerTrackCompletedAuthLocked(srv);
> + }
> +}
> +
> +
> +/**
> * virNetServerSetClientAuthenticated:
> * @srv: server must be unlocked
> * @client: client must be unlocked
> @@ -752,7 +771,7 @@ virNetServerSetClientAuthenticated(virNetServerPtr srv, virNetServerClientPtr cl
> virObjectLock(srv);
> virObjectLock(client);
> virNetServerClientSetAuthLocked(client, VIR_NET_SERVER_SERVICE_AUTH_NONE);
> - virNetServerTrackCompletedAuthLocked(srv);
> + virNetServerSetClientAuthCompletedLocked(srv, client);
> virNetServerCheckLimits(srv);
> virObjectUnlock(client);
> virObjectUnlock(srv);
> @@ -870,8 +889,9 @@ virNetServerProcessClients(virNetServerPtr srv)
> if (virNetServerClientIsClosedLocked(client)) {
> VIR_DELETE_ELEMENT(srv->clients, i, srv->nclients);
>
> - if (virNetServerClientNeedAuthLocked(client))
> - virNetServerTrackCompletedAuthLocked(srv);
> + /* Mark the authentication for this client as no longer
> + * pending */
Some may say a superfluous comment... I'd probably say make it one line
/* Update server authentication tracking */
> + virNetServerSetClientAuthCompletedLocked(srv, client);
> virObjectUnlock(client);
>
> virNetServerCheckLimits(srv);
> diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
> index 6adc3ec3ec01..5224bc7de1d5 100644
> --- a/src/rpc/virnetserverclient.c
> +++ b/src/rpc/virnetserverclient.c
> @@ -71,6 +71,7 @@ struct _virNetServerClient
> bool delayedClose;
> virNetSocketPtr sock;
> int auth;
> + bool auth_pending;
> bool readonly;
> #if WITH_GNUTLS
> virNetTLSContextPtr tlsCtxt;
> @@ -375,6 +376,7 @@ static virNetServerClientPtr
> virNetServerClientNewInternal(unsigned long long id,
> virNetSocketPtr sock,
> int auth,
> + bool auth_pending,
> #ifdef WITH_GNUTLS
> virNetTLSContextPtr tls,
> #endif
> @@ -384,6 +386,12 @@ virNetServerClientNewInternal(unsigned long long id,
> {
> virNetServerClientPtr client;
>
> + /* If the used authentication method implies that the new client
> + * is automatically authenticated, the authentication cannot be
> + * pending */
> + if (auth_pending && virNetServerClientAuthMethodImpliesAuthenticated(auth))
^^^^^^
Why check twice? I'm not clear on the need for this check. The setting
of auth_pending is based on @auth in the caller or perhaps the value of
auth_pending already. If there's some sort of check to be made perhaps
it's in ExecRestart.
Also, if we returned NULL here without some sort of error, then couldn't
we get the libvirt failed for some reason error message which is always
not fun to track down.
> + return NULL;
> +
> if (virNetServerClientInitialize() < 0)
> return NULL;
>
> @@ -393,6 +401,7 @@ virNetServerClientNewInternal(unsigned long long id,
> client->id = id;
> client->sock = virObjectRef(sock);
> client->auth = auth;
> + client->auth_pending = auth_pending;
> client->readonly = readonly;
> #ifdef WITH_GNUTLS
> client->tlsCtxt = virObjectRef(tls);
> @@ -440,6 +449,7 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id,
> {
> virNetServerClientPtr client;
> time_t now;
> + bool auth_pending = !virNetServerClientAuthMethodImpliesAuthenticated(auth);
I think it's much easier for readability to have result be auth !=
VIR_NET_SERVER_SERVICE_AUTH_NONE instead of this call... Perhaps even
easier to find all instances of AUTH_NONE.
>
> VIR_DEBUG("sock=%p auth=%d tls=%p", sock, auth,
> #ifdef WITH_GNUTLS
> @@ -454,7 +464,7 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id,
> return NULL;
> }
>
> - if (!(client = virNetServerClientNewInternal(id, sock, auth,
> + if (!(client = virNetServerClientNewInternal(id, sock, auth, auth_pending,
> #ifdef WITH_GNUTLS
> tls,
> #endif
> @@ -486,7 +496,7 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec
> virNetServerClientPtr client = NULL;
> virNetSocketPtr sock;
> int auth;
> - bool readonly;
> + bool readonly, auth_pending;
> unsigned int nrequests_max;
> unsigned long long id;
> long long timestamp;
> @@ -496,6 +506,17 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec
> _("Missing auth field in JSON state document"));
> return NULL;
> }
> +
> + if (!virJSONValueObjectHasKey(object, "auth_pending")) {
> + auth_pending = !virNetServerClientAuthMethodImpliesAuthenticated(auth);
same here - since not found in .json, then set based on auth !=
AUTH_NONE would be just clearer I think.
> + } else {
> + if (virJSONValueObjectGetBoolean(object, "auth_pending", &auth_pending) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Malformed auth_pending field in JSON state document"));
> + return NULL;
> + }
> + }
> +
> if (virJSONValueObjectGetBoolean(object, "readonly", &readonly) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("Missing readonly field in JSON state document"));
> @@ -544,6 +565,7 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec
> if (!(client = virNetServerClientNewInternal(id,
> sock,
> auth,
> + auth_pending,
> #ifdef WITH_GNUTLS
> NULL,
> #endif
> @@ -592,6 +614,8 @@ virJSONValuePtr virNetServerClientPreExecRestart(virNetServerClientPtr client)
>
> if (virJSONValueObjectAppendNumberInt(object, "auth", client->auth) < 0)
> goto error;
> + if (virJSONValueObjectAppendBoolean(object, "auth_pending", client->auth_pending) < 0)
> + goto error;
> if (virJSONValueObjectAppendBoolean(object, "readonly", client->readonly) < 0)
> goto error;
> if (virJSONValueObjectAppendNumberUint(object, "nrequests_max", client->nrequests_max) < 0)
> @@ -1545,6 +1569,22 @@ virNetServerClientNeedAuth(virNetServerClientPtr client)
> }
>
>
> +/* The caller must hold the lock for @client */
> +void
> +virNetServerClientSetAuthPendingLocked(virNetServerClientPtr client, bool auth_pending)
One line per argument
John
> +{
> + client->auth_pending = auth_pending;
> +}
> +
> +
> +/* The caller must hold the lock for @client */
> +bool
> +virNetServerClientIsAuthPendingLocked(virNetServerClientPtr client)
> +{
> + return client->auth_pending;
> +}
> +
> +
> static void
> virNetServerClientKeepAliveDeadCB(void *opaque)
> {
> diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
> index 19f20b808284..f608682dc600 100644
> --- a/src/rpc/virnetserverclient.h
> +++ b/src/rpc/virnetserverclient.h
> @@ -148,6 +148,8 @@ int virNetServerClientSendMessage(virNetServerClientPtr client,
>
> bool virNetServerClientNeedAuth(virNetServerClientPtr client);
> bool virNetServerClientNeedAuthLocked(virNetServerClientPtr client);
> +bool virNetServerClientIsAuthPendingLocked(virNetServerClientPtr client);
> +void virNetServerClientSetAuthPendingLocked(virNetServerClientPtr client, bool auth_pending);
>
> int virNetServerClientGetTransport(virNetServerClientPtr client);
> int virNetServerClientGetInfo(virNetServerClientPtr client,
> diff --git a/tests/virnetdaemondata/output-data-admin-nomdns.json b/tests/virnetdaemondata/output-data-admin-nomdns.json
> index fc960f02cee7..d6d02163e282 100644
> --- a/tests/virnetdaemondata/output-data-admin-nomdns.json
> +++ b/tests/virnetdaemondata/output-data-admin-nomdns.json
> @@ -41,6 +41,7 @@
> {
> "id": 1,
> "auth": 1,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 15,
> "sock": {
> @@ -53,6 +54,7 @@
> {
> "id": 2,
> "auth": 2,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 66,
> "sock": {
> @@ -105,6 +107,7 @@
> {
> "id": 1,
> "auth": 1,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 15,
> "sock": {
> @@ -117,6 +120,7 @@
> {
> "id": 2,
> "auth": 2,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 66,
> "sock": {
> diff --git a/tests/virnetdaemondata/output-data-admin-server-names.json b/tests/virnetdaemondata/output-data-admin-server-names.json
> index fc960f02cee7..d6d02163e282 100644
> --- a/tests/virnetdaemondata/output-data-admin-server-names.json
> +++ b/tests/virnetdaemondata/output-data-admin-server-names.json
> @@ -41,6 +41,7 @@
> {
> "id": 1,
> "auth": 1,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 15,
> "sock": {
> @@ -53,6 +54,7 @@
> {
> "id": 2,
> "auth": 2,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 66,
> "sock": {
> @@ -105,6 +107,7 @@
> {
> "id": 1,
> "auth": 1,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 15,
> "sock": {
> @@ -117,6 +120,7 @@
> {
> "id": 2,
> "auth": 2,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 66,
> "sock": {
> diff --git a/tests/virnetdaemondata/output-data-anon-clients.json b/tests/virnetdaemondata/output-data-anon-clients.json
> index 9f1c63504a1b..cb6005bfe68c 100644
> --- a/tests/virnetdaemondata/output-data-anon-clients.json
> +++ b/tests/virnetdaemondata/output-data-anon-clients.json
> @@ -41,6 +41,7 @@
> {
> "id": 1,
> "auth": 1,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 15,
> "sock": {
> @@ -53,6 +54,7 @@
> {
> "id": 2,
> "auth": 2,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 66,
> "sock": {
> diff --git a/tests/virnetdaemondata/output-data-client-ids.json b/tests/virnetdaemondata/output-data-client-ids.json
> index 43c61cc46ec7..2b1663d4f8c6 100644
> --- a/tests/virnetdaemondata/output-data-client-ids.json
> +++ b/tests/virnetdaemondata/output-data-client-ids.json
> @@ -41,6 +41,7 @@
> {
> "id": 2,
> "auth": 1,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 15,
> "sock": {
> @@ -53,6 +54,7 @@
> {
> "id": 3,
> "auth": 2,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 66,
> "sock": {
> diff --git a/tests/virnetdaemondata/output-data-client-timestamp.json b/tests/virnetdaemondata/output-data-client-timestamp.json
> index b706c147ed0e..e8c8e9a991d9 100644
> --- a/tests/virnetdaemondata/output-data-client-timestamp.json
> +++ b/tests/virnetdaemondata/output-data-client-timestamp.json
> @@ -41,6 +41,7 @@
> {
> "id": 1,
> "auth": 1,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 15,
> "conn_time": 1234567890,
> @@ -54,6 +55,7 @@
> {
> "id": 2,
> "auth": 2,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 66,
> "conn_time": 1234567890,
> diff --git a/tests/virnetdaemondata/output-data-initial-nomdns.json b/tests/virnetdaemondata/output-data-initial-nomdns.json
> index ca6b99553754..167aef8d295e 100644
> --- a/tests/virnetdaemondata/output-data-initial-nomdns.json
> +++ b/tests/virnetdaemondata/output-data-initial-nomdns.json
> @@ -41,6 +41,7 @@
> {
> "id": 1,
> "auth": 1,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 15,
> "sock": {
> @@ -53,6 +54,7 @@
> {
> "id": 2,
> "auth": 2,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 66,
> "sock": {
> diff --git a/tests/virnetdaemondata/output-data-initial.json b/tests/virnetdaemondata/output-data-initial.json
> index a8df6336c316..3281e868d7aa 100644
> --- a/tests/virnetdaemondata/output-data-initial.json
> +++ b/tests/virnetdaemondata/output-data-initial.json
> @@ -42,6 +42,7 @@
> {
> "id": 1,
> "auth": 1,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 15,
> "sock": {
> @@ -54,6 +55,7 @@
> {
> "id": 2,
> "auth": 2,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 66,
> "sock": {
> diff --git a/tests/virnetdaemondata/output-data-no-keepalive-required.json b/tests/virnetdaemondata/output-data-no-keepalive-required.json
> index fc960f02cee7..d6d02163e282 100644
> --- a/tests/virnetdaemondata/output-data-no-keepalive-required.json
> +++ b/tests/virnetdaemondata/output-data-no-keepalive-required.json
> @@ -41,6 +41,7 @@
> {
> "id": 1,
> "auth": 1,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 15,
> "sock": {
> @@ -53,6 +54,7 @@
> {
> "id": 2,
> "auth": 2,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 66,
> "sock": {
> @@ -105,6 +107,7 @@
> {
> "id": 1,
> "auth": 1,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 15,
> "sock": {
> @@ -117,6 +120,7 @@
> {
> "id": 2,
> "auth": 2,
> + "auth_pending": true,
> "readonly": true,
> "nrequests_max": 66,
> "sock": {
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Dec 15, 2017 at 07:16 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
> On 12/12/2017 06:36 AM, Marc Hartmayer wrote:
>> There is a race between virNetServerProcessClients (main thread) and
>> remoteDispatchAuthList/remoteDispatchAuthPolkit/remoteSASLFinish (worker
>> thread) that can lead to decrementing srv->nclients_unauth when it's
>> zero. Since virNetServerCheckLimits relies on the value
>> srv->nclients_unauth the underrun causes libvirtd to stop accepting
>> new connections forever.
>>
>> Example race scenario (assuming libvirtd is using policykit and the
>> client is privileged):
>> 1. The client calls the RPC remoteDispatchAuthList =>
>> remoteDispatchAuthList is executed on a worker thread (Thread
>> T1). We're assuming now the execution stops for some time before
>> the line 'virNetServerClientSetAuth(client, 0)'
>> 2. The client closes the connection irregularly. This causes the
>> event loop to wake up and virNetServerProcessClient to be
>> called (on the main thread T0). During the
>> virNetServerProcessClients the srv lock is hold. The condition
>> virNetServerClientNeedAuth(client) will be checked and as the
>> authentication is not finished right now
>> virNetServerTrackCompletedAuthLocked(srv) will be called =>
>> --srv->nclients_unauth => 0
>> 3. The Thread T1 continues, marks the client as authenticated, and
>> calls virNetServerTrackCompletedAuthLocked(srv) =>
>> --srv->nclients_unauth => --0 => wrap around as nclient_unauth is
>> unsigned
>> 4. virNetServerCheckLimits(srv) will disable the services forever
>>
>> To fix it, add an auth_pending field to the client struct so that it
>> is now possible to determine if the authentication process has already
>> been handled for this client.
>>
>> Setting the authentication method to none for the client in
>> virNetServerProcessClients is not a proper way to indicate that the
>> counter has been decremented, as this would imply that the client is
>> authenticated.
>>
>> Additionally, adjust the existing test cases for this new field.
>>
>
> In any case, I think perhaps this can be split into two patches - one
> that just sets/clears the auth_pending and then what's leftover as the
> fix.
I’m not sure it’s necessary as the correct usage of auth_pending is
already the fix. But let’s see then.
>
>> diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
>> index 7fcae970484d..cef2794e1122 100644
>> --- a/src/libvirt_remote.syms
>> +++ b/src/libvirt_remote.syms
>> @@ -138,6 +138,7 @@ virNetServerClientGetUNIXIdentity;
>> virNetServerClientImmediateClose;
>> virNetServerClientInit;
>> virNetServerClientInitKeepAlive;
>> +virNetServerClientIsAuthPendingLocked;
>> virNetServerClientIsClosedLocked;
>> virNetServerClientIsLocal;
>> virNetServerClientIsSecure;
>> @@ -152,6 +153,7 @@ virNetServerClientRemoteAddrStringURI;
>> virNetServerClientRemoveFilter;
>> virNetServerClientSendMessage;
>> virNetServerClientSetAuthLocked;
>> +virNetServerClientSetAuthPendingLocked;
>> virNetServerClientSetCloseHook;
>> virNetServerClientSetDispatcher;
>> virNetServerClientStartKeepAlive;
>> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
>> index 72105cd9318f..6dd673ff3b23 100644
>> --- a/src/rpc/virnetserver.c
>> +++ b/src/rpc/virnetserver.c
>> @@ -286,7 +286,7 @@ int virNetServerAddClient(virNetServerPtr srv,
>> srv->clients[srv->nclients-1] = virObjectRef(client);
>>
>> virObjectLock(client);
>> - if (virNetServerClientNeedAuthLocked(client))
>> + if (virNetServerClientIsAuthPendingLocked(client))
>> virNetServerTrackPendingAuthLocked(srv);
>> virObjectUnlock(client);
>>
>> @@ -738,6 +738,25 @@ int virNetServerSetTLSContext(virNetServerPtr srv,
>>
>>
>> /**
>> + * virNetServerSetClientAuthCompletedLocked:
>> + * @srv: server must be locked by the caller
>> + * @client: client must be locked by the caller
>> + *
>> + * Sets on the client that the authentication is no longer pending and
>> + * tracks on @srv that the authentication of this @client has been
>> + * completed.
>
> If the client authentication was pending, clear that pending and update
> the server tracking.
Changed it.
>
>> + */
>> +static void
>> +virNetServerSetClientAuthCompletedLocked(virNetServerPtr srv, virNetServerClientPtr client)
>
> One line for each argument.
Changed.
>
>> +{
>> + if (virNetServerClientIsAuthPendingLocked(client)) {
>> + virNetServerClientSetAuthPendingLocked(client, false);
>> + virNetServerTrackCompletedAuthLocked(srv);
>> + }
>> +}
>> +
>> +
>> +/**
>> * virNetServerSetClientAuthenticated:
>> * @srv: server must be unlocked
>> * @client: client must be unlocked
>> @@ -752,7 +771,7 @@ virNetServerSetClientAuthenticated(virNetServerPtr srv, virNetServerClientPtr cl
>> virObjectLock(srv);
>> virObjectLock(client);
>> virNetServerClientSetAuthLocked(client, VIR_NET_SERVER_SERVICE_AUTH_NONE);
>> - virNetServerTrackCompletedAuthLocked(srv);
>> + virNetServerSetClientAuthCompletedLocked(srv, client);
>> virNetServerCheckLimits(srv);
>> virObjectUnlock(client);
>> virObjectUnlock(srv);
>> @@ -870,8 +889,9 @@ virNetServerProcessClients(virNetServerPtr srv)
>> if (virNetServerClientIsClosedLocked(client)) {
>> VIR_DELETE_ELEMENT(srv->clients, i, srv->nclients);
>>
>> - if (virNetServerClientNeedAuthLocked(client))
>> - virNetServerTrackCompletedAuthLocked(srv);
>> + /* Mark the authentication for this client as no longer
>> + * pending */
>
> Some may say a superfluous comment... I'd probably say make it one line
>
> /* Update server authentication tracking */
Changed.
>
>> + virNetServerSetClientAuthCompletedLocked(srv, client);
>> virObjectUnlock(client);
>>
>> virNetServerCheckLimits(srv);
>> diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
>> index 6adc3ec3ec01..5224bc7de1d5 100644
>> --- a/src/rpc/virnetserverclient.c
>> +++ b/src/rpc/virnetserverclient.c
>> @@ -71,6 +71,7 @@ struct _virNetServerClient
>> bool delayedClose;
>> virNetSocketPtr sock;
>> int auth;
>> + bool auth_pending;
>> bool readonly;
>> #if WITH_GNUTLS
>> virNetTLSContextPtr tlsCtxt;
>> @@ -375,6 +376,7 @@ static virNetServerClientPtr
>> virNetServerClientNewInternal(unsigned long long id,
>> virNetSocketPtr sock,
>> int auth,
>> + bool auth_pending,
>> #ifdef WITH_GNUTLS
>> virNetTLSContextPtr tls,
>> #endif
>> @@ -384,6 +386,12 @@ virNetServerClientNewInternal(unsigned long long id,
>> {
>> virNetServerClientPtr client;
>>
>> + /* If the used authentication method implies that the new client
>> + * is automatically authenticated, the authentication cannot be
>> + * pending */
>> + if (auth_pending && virNetServerClientAuthMethodImpliesAuthenticated(auth))
> ^^^^^^
> Why check twice? I'm not clear on the need for this check. The setting
> of auth_pending is based on @auth in the caller or perhaps the value of
> auth_pending already. If there's some sort of check to be made perhaps
> it's in ExecRestart.
Normally I would prefer an assert here… The intent behind this guard was
that it should prevent the wrong usage of this function.
But I’ll move the check.
>
> Also, if we returned NULL here without some sort of error, then couldn't
> we get the libvirt failed for some reason error message which is always
> not fun to track down.
Does it makes sense to leave the check here (and additionally add it to
ExecRestart) and throw an error message here?
>
>> + return NULL;
>> +
>> if (virNetServerClientInitialize() < 0)
>> return NULL;
>>
>> @@ -393,6 +401,7 @@ virNetServerClientNewInternal(unsigned long long id,
>> client->id = id;
>> client->sock = virObjectRef(sock);
>> client->auth = auth;
>> + client->auth_pending = auth_pending;
>> client->readonly = readonly;
>> #ifdef WITH_GNUTLS
>> client->tlsCtxt = virObjectRef(tls);
>> @@ -440,6 +449,7 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id,
>> {
>> virNetServerClientPtr client;
>> time_t now;
>> + bool auth_pending = !virNetServerClientAuthMethodImpliesAuthenticated(auth);
>
> I think it's much easier for readability to have result be auth !=
> VIR_NET_SERVER_SERVICE_AUTH_NONE instead of this call... Perhaps even
> easier to find all instances of AUTH_NONE.
This function/condition is used in four places. But sure, if you’re
still saying it’s easier to read and to understand that a user is
authenticated (and therefore the authentication is no longer pending) if
auth_method == VIR_NET_SERVER_SERVICE_AUTH_NONE then I’ll replace this
function at all places :)
>
>>
>> VIR_DEBUG("sock=%p auth=%d tls=%p", sock, auth,
>> #ifdef WITH_GNUTLS
>> @@ -454,7 +464,7 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id,
[…snip…]
>> @@ -1545,6 +1569,22 @@ virNetServerClientNeedAuth(virNetServerClientPtr client)
>> }
>>
>>
>> +/* The caller must hold the lock for @client */
>> +void
>> +virNetServerClientSetAuthPendingLocked(virNetServerClientPtr client, bool auth_pending)
>
> One line per argument
Changed.
>
> John
>
>> +{
>> + client->auth_pending = auth_pending;
[…snip]
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/21/2017 06:29 AM, Marc Hartmayer wrote:
> On Fri, Dec 15, 2017 at 07:16 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
>> On 12/12/2017 06:36 AM, Marc Hartmayer wrote:
>>> There is a race between virNetServerProcessClients (main thread) and
>>> remoteDispatchAuthList/remoteDispatchAuthPolkit/remoteSASLFinish (worker
>>> thread) that can lead to decrementing srv->nclients_unauth when it's
>>> zero. Since virNetServerCheckLimits relies on the value
>>> srv->nclients_unauth the underrun causes libvirtd to stop accepting
>>> new connections forever.
>>>
>>> Example race scenario (assuming libvirtd is using policykit and the
>>> client is privileged):
>>> 1. The client calls the RPC remoteDispatchAuthList =>
>>> remoteDispatchAuthList is executed on a worker thread (Thread
>>> T1). We're assuming now the execution stops for some time before
>>> the line 'virNetServerClientSetAuth(client, 0)'
>>> 2. The client closes the connection irregularly. This causes the
>>> event loop to wake up and virNetServerProcessClient to be
>>> called (on the main thread T0). During the
>>> virNetServerProcessClients the srv lock is hold. The condition
>>> virNetServerClientNeedAuth(client) will be checked and as the
>>> authentication is not finished right now
>>> virNetServerTrackCompletedAuthLocked(srv) will be called =>
>>> --srv->nclients_unauth => 0
>>> 3. The Thread T1 continues, marks the client as authenticated, and
>>> calls virNetServerTrackCompletedAuthLocked(srv) =>
>>> --srv->nclients_unauth => --0 => wrap around as nclient_unauth is
>>> unsigned
>>> 4. virNetServerCheckLimits(srv) will disable the services forever
>>>
>>> To fix it, add an auth_pending field to the client struct so that it
>>> is now possible to determine if the authentication process has already
>>> been handled for this client.
>>>
>>> Setting the authentication method to none for the client in
>>> virNetServerProcessClients is not a proper way to indicate that the
>>> counter has been decremented, as this would imply that the client is
>>> authenticated.
>>>
>>> Additionally, adjust the existing test cases for this new field.
>>>
>>
>> In any case, I think perhaps this can be split into two patches - one
>> that just sets/clears the auth_pending and then what's leftover as the
>> fix.
>
> I’m not sure it’s necessary as the correct usage of auth_pending is
> already the fix. But let’s see then.
>
OK. I understand. I assume by this point I was juggling in my head a few
of the various adjustments so I figured that by adding the flag and
separating the logic between just the set/clear flag and the reason it
was added would be easier to review, but then again that would make the
set/clear patch essentially meaningless.
>>
>>> diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
>>> index 7fcae970484d..cef2794e1122 100644
>>> --- a/src/libvirt_remote.syms
>>> +++ b/src/libvirt_remote.syms
>>> @@ -138,6 +138,7 @@ virNetServerClientGetUNIXIdentity;
>>> virNetServerClientImmediateClose;
>>> virNetServerClientInit;
>>> virNetServerClientInitKeepAlive;
>>> +virNetServerClientIsAuthPendingLocked;
>>> virNetServerClientIsClosedLocked;
>>> virNetServerClientIsLocal;
>>> virNetServerClientIsSecure;
>>> @@ -152,6 +153,7 @@ virNetServerClientRemoteAddrStringURI;
>>> virNetServerClientRemoveFilter;
>>> virNetServerClientSendMessage;
>>> virNetServerClientSetAuthLocked;
>>> +virNetServerClientSetAuthPendingLocked;
>>> virNetServerClientSetCloseHook;
>>> virNetServerClientSetDispatcher;
>>> virNetServerClientStartKeepAlive;
>>> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
>>> index 72105cd9318f..6dd673ff3b23 100644
>>> --- a/src/rpc/virnetserver.c
>>> +++ b/src/rpc/virnetserver.c
>>> @@ -286,7 +286,7 @@ int virNetServerAddClient(virNetServerPtr srv,
>>> srv->clients[srv->nclients-1] = virObjectRef(client);
>>>
>>> virObjectLock(client);
>>> - if (virNetServerClientNeedAuthLocked(client))
>>> + if (virNetServerClientIsAuthPendingLocked(client))
>>> virNetServerTrackPendingAuthLocked(srv);
>>> virObjectUnlock(client);
>>>
>>> @@ -738,6 +738,25 @@ int virNetServerSetTLSContext(virNetServerPtr srv,
>>>
>>>
>>> /**
>>> + * virNetServerSetClientAuthCompletedLocked:
>>> + * @srv: server must be locked by the caller
>>> + * @client: client must be locked by the caller
>>> + *
>>> + * Sets on the client that the authentication is no longer pending and
>>> + * tracks on @srv that the authentication of this @client has been
>>> + * completed.
>>
>> If the client authentication was pending, clear that pending and update
>> the server tracking.
>
> Changed it.
>
>>
>>> + */
>>> +static void
>>> +virNetServerSetClientAuthCompletedLocked(virNetServerPtr srv, virNetServerClientPtr client)
>>
>> One line for each argument.
>
> Changed.
>
>>
>>> +{
>>> + if (virNetServerClientIsAuthPendingLocked(client)) {
>>> + virNetServerClientSetAuthPendingLocked(client, false);
>>> + virNetServerTrackCompletedAuthLocked(srv);
>>> + }
>>> +}
>>> +
>>> +
>>> +/**
>>> * virNetServerSetClientAuthenticated:
>>> * @srv: server must be unlocked
>>> * @client: client must be unlocked
>>> @@ -752,7 +771,7 @@ virNetServerSetClientAuthenticated(virNetServerPtr srv, virNetServerClientPtr cl
>>> virObjectLock(srv);
>>> virObjectLock(client);
>>> virNetServerClientSetAuthLocked(client, VIR_NET_SERVER_SERVICE_AUTH_NONE);
>>> - virNetServerTrackCompletedAuthLocked(srv);
>>> + virNetServerSetClientAuthCompletedLocked(srv, client);
>>> virNetServerCheckLimits(srv);
>>> virObjectUnlock(client);
>>> virObjectUnlock(srv);
>>> @@ -870,8 +889,9 @@ virNetServerProcessClients(virNetServerPtr srv)
>>> if (virNetServerClientIsClosedLocked(client)) {
>>> VIR_DELETE_ELEMENT(srv->clients, i, srv->nclients);
>>>
>>> - if (virNetServerClientNeedAuthLocked(client))
>>> - virNetServerTrackCompletedAuthLocked(srv);
>>> + /* Mark the authentication for this client as no longer
>>> + * pending */
>>
>> Some may say a superfluous comment... I'd probably say make it one line
>>
>> /* Update server authentication tracking */
>
> Changed.
>
>>
>>> + virNetServerSetClientAuthCompletedLocked(srv, client);
>>> virObjectUnlock(client);
>>>
>>> virNetServerCheckLimits(srv);
>>> diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
>>> index 6adc3ec3ec01..5224bc7de1d5 100644
>>> --- a/src/rpc/virnetserverclient.c
>>> +++ b/src/rpc/virnetserverclient.c
>>> @@ -71,6 +71,7 @@ struct _virNetServerClient
>>> bool delayedClose;
>>> virNetSocketPtr sock;
>>> int auth;
>>> + bool auth_pending;
>>> bool readonly;
>>> #if WITH_GNUTLS
>>> virNetTLSContextPtr tlsCtxt;
>>> @@ -375,6 +376,7 @@ static virNetServerClientPtr
>>> virNetServerClientNewInternal(unsigned long long id,
>>> virNetSocketPtr sock,
>>> int auth,
>>> + bool auth_pending,
>>> #ifdef WITH_GNUTLS
>>> virNetTLSContextPtr tls,
>>> #endif
>>> @@ -384,6 +386,12 @@ virNetServerClientNewInternal(unsigned long long id,
>>> {
>>> virNetServerClientPtr client;
>>>
>>> + /* If the used authentication method implies that the new client
>>> + * is automatically authenticated, the authentication cannot be
>>> + * pending */
>>> + if (auth_pending && virNetServerClientAuthMethodImpliesAuthenticated(auth))
>> ^^^^^^
>> Why check twice? I'm not clear on the need for this check. The setting
>> of auth_pending is based on @auth in the caller or perhaps the value of
>> auth_pending already. If there's some sort of check to be made perhaps
>> it's in ExecRestart.
>
> Normally I would prefer an assert here… The intent behind this guard was
> that it should prevent the wrong usage of this function.
>
> But I’ll move the check.
>
I think this is perhaps why I made the separate the patches comment. I
was starting to overflow my heap.
>>
>> Also, if we returned NULL here without some sort of error, then couldn't
>> we get the libvirt failed for some reason error message which is always
>> not fun to track down.
>
> Does it makes sense to leave the check here (and additionally add it to
> ExecRestart) and throw an error message here?
>
Adding a check, a message, and having a NULL return to guard against
some future change passing the wrong parameters perhaps means that
future patch submitter (and eventual reviewer) didn't clearly understand
the purpose of the function...
Still this is a *NewInternal function - I would hope passing correct
arguments wouldn't be problem for it.
>>
>>> + return NULL;
>>> +
>>> if (virNetServerClientInitialize() < 0)
>>> return NULL;
>>>
>>> @@ -393,6 +401,7 @@ virNetServerClientNewInternal(unsigned long long id,
>>> client->id = id;
>>> client->sock = virObjectRef(sock);
>>> client->auth = auth;
>>> + client->auth_pending = auth_pending;
>>> client->readonly = readonly;
>>> #ifdef WITH_GNUTLS
>>> client->tlsCtxt = virObjectRef(tls);
>>> @@ -440,6 +449,7 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id,
>>> {
>>> virNetServerClientPtr client;
>>> time_t now;
>>> + bool auth_pending = !virNetServerClientAuthMethodImpliesAuthenticated(auth);
>>
>> I think it's much easier for readability to have result be auth !=
>> VIR_NET_SERVER_SERVICE_AUTH_NONE instead of this call... Perhaps even
>> easier to find all instances of AUTH_NONE.
>
> This function/condition is used in four places. But sure, if you’re
> still saying it’s easier to read and to understand that a user is
> authenticated (and therefore the authentication is no longer pending) if
> auth_method == VIR_NET_SERVER_SERVICE_AUTH_NONE then I’ll replace this
> function at all places :)
>
By this time I wasn't sure what was going to happen from Dan's patch 8
comment and the virNetServerClientNeedAuth[Locked] functions vs. the
name chosen (ImpliesAuthenticated) that was AFAICT trying to convey what
you discovered.
Whether the comparison is hidden behind some call or a direct comparison
I guess just becomes an implementation detail. I use cscope a lot to
find things, so searching on NONE is perhaps easier that searching on
NONE, finding some API that returning true/false based off of NONE, and
then searching on callers for that too. OTOH, it's cleaner to have a
helper <sigh>.
Shorter answer, your call. If you want the helper, that's fine.
John
>>
>>>
>>> VIR_DEBUG("sock=%p auth=%d tls=%p", sock, auth,
>>> #ifdef WITH_GNUTLS
>>> @@ -454,7 +464,7 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id,
>
> […snip…]
>
>>> @@ -1545,6 +1569,22 @@ virNetServerClientNeedAuth(virNetServerClientPtr client)
>>> }
>>>
>>>
>>> +/* The caller must hold the lock for @client */
>>> +void
>>> +virNetServerClientSetAuthPendingLocked(virNetServerClientPtr client, bool auth_pending)
>>
>> One line per argument
>
> Changed.
>
>>
>> John
>>
>>> +{
>>> + client->auth_pending = auth_pending;
>
> […snip]
>
> --
> Beste Grüße / Kind regards
> Marc Hartmayer
>
> IBM Deutschland Research & Development GmbH
> Vorsitzende des Aufsichtsrats: Martina Koederitz
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen
> Registergericht: Amtsgericht Stuttgart, HRB 243294
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Dec 21, 2017 at 01:18 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
> On 12/21/2017 06:29 AM, Marc Hartmayer wrote:
>> On Fri, Dec 15, 2017 at 07:16 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
>>> On 12/12/2017 06:36 AM, Marc Hartmayer wrote:
>>>> There is a race between virNetServerProcessClients (main thread) and
>>>> remoteDispatchAuthList/remoteDispatchAuthPolkit/remoteSASLFinish (worker
>>>> thread) that can lead to decrementing srv->nclients_unauth when it's
>>>> zero. Since virNetServerCheckLimits relies on the value
>>>> srv->nclients_unauth the underrun causes libvirtd to stop accepting
>>>> new connections forever.
>>>>
>>>> Example race scenario (assuming libvirtd is using policykit and the
>>>> client is privileged):
>>>> 1. The client calls the RPC remoteDispatchAuthList =>
>>>> remoteDispatchAuthList is executed on a worker thread (Thread
>>>> T1). We're assuming now the execution stops for some time before
>>>> the line 'virNetServerClientSetAuth(client, 0)'
>>>> 2. The client closes the connection irregularly. This causes the
>>>> event loop to wake up and virNetServerProcessClient to be
>>>> called (on the main thread T0). During the
>>>> virNetServerProcessClients the srv lock is hold. The condition
>>>> virNetServerClientNeedAuth(client) will be checked and as the
>>>> authentication is not finished right now
>>>> virNetServerTrackCompletedAuthLocked(srv) will be called =>
>>>> --srv->nclients_unauth => 0
>>>> 3. The Thread T1 continues, marks the client as authenticated, and
>>>> calls virNetServerTrackCompletedAuthLocked(srv) =>
>>>> --srv->nclients_unauth => --0 => wrap around as nclient_unauth is
>>>> unsigned
>>>> 4. virNetServerCheckLimits(srv) will disable the services forever
>>>>
>>>> To fix it, add an auth_pending field to the client struct so that it
>>>> is now possible to determine if the authentication process has already
>>>> been handled for this client.
>>>>
>>>> Setting the authentication method to none for the client in
>>>> virNetServerProcessClients is not a proper way to indicate that the
>>>> counter has been decremented, as this would imply that the client is
>>>> authenticated.
>>>>
>>>> Additionally, adjust the existing test cases for this new field.
>>>>
>>>
>>> In any case, I think perhaps this can be split into two patches - one
>>> that just sets/clears the auth_pending and then what's leftover as the
>>> fix.
>>
>> I’m not sure it’s necessary as the correct usage of auth_pending is
>> already the fix. But let’s see then.
>>
>
> OK. I understand. I assume by this point I was juggling in my head a few
> of the various adjustments so I figured that by adding the flag and
> separating the logic between just the set/clear flag and the reason it
> was added would be easier to review, but then again that would make the
> set/clear patch essentially meaningless.
>
>>>
>>>> diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
>>>> index 7fcae970484d..cef2794e1122 100644
>>>> --- a/src/libvirt_remote.syms
>>>> +++ b/src/libvirt_remote.syms
>>>> @@ -138,6 +138,7 @@ virNetServerClientGetUNIXIdentity;
>>>> virNetServerClientImmediateClose;
[…snip…]
>>>> @@ -375,6 +376,7 @@ static virNetServerClientPtr
>>>> virNetServerClientNewInternal(unsigned long long id,
>>>> virNetSocketPtr sock,
>>>> int auth,
>>>> + bool auth_pending,
>>>> #ifdef WITH_GNUTLS
>>>> virNetTLSContextPtr tls,
>>>> #endif
>>>> @@ -384,6 +386,12 @@ virNetServerClientNewInternal(unsigned long long id,
>>>> {
>>>> virNetServerClientPtr client;
>>>>
>>>> + /* If the used authentication method implies that the new client
>>>> + * is automatically authenticated, the authentication cannot be
>>>> + * pending */
>>>> + if (auth_pending && virNetServerClientAuthMethodImpliesAuthenticated(auth))
>>> ^^^^^^
>>> Why check twice? I'm not clear on the need for this check. The setting
>>> of auth_pending is based on @auth in the caller or perhaps the value of
>>> auth_pending already. If there's some sort of check to be made perhaps
>>> it's in ExecRestart.
>>
>> Normally I would prefer an assert here… The intent behind this guard was
>> that it should prevent the wrong usage of this function.
>>
>> But I’ll move the check.
>>
>
> I think this is perhaps why I made the separate the patches comment. I
> was starting to overflow my heap.
>
>>>
>>> Also, if we returned NULL here without some sort of error, then couldn't
>>> we get the libvirt failed for some reason error message which is always
>>> not fun to track down.
>>
>> Does it makes sense to leave the check here (and additionally add it to
>> ExecRestart) and throw an error message here?
>>
>
> Adding a check, a message, and having a NULL return to guard against
> some future change passing the wrong parameters perhaps means that
> future patch submitter (and eventual reviewer) didn't clearly understand
> the purpose of the function...
Yep. That’s why I like asserts… (for functions used internally
only). You can 'tell' other developers what are the
preconditions/postconditions and help them to understand the code
flow. Sorry for the off topic…
Short answer, I’ll move the check.
>
> Still this is a *NewInternal function - I would hope passing correct
> arguments wouldn't be problem for it.
Me too.
>
>>>
>>>> + return NULL;
>>>> +
>>>> if (virNetServerClientInitialize() < 0)
>>>> return NULL;
>>>>
>>>> @@ -393,6 +401,7 @@ virNetServerClientNewInternal(unsigned long long id,
>>>> client->id = id;
>>>> client->sock = virObjectRef(sock);
>>>> client->auth = auth;
>>>> + client->auth_pending = auth_pending;
>>>> client->readonly = readonly;
>>>> #ifdef WITH_GNUTLS
>>>> client->tlsCtxt = virObjectRef(tls);
>>>> @@ -440,6 +449,7 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id,
>>>> {
>>>> virNetServerClientPtr client;
>>>> time_t now;
>>>> + bool auth_pending = !virNetServerClientAuthMethodImpliesAuthenticated(auth);
>>>
>>> I think it's much easier for readability to have result be auth !=
>>> VIR_NET_SERVER_SERVICE_AUTH_NONE instead of this call... Perhaps even
>>> easier to find all instances of AUTH_NONE.
>>
>> This function/condition is used in four places. But sure, if you’re
>> still saying it’s easier to read and to understand that a user is
>> authenticated (and therefore the authentication is no longer pending) if
>> auth_method == VIR_NET_SERVER_SERVICE_AUTH_NONE then I’ll replace this
>> function at all places :)
>>
>
> By this time I wasn't sure what was going to happen from Dan's patch 8
> comment and the virNetServerClientNeedAuth[Locked] functions vs. the
> name chosen (ImpliesAuthenticated) that was AFAICT trying to convey what
> you discovered.
>
> Whether the comparison is hidden behind some call or a direct comparison
> I guess just becomes an implementation detail. I use cscope a lot to
> find things, so searching on NONE is perhaps easier that searching on
> NONE, finding some API that returning true/false based off of NONE, and
> then searching on callers for that too. OTOH, it's cleaner to have a
> helper <sigh>.
>
> Shorter answer, your call. If you want the helper, that's fine.
I’ll think about it.
[…snip]
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.