Re: [libvirt] [PATCH 11/14] rpc: virnetserver: Fix race on srv->nclients_unauth

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.

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

> @@ -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;