Re: [PATCH v2] fence: introduce a file-based self-fence mechanism

[Marc-André Lureau]

Hi

On Wed, Feb 5, 2020 at 10:57 AM Felipe Franciosi <address@hidden> wrote:
>
> This introduces a self-fence mechanism to Qemu, causing it to die if a
> heartbeat condition is not met. Currently, a file-based heartbeat is
> available and can be configured as follows:
>
> -object file-fence,id=ff0,file=/foo,qtimeout=20,ktimeout=25,signal=kill
>
> Qemu will watch 'file' for attribute changes. Touching the file works as
> a heartbeat. This parameter is mandatory.
>
> Fencing happens after 'qtimeout' or 'ktimeout' seconds elapse without a
> heartbeat. At least one of these must be specified. Both may be used, in
> which case 'ktimeout' must be greater than 'qtimeout'. Setting either to
> zero has no effect (as if they weren't specified).
>
> When using 'qtimeout', an internal Qemu timer is used. Fencing with this
> method gives Qemu a chance to write a log message indicating which file
> caused the event. If Qemu's main loop is hung for whatever reason, this
> method won't successfully kill Qemu.
>
> When using 'ktimeout', a kernel timer is used. In this case, 'signal'
> can be 'kill' (for SIGKILL, default) or 'quit' (for SIGQUIT). Using
> SIGQUIT may be preferred for obtaining core dumps. If Qemu is hung
> (eg. uninterruptable sleep), this method won't successfully kill Qemu.
>
> It is worth noting that even successfully killing Qemu may not be
> sufficient to completely fence a VM as certain operations like network
> packets or block commands may be pending in the kernel. If that is a
> concern, systems should consider using further fencing mechanisms like
> hardware watchdogs either instead or in conjunction with this for
> additional protection.
>
> Signed-off-by: Felipe Franciosi <address@hidden>
> ---
>  backends/Makefile.objs |   2 +
>  backends/file-fence.c  | 374 +++++++++++++++++++++++++++++++++++++++++
>  qemu-options.hx        |  27 ++-
>  3 files changed, 402 insertions(+), 1 deletion(-)
>  create mode 100644 backends/file-fence.c
>
> Changelog:
> v1->v2:
> - Publish patch in https://github.com/franciozzy/qemu/tree/filefence
> - Rename file_fence to file-fence and move to backends/
> - Use error_printf() instead of printf() when fencing
> - Replace a check already done by filemonitor-inotify with assert
> - Add return value to _setup() functions to simplify error logic
> - Use g_ascii_strcasecmp() to simplify logic in _set_signal()
> - Use glib memory allocation helpers in _set_file()
> - Fix bug to allow using qtimeout without ktimeout
> - Clarify usage of q/k timeouts in commit message
> - Clarify usage of hardware watchdogs in commits message
>
> diff --git a/backends/Makefile.objs b/backends/Makefile.objs
> index 28a847cd57..da2a589bdf 100644
> --- a/backends/Makefile.objs
> +++ b/backends/Makefile.objs
> @@ -9,6 +9,8 @@ common-obj-$(CONFIG_POSIX) += hostmem-file.o
>  common-obj-y += cryptodev.o
>  common-obj-y += cryptodev-builtin.o
>
> +common-obj-y += file-fence.o
> +
>  ifeq ($(CONFIG_VIRTIO_CRYPTO),y)
>  common-obj-y += cryptodev-vhost.o
>  common-obj-$(CONFIG_VHOST_CRYPTO) += cryptodev-vhost-user.o
> diff --git a/backends/file-fence.c b/backends/file-fence.c
> new file mode 100644
> index 0000000000..3dbbed7325
> --- /dev/null
> +++ b/backends/file-fence.c
> @@ -0,0 +1,374 @@
> +/*
> + * QEMU file-based self-fence mechanism
> + *
> + * Copyright (c) 2019 Nutanix Inc. All rights reserved.
> + *
> + * Authors:
> + *    Felipe Franciosi <address@hidden>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qom/object_interfaces.h"
> +#include "qemu/error-report.h"
> +#include "qemu/filemonitor.h"
> +#include "qemu/timer.h"
> +
> +#include <time.h>
> +
> +#define TYPE_FILE_FENCE "file-fence"
> +
> +typedef struct FileFence {
> +    Object parent_obj;
> +
> +    gchar *dir;
> +    gchar *file;
> +    uint32_t qtimeout;
> +    uint32_t ktimeout;
> +    int signal;
> +
> +    timer_t ktimer;
> +    QEMUTimer *qtimer;
> +
> +    QFileMonitor *fm;
> +    uint64_t id;
> +} FileFence;
> +
> +#define FILE_FENCE(obj) \
> +    OBJECT_CHECK(FileFence, (obj), TYPE_FILE_FENCE)
> +
> +static void
> +timer_update(FileFence *ff)
> +{
> +    struct itimerspec its = {
> +        .it_value.tv_sec = ff->ktimeout,
> +    };
> +    int err;
> +
> +    if (ff->qtimeout) {
> +        timer_mod(ff->qtimer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
> +                              ff->qtimeout * 1000);
> +    }
> +
> +    if (ff->ktimeout) {
> +        err = timer_settime(ff->ktimer, 0, &its, NULL);
> +        g_assert(err == 0);
> +    }
> +}
> +
> +static void
> +file_fence_abort_cb(void *opaque)
> +{
> +    FileFence *ff = opaque;
> +    error_printf("Fencing after %u seconds on '%s'\n",
> +                 ff->qtimeout, g_strconcat(ff->dir, "/", ff->file, NULL));
> +    abort();
> +}
> +
> +static void
> +file_fence_watch_cb(int64_t id, QFileMonitorEvent ev, const char *file,
> +                    void *opaque)
> +{
> +    FileFence *ff = opaque;
> +
> +    if (ev != QFILE_MONITOR_EVENT_ATTRIBUTES) {
> +        return;
> +    }
> +
> +    g_assert(g_str_equal(file, ff->file));
> +
> +    timer_update(ff);
> +}
> +
> +static void
> +ktimer_tear(FileFence *ff)
> +{
> +    int err;
> +
> +    if (ff->ktimer) {
> +        err = timer_delete(ff->ktimer);
> +        g_assert(err == 0);
> +        ff->ktimer = NULL;
> +    }
> +}
> +
> +static gboolean
> +ktimer_setup(FileFence *ff, Error **errp)
> +{
> +    int err;
> +
> +    struct sigevent sev = {
> +        .sigev_notify = SIGEV_SIGNAL,
> +        .sigev_signo = ff->signal ? ff->signal : SIGKILL,
> +    };
> +
> +    if (ff->ktimeout == 0) {
> +        return TRUE;
> +    }
> +
> +    err = timer_create(CLOCK_MONOTONIC, &sev, &ff->ktimer);
> +    if (err == -1) {
> +        error_setg(errp, "Error creating kernel timer: %m");
> +        return FALSE;
> +    }
> +
> +    return TRUE;
> +}
> +
> +static void
> +qtimer_tear(FileFence *ff)
> +{
> +    if (ff->qtimer) {
> +        timer_del(ff->qtimer);
> +        timer_free(ff->qtimer);
> +    }
> +    ff->qtimer = NULL;
> +}
> +
> +static gboolean
> +qtimer_setup(FileFence *ff, Error **errp)
> +{
> +    QEMUTimer *qtimer;
> +
> +    if (ff->qtimeout == 0) {
> +        return TRUE;
> +    }
> +
> +    qtimer = timer_new_ms(QEMU_CLOCK_REALTIME, file_fence_abort_cb, ff);
> +    if (qtimer == NULL) {
> +        error_setg(errp, "Error creating Qemu timer");
> +        return FALSE;
> +    }
> +
> +    ff->qtimer = qtimer;
> +
> +    return TRUE;
> +}
> +
> +static void
> +watch_tear(FileFence *ff)
> +{
> +    if (ff->fm) {
> +        qemu_file_monitor_remove_watch(ff->fm, ff->dir, ff->id);
> +        qemu_file_monitor_free(ff->fm);
> +        ff->fm = NULL;
> +        ff->id = 0;
> +    }
> +}
> +
> +static gboolean
> +watch_setup(FileFence *ff, Error **errp)
> +{
> +    QFileMonitor *fm;
> +    int64_t id;
> +
> +    fm = qemu_file_monitor_new(errp);
> +    if (!fm) {
> +        return FALSE;
> +    }
> +
> +    id = qemu_file_monitor_add_watch(fm, ff->dir, ff->file,
> +                                     file_fence_watch_cb, ff, errp);
> +    if (id < 0) {
> +        qemu_file_monitor_free(fm);
> +        return FALSE;
> +    }
> +
> +    ff->fm = fm;
> +    ff->id = id;
> +
> +    return TRUE;
> +}
> +
> +static void
> +file_fence_complete(UserCreatable *obj, Error **errp)
> +{
> +    FileFence *ff = FILE_FENCE(obj);
> +
> +    if (ff->dir == NULL) {
> +        error_setg(errp, "A 'file' must be set");
> +        return;
> +    }
> +
> +    if (ff->signal != 0 && ff->ktimeout == 0) {
> +        error_setg(errp, "Using 'signal' requires 'ktimeout' to be set");
> +        return;
> +    }
> +
> +    if (ff->ktimeout == 0 && ff->qtimeout == 0) {
> +        error_setg(errp, "One or both of 'ktimeout' or 'qtimeout' must be 
> set");
> +        return;
> +    }
> +
> +    if (ff->qtimeout >= ff->ktimeout && ff->ktimeout != 0) {
> +        error_setg(errp, "Using 'qtimeout' >= 'ktimeout' doesn't make 
> sense");
> +        return;
> +    }
> +
> +    if (!watch_setup(ff, errp) ||
> +        !qtimer_setup(ff, errp) ||
> +        !ktimer_setup(ff, errp)) {
> +        return;
> +    }
> +
> +    timer_update(ff);
> +
> +    return;
> +}
> +
> +static void
> +file_fence_set_signal(Object *obj, const char *value, Error **errp)
> +{
> +    FileFence *ff = FILE_FENCE(obj);
> +
> +    if (ff->signal) {
> +        error_setg(errp, "Signal property already set");
> +        return;
> +    }
> +
> +    if (value == NULL) {
> +        goto err;
> +    }
> +
> +    if (g_ascii_strcasecmp(value, "QUIT") == 0) {
> +        ff->signal = SIGQUIT;
> +        return;
> +    }
> +
> +    if (g_ascii_strcasecmp(value, "KILL") == 0) {
> +        ff->signal = SIGKILL;
> +        return;
> +    }
> +
> +err:
> +    error_setg(errp, "Invalid signal. Must be 'quit' or 'kill'");
> +}
> +
> +static char *
> +file_fence_get_signal(Object *obj, Error **errp)
> +{
> +    FileFence *ff = FILE_FENCE(obj);
> +
> +    switch (ff->signal) {
> +    case SIGKILL:
> +        return g_strdup("kill");
> +    case SIGQUIT:
> +        return g_strdup("quit");
> +    }
> +
> +    /* Unreachable */
> +    abort();
> +}
> +
> +static void
> +file_fence_set_file(Object *obj, const char *value, Error **errp)
> +{
> +    FileFence *ff = FILE_FENCE(obj);
> +    g_autofree gchar *dir = NULL, *file = NULL;
> +
> +    if (ff->dir) {
> +        error_setg(errp, "File property already set");
> +        return;
> +    }
> +
> +    dir = g_path_get_dirname(value);
> +    if (g_str_equal(dir, ".")) {
> +        error_setg(errp, "Path for file-fence must be absolute");

g_path_is_absolute() ? why such limitation ?

> +        return;
> +    }
> +
> +    file = g_path_get_basename(value);
> +    if (g_str_equal(file, ".")) {
> +        error_setg(errp, "Path for file-fence must be a file");

I think you would get "." if value is "".

I am not sure you need extra error handling here, since watch_setup()
will fail if it can't open the file.

> +        return;
> +    }
> +
> +    ff->dir = g_steal_pointer(&dir);
> +    ff->file = g_steal_pointer(&file);
> +}
> +
> +static char *
> +file_fence_get_file(Object *obj, Error **errp)
> +{
> +    FileFence *ff = FILE_FENCE(obj);
> +
> +    if (ff->file) {
> +        return g_build_filename(ff->dir, ff->file, NULL);
> +    }
> +
> +    return NULL;
> +}
> +
> +static void
> +file_fence_instance_finalize(Object *obj)
> +{
> +    FileFence *ff = FILE_FENCE(obj);
> +
> +    ktimer_tear(ff);
> +    qtimer_tear(ff);
> +    watch_tear(ff);
> +
> +    g_free(ff->file);
> +    g_free(ff->dir);
> +}
> +
> +static void
> +file_fence_instance_init(Object *obj)
> +{
> +    FileFence *ff = FILE_FENCE(obj);
> +
> +    object_property_add_str(obj, "file",
> +                            file_fence_get_file,
> +                            file_fence_set_file,
> +                            &error_abort);
> +    object_property_add_str(obj, "signal",
> +                            file_fence_get_signal,
> +                            file_fence_set_signal,
> +                            &error_abort);
> +    object_property_add_uint32_ptr(obj, "qtimeout", &ff->qtimeout,
> +                                   OBJ_PROP_FLAG_READWRITE, &error_abort);
> +    object_property_add_uint32_ptr(obj, "ktimeout", &ff->ktimeout,
> +                                   OBJ_PROP_FLAG_READWRITE, &error_abort);

You could make them all class properties, right?

> +}
> +
> +static void
> +file_fence_class_init(ObjectClass *klass, void *class_data)
> +{
> +    UserCreatableClass *ucc = USER_CREATABLE_CLASS(klass);
> +    ucc->complete = file_fence_complete;
> +}
> +
> +static const TypeInfo file_fence_info = {
> +    .name = TYPE_FILE_FENCE,
> +    .parent = TYPE_OBJECT,
> +    .class_init = file_fence_class_init,
> +    .instance_size = sizeof(FileFence),
> +    .instance_init = file_fence_instance_init,
> +    .instance_finalize = file_fence_instance_finalize,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_USER_CREATABLE },
> +        { }
> +    }
> +};
> +
> +static void
> +register_types(void)
> +{
> +    type_register_static(&file_fence_info);
> +}
> +
> +type_init(register_types);
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 224a8e8712..5ea94b37af 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4974,8 +4974,33 @@ The polling parameters can be modified at run-time 
> using the @code{qom-set} comm
>
>  @end table
>
> -ETEXI
> +@item -object 
> file-fence,id=@var{id},file=@var{file},qtimeout=@var{qtimeout},ktimeout=@var{ktimeout},signal=@{signal}
> +
> +Self-fence Qemu if @var{file} is not modified within a given timeout.
> +
> +Qemu will watch @var{file} for attribute changes. Touching the file works as 
> a
> +heartbeat. This parameter is mandatory.
> +
> +Fencing happens after @var{qtimeout} or @var{ktimeout} seconds elapse
> +without a heartbeat. At least one of these must be specified. Both may be 
> used.
>
> +When using @var{qtimeout}, an internal Qemu timer is used. Fencing with
> +this method gives Qemu a chance to write a log message indicating which file
> +caused the event. If Qemu's main loop is hung for whatever reason, this 
> method
> +won't successfully kill Qemu.
> +
> +When using @var{ktimeout}, a kernel timer is used. In this case, @var{signal}
> +can be 'kill' (for SIGKILL, default) or 'quit' (for SIGQUIT). Using SIGQUIT 
> may
> +be preferred for obtaining core dumps. If Qemu is hung (eg. uninterruptable
> +sleep), this method won't successfully kill Qemu.
> +
> +It is worth noting that even successfully killing Qemu may not be sufficient 
> to
> +completely fence a VM as certain operations like network packets or block
> +commands may be pending in the kernel. If that is a concern, systems should
> +consider using further fencing mechanisms like hardware watchdogs either in
> +addition or in conjunction with this feature for additional protection.
> +
> +ETEXI
>
>  HXCOMM This is the last statement. Insert new options before this line!
>  STEXI
> --
> 2.20.1
>


-- 
Marc-André Lureau