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

[Marc Hartmayer]

On Thu, Jul 19, 2018 at 04:51 PM +0200, John Ferlan <jferlan at redhat.com> wrote:
> 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,

Yep, I’m fine with that.

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

Thanks!

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