Re: [Qemu-devel] [PATCH] qemu-ga: use key-value store to avoid recycling fd handles after restart

[mdroth]

On Wed, Mar 20, 2013 at 10:54:51AM -0400, Luiz Capitulino wrote:
> 
> On Fri,  1 Mar 2013 11:40:27 -0600
> Michael Roth <address@hidden> wrote:
> 
> > Hosts hold on to handles provided by guest-file-open for periods that can
> > span beyond the life of the qemu-ga process that issued them. Since these
> > are issued starting from 0 on every restart, we run the risk of issuing
> > duplicate handles after restarts/reboots.
> > 
> > As a result, users with a stale copy of these handles may end up
> > reading/writing corrupted data due to their existing handles effectively
> > being re-assigned to an unexpected file or offset.
> > 
> > We unfortunately do not issue handles as strings, but as integers, so a
> > solution such as using UUIDs can't be implemented without introducing a
> > new interface.
> > 
> > As a workaround, we fix this by implementing a persistent key-value store
> > that will be used to track the value of the last handle that was issued
> > across restarts/reboots to avoid issuing duplicates.
> > 
> > The store is automatically written to the same directory we currently
> > set via --statedir to track fsfreeze state, and so should be applicable
> > for stable releases where this flag is supported.
> 
> Did you consider that statedir is normally set to /var/run which is
> cleared by some distros on system reboot? Is this OK?

Yes, though it's not ideal. The statedir should be a directory that
persists across reboots to completely fix the problem. But using a
directory that at least persists until reboot will avoid duplicate handles
being issued for qemu-ga restarts. So it's an improvement at least,
which is why I think it's worth considering for stable as well.

> 
> Also, I find it unfortunate that fd_counter never goes down...

The required book-keeping makes it just not worth it, imo, but it could
be done in the future without breaking compatibility (see below)

Not ideal certainly, but I think it's about the best we can do with
integer handles unfortunately.

> 
> One more comment below.
> 
> > 
> > A follow-up can use this same store for handling fsfreeze state, but
> > that change is cosmetic and left out for now.
> > 
> > Signed-off-by: Michael Roth <address@hidden>
> > Cc: address@hidden
> > ---
> >  qga/commands-posix.c   |   25 +++++--
> >  qga/guest-agent-core.h |    1 +
> >  qga/main.c             |  184 
> > ++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 204 insertions(+), 6 deletions(-)
> > 
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 7a0202e..5d12716 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -129,14 +129,22 @@ static struct {
> >      QTAILQ_HEAD(, GuestFileHandle) filehandles;
> >  } guest_file_state;
> >  
> > -static void guest_file_handle_add(FILE *fh)
> > +static uint64_t guest_file_handle_add(FILE *fh, Error **errp)
> >  {
> >      GuestFileHandle *gfh;
> > +    uint64_t handle;
> > +
> > +    handle = ga_get_fd_handle(ga_state, errp);
> > +    if (error_is_set(errp)) {
> > +        return 0;
> > +    }
> >  
> >      gfh = g_malloc0(sizeof(GuestFileHandle));
> > -    gfh->id = fileno(fh);
> > +    gfh->id = handle;
> >      gfh->fh = fh;
> >      QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
> > +
> > +    return handle;
> >  }
> >  
> >  static GuestFileHandle *guest_file_handle_find(int64_t id, Error **err)
> > @@ -158,7 +166,7 @@ int64_t qmp_guest_file_open(const char *path, bool 
> > has_mode, const char *mode, E
> >  {
> >      FILE *fh;
> >      int fd;
> > -    int64_t ret = -1;
> > +    int64_t ret = -1, handle;
> >  
> >      if (!has_mode) {
> >          mode = "r";
> > @@ -184,9 +192,14 @@ int64_t qmp_guest_file_open(const char *path, bool 
> > has_mode, const char *mode, E
> >          return -1;
> >      }
> >  
> > -    guest_file_handle_add(fh);
> > -    slog("guest-file-open, handle: %d", fd);
> > -    return fd;
> > +    handle = guest_file_handle_add(fh, err);
> > +    if (error_is_set(err)) {
> > +        fclose(fh);
> > +        return -1;
> > +    }
> > +
> > +    slog("guest-file-open, handle: %d", handle);
> > +    return handle;
> >  }
> >  
> >  void qmp_guest_file_close(int64_t handle, Error **err)
> > diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> > index 3354598..624a559 100644
> > --- a/qga/guest-agent-core.h
> > +++ b/qga/guest-agent-core.h
> > @@ -35,6 +35,7 @@ bool ga_is_frozen(GAState *s);
> >  void ga_set_frozen(GAState *s);
> >  void ga_unset_frozen(GAState *s);
> >  const char *ga_fsfreeze_hook(GAState *s);
> > +int64_t ga_get_fd_handle(GAState *s, Error **errp);
> >  
> >  #ifndef _WIN32
> >  void reopen_fd_to_null(int fd);
> > diff --git a/qga/main.c b/qga/main.c
> > index db281a5..3635430 100644
> > --- a/qga/main.c
> > +++ b/qga/main.c
> > @@ -15,6 +15,7 @@
> >  #include <stdbool.h>
> >  #include <glib.h>
> >  #include <getopt.h>
> > +#include <glib/gstdio.h>
> >  #ifndef _WIN32
> >  #include <syslog.h>
> >  #include <sys/wait.h>
> > @@ -30,6 +31,7 @@
> >  #include "qapi/qmp/qerror.h"
> >  #include "qapi/qmp/dispatch.h"
> >  #include "qga/channel.h"
> > +#include "qemu/bswap.h"
> >  #ifdef _WIN32
> >  #include "qga/service-win32.h"
> >  #include <windows.h>
> > @@ -53,6 +55,11 @@
> >  #endif
> >  #define QGA_SENTINEL_BYTE 0xFF
> >  
> > +typedef struct GAPersistentState {
> > +#define QGA_PSTATE_DEFAULT_FD_COUNTER 1000
> > +    int64_t fd_counter;
> > +} GAPersistentState;
> > +
> >  struct GAState {
> >      JSONMessageParser parser;
> >      GMainLoop *main_loop;
> > @@ -76,6 +83,8 @@ struct GAState {
> >  #ifdef CONFIG_FSFREEZE
> >      const char *fsfreeze_hook;
> >  #endif
> > +    const gchar *pstate_filepath;
> > +    GAPersistentState pstate;
> >  };
> >  
> >  struct GAState *ga_state;
> > @@ -724,6 +733,171 @@ VOID WINAPI service_main(DWORD argc, TCHAR *argv[])
> >  }
> >  #endif
> >  
> > +static void set_persistent_state_defaults(GAPersistentState *pstate)
> > +{
> > +    g_assert(pstate);
> > +    pstate->fd_counter = QGA_PSTATE_DEFAULT_FD_COUNTER;
> > +}
> > +
> > +static void persistent_state_from_keyfile(GAPersistentState *pstate,
> > +                                          GKeyFile *keyfile)
> > +{
> > +    g_assert(pstate);
> > +    g_assert(keyfile);
> > +    /* if any fields are missing, either because the file was tampered with
> > +     * by agents of chaos, or because the field wasn't present at the time 
> > the
> > +     * file was created, the best we can ever do is start over with the 
> > default
> > +     * values. so load them now, and ignore any errors in accessing 
> > key-value
> > +     * pairs
> > +     */
> > +    set_persistent_state_defaults(pstate);
> > +
> > +    if (g_key_file_has_key(keyfile, "global", "fd_counter", NULL)) {
> > +        pstate->fd_counter =
> > +            g_key_file_get_int64(keyfile, "global", "fd_counter", NULL);
> > +    }
> > +}
> > +
> > +static void persistent_state_to_keyfile(const GAPersistentState *pstate,
> > +                                        GKeyFile *keyfile)
> > +{
> > +    g_assert(pstate);
> > +    g_assert(keyfile);
> > +
> > +    g_key_file_set_int64(keyfile, "global", "fd_counter", 
> > pstate->fd_counter);
> > +}
> > +
> > +static gboolean write_persistent_state(const GAPersistentState *pstate,
> > +                                       const gchar *path)
> > +{
> > +    GKeyFile *keyfile = g_key_file_new();
> > +    GError *gerr = NULL;
> > +    gboolean ret = true;
> > +    gchar *data = NULL;
> > +    gsize data_len;
> > +
> > +    g_assert(pstate);
> > +
> > +    persistent_state_to_keyfile(pstate, keyfile);
> > +    data = g_key_file_to_data(keyfile, &data_len, &gerr);
> > +    if (gerr) {
> > +        g_critical("failed to convert persistent state to string: %s",
> > +                   gerr->message);
> > +        ret = false;
> > +        goto out;
> > +    }
> > +
> > +    g_file_set_contents(path, data, data_len, &gerr);
> > +    if (gerr) {
> > +        g_critical("failed to write persistent state to %s: %s",
> > +                    path, gerr->message);
> > +        ret = false;
> > +        goto out;
> > +    }
> > +
> > +out:
> > +    if (gerr) {
> > +        g_error_free(gerr);
> > +    }
> > +    if (keyfile) {
> > +        g_key_file_free(keyfile);
> > +    }
> > +    g_free(data);
> > +    return ret;
> > +}
> > +
> > +static gboolean read_persistent_state(GAPersistentState *pstate,
> > +                                      const gchar *path, gboolean frozen)
> > +{
> > +    GKeyFile *keyfile = NULL;
> > +    GError *gerr = NULL;
> > +    struct stat st;
> > +    gboolean ret = true;
> > +
> > +    g_assert(pstate);
> > +
> > +    if (stat(path, &st) == -1) {
> > +        /* it's okay if state file doesn't exist, but any other error
> > +         * indicates a permissions issue or some other misconfiguration
> > +         * that we likely won't be able to recover from.
> > +         */
> > +        if (errno != ENOENT) {
> > +            g_critical("unable to access state file at path %s: %s",
> > +                       path, strerror(errno));
> > +            ret = false;
> > +            goto out;
> > +        }
> > +
> > +        /* file doesn't exist. initialize state to default values and
> > +         * attempt to save now. (we could wait till later when we have
> > +         * modified state we need to commit, but if there's a problem,
> > +         * such as a missing parent directory, we want to catch it now)
> > +         *
> > +         * there is a potential scenario where someone either managed to
> > +         * update the agent from a version that didn't use a key store
> > +         * while qemu-ga thought the filesystem was frozen, or
> > +         * deleted the key store prior to issuing a fsfreeze, prior
> > +         * to restarting the agent. in this case we go ahead and defer
> > +         * initial creation till we actually have modified state to
> > +         * write, otherwise fail to recover from freeze.
> > +         */
> > +        set_persistent_state_defaults(pstate);
> > +        if (!frozen) {
> > +            ret = write_persistent_state(pstate, path);
> > +            if (!ret) {
> > +                g_critical("unable to create state file at path %s", path);
> > +                ret = false;
> > +                goto out;
> > +            }
> > +        }
> > +        ret = true;
> > +        goto out;
> > +    }
> > +
> > +    keyfile = g_key_file_new();
> > +    g_key_file_load_from_file(keyfile, path, 0, &gerr);
> > +    if (gerr) {
> > +        g_critical("error loading persistent state from path: %s, %s",
> > +                   path, gerr->message);
> > +        ret = false;
> > +        goto out;
> > +    }
> > +
> > +    persistent_state_from_keyfile(pstate, keyfile);
> > +
> > +out:
> > +    if (keyfile) {
> > +        g_key_file_free(keyfile);
> > +    }
> > +    if (gerr) {
> > +        g_error_free(gerr);
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +int64_t ga_get_fd_handle(GAState *s, Error **errp)
> > +{
> > +    int64_t handle;
> > +
> > +    g_assert(s->pstate_filepath);
> > +    /* we blacklist commands and avoid operations that potentially require
> > +     * writing to disk when we're in a frozen state. this includes opening
> > +     * new files, so we should never get here in that situation
> > +     */
> > +    g_assert(!ga_is_frozen(s));
> > +
> > +    handle = s->pstate.fd_counter++;
> > +    if (s->pstate.fd_counter < 0) {
> > +        s->pstate.fd_counter = 0;
> > +    }
> 
> Is this handling the overflow case? Can't fd 0 be in use already?

It could, but it's very unlikely that an overflow/counter reset would
result in issuing still-in-use handle, since guest-file-open would need
to be called 2^63 times in the meantime.

Not impossible though I guess, but not really something we can
avoid without maintaining a list of all outstanding handles that
persists across reboot. That would allow us to make a nice statement
like "only x outstanding handles supported at a time", but that's
potentially a lot of book-keeping for each guest-file-open. I think it
can be implemented using the same key store, we'd just introduce a new
key via g_key_file_get_integer_list/g_key_file_set_integer_list and give
it priority over the fd_counter key for issuing handles if it's present.

So it's potentially doable, I'm just not sure it's worth it atm.

> 
> I'd check against fd_counter max value and return an error if that's reached.

It's meant to be a cyclical counter, otherwise we'd hit a point where
qemu-ga just stopped working forever. In retrospect, "fd_counter" was a
poor name for the field, "next_fd_handle" is really what it is.

Also, welcome back! :)

> 
> > +    if (!write_persistent_state(&s->pstate, s->pstate_filepath)) {
> > +        error_setg(errp, "failed to commit persistent state to disk");
> > +    }
> > +
> > +    return handle;
> > +}
> > +
> >  int main(int argc, char **argv)
> >  {
> >      const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
> > @@ -853,7 +1027,9 @@ int main(int argc, char **argv)
> >      ga_enable_logging(s);
> >      s->state_filepath_isfrozen = g_strdup_printf("%s/qga.state.isfrozen",
> >                                                   state_dir);
> > +    s->pstate_filepath = g_strdup_printf("%s/qga.state", state_dir);
> >      s->frozen = false;
> > +
> >  #ifndef _WIN32
> >      /* check if a previous instance of qemu-ga exited with filesystems' 
> > state
> >       * marked as frozen. this could be a stale value (a non-qemu-ga process
> > @@ -910,6 +1086,14 @@ int main(int argc, char **argv)
> >          }
> >      }
> >  
> > +    /* load persistent state from disk */
> > +    if (!read_persistent_state(&s->pstate,
> > +                               s->pstate_filepath,
> > +                               ga_is_frozen(s))) {
> > +        g_critical("failed to load persistent state");
> > +        goto out_bad;
> > +    }
> > +
> >      if (blacklist) {
> >          s->blacklist = blacklist;
> >          do {
>