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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

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.

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

Attachment: signature.asc
Description: PGP signature