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

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(a)linux.ibm.com&gt; > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com&gt; > --- > 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(a)redhat.com&gt;

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 >