[PATCH] remote_daemon: Don't run virStateCleanup() if virStateReload() is still running

[Michal Prívozník]

On 5/19/22 15:27, Martin Kletzander wrote:
> On Mon, Apr 25, 2022 at 11:06:06AM +0200, Michal Privoznik wrote:
>> When a SIGHUP is received a thread is spawned that runs
>> virStateReload(). However, if SIGINT is received while the former
>> thread is still running then we may get into problematic
>> situation: the cleanup code in main() sees drivers initialized
>> and thus calls virStateCleanup(). So now we have two threads, one
>> running virStateReload() the other virStateCleanup(). In this
>> situation it's very likely that a race condition occurs and
>> either of threads causes SIGSEGV.
>>
>> To fix this, unmark drivers as initialized in the
>> virStateReload() thread for the time the function runs.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2075837
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>> src/remote/remote_daemon.c | 17 +++++++++++------
>> 1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
>> index 26469e0d9f..37d27f93f4 100644
>> --- a/src/remote/remote_daemon.c
>> +++ b/src/remote/remote_daemon.c
>> @@ -77,7 +77,7 @@ virNetSASLContext *saslCtxt = NULL;
>> virNetServerProgram *remoteProgram = NULL;
>> virNetServerProgram *qemuProgram = NULL;
>>
>> -volatile bool driversInitialized = false;
>> +volatile gint driversInitialized = 0;
>>
>> static void daemonErrorHandler(void *opaque G_GNUC_UNUSED,
>>                                virErrorPtr err G_GNUC_UNUSED)
>> @@ -453,8 +453,13 @@ static void daemonReloadHandlerThread(void
>> *opaque G_GNUC_UNUSED)
>>     VIR_INFO("Reloading configuration on SIGHUP");
>>     virHookCall(VIR_HOOK_DRIVER_DAEMON, "-",
>>                 VIR_HOOK_DAEMON_OP_RELOAD, SIGHUP, "SIGHUP", NULL, NULL);
>> -    if (virStateReload() < 0)
>> +
>> +    g_atomic_int_set(&driversInitialized, 0);
> 
> [0] come back here after the comment below [1]
> 
> Something like the following would be a "workaround":
> 
> if (g_atomic_int_dec_and_test(&driversInitialized))
>     return;
> 
>> +    if (virStateReload() < 0) {
>>         VIR_WARN("Error while reloading drivers");
> 
> But I wonder about this.  Since we could not reload the drivers, we will
> never be able to reload them again.  I guess we leave it to the user to
> restart the daemon whenever they find a non-functioning API, right?  I
> guess that's fine, it's just that before the reload could've been made
> again and maybe increasing it to 1 again would not be that big of a deal.

Alright, I can do that.

> 
>> +    } else {
>> +        g_atomic_int_inc(&driversInitialized);
>> +    }
>> }
>>
>> static void daemonReloadHandler(virNetDaemon *dmn G_GNUC_UNUSED,
>> @@ -463,7 +468,7 @@ static void daemonReloadHandler(virNetDaemon *dmn
>> G_GNUC_UNUSED,
>> {
>>     virThread thr;
>>
>> -    if (!driversInitialized) {
>> +    if (g_atomic_int_get(&driversInitialized) == 0) {
> 
> [1] I would prefer something like g_atomic_int_inc_and_test (which does
> not exist) here so that there are no two reload handlers running at the
> same time, essentially eliminating the race window.  Since it does not
> exist we can do [0]

But g_atomic_int_add() exists and according to documentation it's atomic
version of:

  { tmp = *atomic; *atomic += val; return tmp; }

meaning if two threads execute g_atomic_int_add(&driversInitialized, 1)
only one will see retval of 0, whilst the other will see 1.

Let me see if I can rewrite the code and work your comments in.

Michal