Daniel P. Berrangé <berra...@redhat.com> 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é <berra...@redhat.com> > --- > > Depends: <20190314151527.25533-1-berra...@redhat.com> >
Reviewed-by: Bandan Das <b...@redhat.com> > 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"
