Re: [Qemu-devel] [PATCH] filemon: fix watch IDs to avoid potential wraparound issues

[Bandan Das]

Daniel P. Berrangé <address@hidden> writes:

> Watch IDs are allocated from incrementing a int counter against
> the QFileMonitor object. In very long life QEMU processes with
> a huge amount of USB MTP activity creating & deleting directories
> it is just about conceivable that the int counter can wrap
> around. This would result in incorrect behaviour of the file
> monitor watch APIs due to clashing watch IDs.
>
> Instead of trying to detect this situation, this patch changes
> the way watch IDs are allocated. It is turned into an int64_t
> variable where the high 32 bits are set from the underlying
> inotify "int" ID. This gives an ID that is guaranteed unique
> for the directory as a whole, and we can rely on the kernel
> to enforce this. QFileMonitor then sets the low 32 bits from
> a per-directory counter.
>
> The USB MTP device only sets watches on the directory as a
> whole, not files within, so there is no risk of guest
> triggered wrap around on the low 32 bits.
>
> Signed-off-by: Daniel P. Berrangé <address@hidden>
> ---
>
> Depends: <address@hidden>
>

Reviewed-by: Bandan Das <address@hidden>

> This is an issue raised in
>
>   https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg05285.html
>
>  authz/listfile.c           |  2 +-
>  hw/usb/dev-mtp.c           | 10 +++++-----
>  include/authz/listfile.h   |  2 +-
>  include/qemu/filemonitor.h | 16 ++++++++--------
>  util/filemonitor-inotify.c | 25 +++++++++++++------------
>  util/filemonitor-stub.c    |  4 ++--
>  util/trace-events          |  6 +++---
>  7 files changed, 33 insertions(+), 32 deletions(-)
>
> diff --git a/authz/listfile.c b/authz/listfile.c
> index d4579767e7..bc2b58ef6d 100644
> --- a/authz/listfile.c
> +++ b/authz/listfile.c
> @@ -93,7 +93,7 @@ qauthz_list_file_load(QAuthZListFile *fauthz, Error **errp)
>  
>  
>  static void
> -qauthz_list_file_event(int wd G_GNUC_UNUSED,
> +qauthz_list_file_event(int64_t wd G_GNUC_UNUSED,
>                         QFileMonitorEvent ev G_GNUC_UNUSED,
>                         const char *name G_GNUC_UNUSED,
>                         void *opaque)
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 06e376bcd2..0cce3f53fe 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -170,7 +170,7 @@ struct MTPObject {
>      char         *path;
>      struct stat  stat;
>      /* file monitor watch id */
> -    int          watchid;
> +    int64_t      watchid;
>      MTPObject    *parent;
>      uint32_t     nchildren;
>      QLIST_HEAD(, MTPObject) children;
> @@ -498,7 +498,7 @@ static MTPObject *usb_mtp_object_lookup_name(MTPObject 
> *parent,
>      return NULL;
>  }
>  
> -static MTPObject *usb_mtp_object_lookup_id(MTPState *s, int id)
> +static MTPObject *usb_mtp_object_lookup_id(MTPState *s, int64_t id)
>  {
>      MTPObject *iter;
>  
> @@ -511,7 +511,7 @@ static MTPObject *usb_mtp_object_lookup_id(MTPState *s, 
> int id)
>      return NULL;
>  }
>  
> -static void file_monitor_event(int id,
> +static void file_monitor_event(int64_t id,
>                                 QFileMonitorEvent ev,
>                                 const char *name,
>                                 void *opaque)
> @@ -625,8 +625,8 @@ static void usb_mtp_object_readdir(MTPState *s, MTPObject 
> *o)
>      }
>  
>      if (s->file_monitor) {
> -        int id = qemu_file_monitor_add_watch(s->file_monitor, o->path, NULL,
> -                                             file_monitor_event, s, &err);
> +        int64_t id = qemu_file_monitor_add_watch(s->file_monitor, o->path, 
> NULL,
> +                                                 file_monitor_event, s, 
> &err);
>          if (id == -1) {
>              error_report("usb-mtp: failed to add watch for %s: %s", o->path,
>                           error_get_pretty(err));
> diff --git a/include/authz/listfile.h b/include/authz/listfile.h
> index bcc8d80743..9118a703ec 100644
> --- a/include/authz/listfile.h
> +++ b/include/authz/listfile.h
> @@ -92,7 +92,7 @@ struct QAuthZListFile {
>      char *filename;
>      bool refresh;
>      QFileMonitor *file_monitor;
> -    int file_watch;
> +    int64_t file_watch;
>  };
>  
>  
> diff --git a/include/qemu/filemonitor.h b/include/qemu/filemonitor.h
> index cd031832ed..64267d09b2 100644
> --- a/include/qemu/filemonitor.h
> +++ b/include/qemu/filemonitor.h
> @@ -52,7 +52,7 @@ typedef enum {
>   * empty.
>   *
>   */
> -typedef void (*QFileMonitorHandler)(int id,
> +typedef void (*QFileMonitorHandler)(int64_t id,
>                                      QFileMonitorEvent event,
>                                      const char *filename,
>                                      void *opaque);
> @@ -103,12 +103,12 @@ void qemu_file_monitor_free(QFileMonitor *mon);
>   *
>   * Returns: a positive integer watch ID, or -1 on error
>   */
> -int qemu_file_monitor_add_watch(QFileMonitor *mon,
> -                                const char *dirpath,
> -                                const char *filename,
> -                                QFileMonitorHandler cb,
> -                                void *opaque,
> -                                Error **errp);
> +int64_t qemu_file_monitor_add_watch(QFileMonitor *mon,
> +                                    const char *dirpath,
> +                                    const char *filename,
> +                                    QFileMonitorHandler cb,
> +                                    void *opaque,
> +                                    Error **errp);
>  
>  /**
>   * qemu_file_monitor_remove_watch:
> @@ -123,6 +123,6 @@ int qemu_file_monitor_add_watch(QFileMonitor *mon,
>   */
>  void qemu_file_monitor_remove_watch(QFileMonitor *mon,
>                                      const char *dirpath,
> -                                    int id);
> +                                    int64_t id);
>  
>  #endif /* QEMU_FILE_MONITOR_H */
> diff --git a/util/filemonitor-inotify.c b/util/filemonitor-inotify.c
> index 3eb29f860b..b5f4b93f3f 100644
> --- a/util/filemonitor-inotify.c
> +++ b/util/filemonitor-inotify.c
> @@ -29,7 +29,6 @@
>  
>  struct QFileMonitor {
>      int fd;
> -    int nextid; /* watch ID counter */
>      QemuMutex lock; /* protects dirs & idmap */
>      GHashTable *dirs; /* dirname => QFileMonitorDir */
>      GHashTable *idmap; /* inotify ID => dirname */
> @@ -37,7 +36,7 @@ struct QFileMonitor {
>  
>  
>  typedef struct {
> -    int id; /* watch ID */
> +    int64_t id; /* watch ID */
>      char *filename; /* optional filter */
>      QFileMonitorHandler cb;
>      void *opaque;
> @@ -46,7 +45,8 @@ typedef struct {
>  
>  typedef struct {
>      char *path;
> -    int id; /* inotify ID */
> +    int inotify_id; /* inotify ID */
> +    int next_file_id; /* file ID counter */
>      GArray *watches; /* QFileMonitorWatch elements */
>  } QFileMonitorDir;
>  
> @@ -126,7 +126,8 @@ static void qemu_file_monitor_watch(void *arg)
>              g_assert_not_reached();
>          }
>  
> -        trace_qemu_file_monitor_event(mon, dir->path, name, ev->mask, 
> dir->id);
> +        trace_qemu_file_monitor_event(mon, dir->path, name, ev->mask,
> +                                      dir->inotify_id);
>          for (i = 0; i < dir->watches->len; i++) {
>              QFileMonitorWatch *watch = &g_array_index(dir->watches,
>                                                        QFileMonitorWatch,
> @@ -237,7 +238,7 @@ qemu_file_monitor_free(QFileMonitor *mon)
>      g_idle_add((GSourceFunc)qemu_file_monitor_free_idle, mon);
>  }
>  
> -int
> +int64_t
>  qemu_file_monitor_add_watch(QFileMonitor *mon,
>                              const char *dirpath,
>                              const char *filename,
> @@ -247,7 +248,7 @@ qemu_file_monitor_add_watch(QFileMonitor *mon,
>  {
>      QFileMonitorDir *dir;
>      QFileMonitorWatch watch;
> -    int ret = -1;
> +    int64_t ret = -1;
>  
>      qemu_mutex_lock(&mon->lock);
>      dir = g_hash_table_lookup(mon->dirs, dirpath);
> @@ -265,7 +266,7 @@ qemu_file_monitor_add_watch(QFileMonitor *mon,
>  
>          dir = g_new0(QFileMonitorDir, 1);
>          dir->path = g_strdup(dirpath);
> -        dir->id = rv;
> +        dir->inotify_id = rv;
>          dir->watches = g_array_new(FALSE, TRUE, sizeof(QFileMonitorWatch));
>  
>          g_hash_table_insert(mon->dirs, dir->path, dir);
> @@ -276,7 +277,7 @@ qemu_file_monitor_add_watch(QFileMonitor *mon,
>          }
>      }
>  
> -    watch.id = mon->nextid++;
> +    watch.id = (((int64_t)dir->inotify_id) << 32) | dir->next_file_id++;
>      watch.filename = g_strdup(filename);
>      watch.cb = cb;
>      watch.opaque = opaque;
> @@ -297,7 +298,7 @@ qemu_file_monitor_add_watch(QFileMonitor *mon,
>  
>  void qemu_file_monitor_remove_watch(QFileMonitor *mon,
>                                      const char *dirpath,
> -                                    int id)
> +                                    int64_t id)
>  {
>      QFileMonitorDir *dir;
>      gsize i;
> @@ -322,10 +323,10 @@ void qemu_file_monitor_remove_watch(QFileMonitor *mon,
>      }
>  
>      if (dir->watches->len == 0) {
> -        inotify_rm_watch(mon->fd, dir->id);
> -        trace_qemu_file_monitor_disable_watch(mon, dir->path, dir->id);
> +        inotify_rm_watch(mon->fd, dir->inotify_id);
> +        trace_qemu_file_monitor_disable_watch(mon, dir->path, 
> dir->inotify_id);
>  
> -        g_hash_table_remove(mon->idmap, GINT_TO_POINTER(dir->id));
> +        g_hash_table_remove(mon->idmap, GINT_TO_POINTER(dir->inotify_id));
>          g_hash_table_remove(mon->dirs, dir->path);
>  
>          if (g_hash_table_size(mon->dirs) == 0) {
> diff --git a/util/filemonitor-stub.c b/util/filemonitor-stub.c
> index 48268b2bb6..2c0e97edd8 100644
> --- a/util/filemonitor-stub.c
> +++ b/util/filemonitor-stub.c
> @@ -38,7 +38,7 @@ qemu_file_monitor_free(QFileMonitor *mon G_GNUC_UNUSED)
>  }
>  
>  
> -int
> +int64_t
>  qemu_file_monitor_add_watch(QFileMonitor *mon G_GNUC_UNUSED,
>                              const char *dirpath G_GNUC_UNUSED,
>                              const char *filename G_GNUC_UNUSED,
> @@ -54,6 +54,6 @@ qemu_file_monitor_add_watch(QFileMonitor *mon G_GNUC_UNUSED,
>  void
>  qemu_file_monitor_remove_watch(QFileMonitor *mon G_GNUC_UNUSED,
>                                 const char *dirpath G_GNUC_UNUSED,
> -                               int id G_GNUC_UNUSED)
> +                               int64_t id G_GNUC_UNUSED)
>  {
>  }
> diff --git a/util/trace-events b/util/trace-events
> index ff19b253e2..8d22e16ce1 100644
> --- a/util/trace-events
> +++ b/util/trace-events
> @@ -22,13 +22,13 @@ buffer_move(const char *buf, size_t len, const char 
> *from) "%s: %zd bytes from %
>  buffer_free(const char *buf, size_t len) "%s: capacity %zd"
>  
>  # util/filemonitor.c
> -qemu_file_monitor_add_watch(void *mon, const char *dirpath, const char 
> *filename, void *cb, void *opaque, int id) "File monitor %p add watch 
> dir='%s' file='%s' cb=%p opaque=%p id=%u"
> -qemu_file_monitor_remove_watch(void *mon, const char *dirpath, int id) "File 
> monitor %p remove watch dir='%s' id=%u"
> +qemu_file_monitor_add_watch(void *mon, const char *dirpath, const char 
> *filename, void *cb, void *opaque, int64_t id) "File monitor %p add watch 
> dir='%s' file='%s' cb=%p opaque=%p id=%" PRId64
> +qemu_file_monitor_remove_watch(void *mon, const char *dirpath, int64_t id) 
> "File monitor %p remove watch dir='%s' id=%" PRId64
>  qemu_file_monitor_new(void *mon, int fd) "File monitor %p created fd=%d"
>  qemu_file_monitor_enable_watch(void *mon, const char *dirpath, int id) "File 
> monitor %p enable watch dir='%s' id=%u"
>  qemu_file_monitor_disable_watch(void *mon, const char *dirpath, int id) "Fle 
> monitor %p disable watch dir='%s' id=%u"
>  qemu_file_monitor_event(void *mon, const char *dirpath, const char 
> *filename, int mask, unsigned int id) "File monitor %p event dir='%s' 
> file='%s' mask=0x%x id=%u"
> -qemu_file_monitor_dispatch(void *mon, const char *dirpath, const char 
> *filename, int ev, void *cb, void *opaque, unsigned int id) "File monitor %p 
> dispatch dir='%s' file='%s' ev=%d cb=%p opaque=%p id=%u"
> +qemu_file_monitor_dispatch(void *mon, const char *dirpath, const char 
> *filename, int ev, void *cb, void *opaque, int64_t id) "File monitor %p 
> dispatch dir='%s' file='%s' ev=%d cb=%p opaque=%p id=%" PRId64
>  
>  # util/qemu-coroutine.c
>  qemu_aio_coroutine_enter(void *ctx, void *from, void *to, void *opaque) "ctx 
> %p from %p to %p opaque %p"