1. Patchew
  2. Libvirt
  3. View series

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

Michal Privoznik posted 1 patch 2 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/f9e3fafe230ad64963a4c4fd644ca6abf9fe5f01.1650877566.git.mprivozn@redhat.com
There is a newer version of this series
src/remote/remote_daemon.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
[PATCH] remote_daemon: Don't run virStateCleanup() if virStateReload() is still running
Posted by Michal Privoznik 2 years ago 
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@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);
+    if (virStateReload() < 0) {
         VIR_WARN("Error while reloading drivers");
+    } 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) {
         VIR_WARN("Drivers are not initialized, reload ignored");
         return;
     }
@@ -607,7 +612,7 @@ static void daemonRunStateInit(void *opaque)
         goto cleanup;
     }
 
-    driversInitialized = true;
+    g_atomic_int_inc(&driversInitialized);
 
     virNetDaemonSetShutdownCallbacks(dmn,
                                      virStateShutdownPrepare,
@@ -1212,10 +1217,10 @@ int main(int argc, char **argv) {
  cleanup:
     virNetlinkEventServiceStopAll();
 
-    if (driversInitialized) {
+    if (g_atomic_int_get(&driversInitialized) != 0) {
         /* NB: Possible issue with timing window between driversInitialized
          * setting if virNetlinkEventServerStart fails */
-        driversInitialized = false;
+        g_atomic_int_set(&driversInitialized, 0);
         virStateCleanup();
     }
 
-- 
2.35.1
Re: [PATCH] remote_daemon: Don't run virStateCleanup() if virStateReload() is still running
Posted by Martin Kletzander 1 year, 11 months ago 
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@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.

>+    } 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]

>         VIR_WARN("Drivers are not initialized, reload ignored");
>         return;
>     }
>@@ -607,7 +612,7 @@ static void daemonRunStateInit(void *opaque)
>         goto cleanup;
>     }
>
>-    driversInitialized = true;
>+    g_atomic_int_inc(&driversInitialized);
>
>     virNetDaemonSetShutdownCallbacks(dmn,
>                                      virStateShutdownPrepare,
>@@ -1212,10 +1217,10 @@ int main(int argc, char **argv) {
>  cleanup:
>     virNetlinkEventServiceStopAll();
>
>-    if (driversInitialized) {
>+    if (g_atomic_int_get(&driversInitialized) != 0) {
>         /* NB: Possible issue with timing window between driversInitialized
>          * setting if virNetlinkEventServerStart fails */
>-        driversInitialized = false;
>+        g_atomic_int_set(&driversInitialized, 0);
>         virStateCleanup();
>     }
>
>-- 
>2.35.1
>
Re: [PATCH] remote_daemon: Don't run virStateCleanup() if virStateReload() is still running
Posted by Michal Prívozník 1 year, 11 months ago 
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@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
Re: [PATCH] remote_daemon: Don't run virStateCleanup() if virStateReload() is still running
Posted by Michal Prívozník 2 years ago 
On 4/25/22 11:06, 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@redhat.com>
> ---
>  src/remote/remote_daemon.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)

Polite ping.

Michal
Re: [PATCH] remote_daemon: Don't run virStateCleanup() if virStateReload() is still running
Posted by Michal Prívozník 1 year, 11 months ago 
On 5/5/22 13:10, Michal Prívozník wrote:
> On 4/25/22 11:06, 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@redhat.com>
>> ---
>>  src/remote/remote_daemon.c | 17 +++++++++++------
>>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> Polite ping.

Less polite ping.

Michal

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.