[PATCH] remote_daemon: Don't run virStateCleanup() if virStateReload() is still running
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
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
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
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
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
participants (3)
-
Martin Kletzander -
Michal Privoznik -
Michal Prívozník