4:06 a.m.
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(a)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
6:10 a.m.
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(a)redhat.com>
---
src/remote/remote_daemon.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
Polite ping.
Michal
7:51 a.m.
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(a)redhat.com>
> ---
> src/remote/remote_daemon.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
Polite ping.
Less polite ping.
Michal
8:27 a.m.
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(a)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
9:35 a.m.
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(a)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
919
days inactive
943
days old
4 comments
3 participants
participants (3)
-
Martin Kletzander -
Michal Privoznik -
Michal Prívozník