[libvirt] [PATCH v2 1/6] rpc: Fix deadlock if there is no worker pool available

[John Ferlan]

On 07/03/2018 07:37 AM, Marc Hartmayer wrote:
> @srv must be unlocked for the call virNetServerProcessMsg otherwise a
> deadlock can occur.
> 
> Since the pointer 'srv->workers' will never be changed after
> initialization and the thread pool has it's own locking we can release
> the lock of 'srv' earlier. This also fixes the deadlock.
> 
> Signed-off-by: Marc Hartmayer <mhartmay at linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy at linux.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk at linux.ibm.com>
> ---
>  src/rpc/virnetserver.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> index 5c7f7dd08fe1..091e3ef23406 100644
> --- a/src/rpc/virnetserver.c
> +++ b/src/rpc/virnetserver.c
> @@ -51,6 +51,7 @@ struct _virNetServer {
>  
>      char *name;
>  
> +    /* Immutable pointer, self-locking APIs */
>      virThreadPoolPtr workers;
>  
>      char *mdnsGroupName;
> @@ -177,9 +178,11 @@ static void virNetServerHandleJob(void *jobOpaque, void *opaque)
>      VIR_FREE(job);
>  }
>  
> -static void virNetServerDispatchNewMessage(virNetServerClientPtr client,
> -                                           virNetMessagePtr msg,
> -                                           void *opaque)
> +
> +static void
> +virNetServerDispatchNewMessage(virNetServerClientPtr client,
> +                               virNetMessagePtr msg,
> +                               void *opaque)
>  {
>      virNetServerPtr srv = opaque;
>      virNetServerProgramPtr prog = NULL;
> @@ -196,6 +199,9 @@ static void virNetServerDispatchNewMessage(virNetServerClientPtr client,
>              break;
>          }
>      }

Should we grab an object reference then to avoid the chance that
something disposes srv right after it's unlocked? And then Unref instead
of Unlock later on?

Following virThreadPoolSendJob finds that it'll grab the srv->workers
pool mutex and check the quit flag before adding a job to and of course
as you point out virNetServerProcessMsg would eventually assume that the
server is unlocked in virNetServerProgramDispatchCall before calling
dispatcher->func.

With a Ref/Unref, I'd feel better and thus consider this,

Reviewed-by: John Ferlan <jferlan at redhat.com>

John

> +    /* we can unlock @srv since @prog can only become invalid in case
> +     * of disposing @srv */
> +    virObjectUnlock(srv);
>  
>      if (srv->workers) {
>          virNetServerJobPtr job;
> @@ -223,15 +229,14 @@ static void virNetServerDispatchNewMessage(virNetServerClientPtr client,
>              goto error;
>      }
>  
> -    virObjectUnlock(srv);
>      return;
>  
>   error:
>      virNetMessageFree(msg);
>      virNetServerClientClose(client);
> -    virObjectUnlock(srv);
>  }
>  
> +
>  /**
>   * virNetServerCheckLimits:
>   * @srv: server to check limits on
>