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