From 3e81a71c9f3d23002b1e0dfff902c155d6c8d224 Mon Sep 17 00:00:00 2001
From: Igor Druzhinin <igor.druzhinin@citrix.com>
Date: Tue, 20 Apr 2021 04:35:02 +0100
Subject: [PATCH 1/3] xen-mapcache: avoid a race on memory map while using
MAP_FIXED
When we're replacing the existing mapping there is possibility of a race
on memory map with other threads doing mmap operations - the address being
unmapped/re-mapped could be occupied by another thread in between.
Linux mmap man page recommends keeping the existing mappings in place to
reserve the place and instead utilize the fact that the next mmap operation
with MAP_FIXED flag passed will implicitly destroy the existing mappings
behind the chosen address. This behavior is guaranteed by POSIX / BSD and
therefore is portable.
Note that it wouldn't make the replacement atomic for parallel accesses to
the replaced region - those might still fail with SIGBUS due to
xenforeignmemory_map not being atomic. So we're still not expecting those.
Tested-by: Anthony PERARD <anthony.perard@citrix.com>
Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
Reviewed-by: Paul Durrant <paul@xen.org>
Message-Id: <1618889702-13104-1-git-send-email-igor.druzhinin@citrix.com>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
hw/i386/xen/xen-mapcache.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index 5b120ed44b..e82b7dcdd2 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -171,7 +171,20 @@ static void xen_remap_bucket(MapCacheEntry *entry,
if (!(entry->flags & XEN_MAPCACHE_ENTRY_DUMMY)) {
ram_block_notify_remove(entry->vaddr_base, entry->size);
}
- if (munmap(entry->vaddr_base, entry->size) != 0) {
+
+ /*
+ * If an entry is being replaced by another mapping and we're using
+ * MAP_FIXED flag for it - there is possibility of a race for vaddr
+ * address with another thread doing an mmap call itself
+ * (see man 2 mmap). To avoid that we skip explicit unmapping here
+ * and allow the kernel to destroy the previous mappings by replacing
+ * them in mmap call later.
+ *
+ * Non-identical replacements are not allowed therefore.
+ */
+ assert(!vaddr || (entry->vaddr_base == vaddr && entry->size == size));
+
+ if (!vaddr && munmap(entry->vaddr_base, entry->size) != 0) {
perror("unmap fails");
exit(-1);
}
From f1e43b6026500690fc402828fa7cc735175b93b6 Mon Sep 17 00:00:00 2001
From: Anthony PERARD <anthony.perard@citrix.com>
Date: Fri, 30 Apr 2021 17:37:42 +0100
Subject: [PATCH 2/3] xen: Free xenforeignmemory_resource at exit
Because Coverity complains about it and this is one leak that Valgrind
reports.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Paul Durrant <paul@xen.org>
Message-Id: <20210430163742.469739-1-anthony.perard@citrix.com>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
hw/i386/xen/xen-hvm.c | 9 ++++++---
include/hw/xen/xen_common.h | 6 ++++++
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index c53fa17c50..9b432773f0 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -108,6 +108,7 @@ typedef struct XenIOState {
shared_iopage_t *shared_page;
shared_vmport_iopage_t *shared_vmport_page;
buffered_iopage_t *buffered_io_page;
+ xenforeignmemory_resource_handle *fres;
QEMUTimer *buffered_io_timer;
CPUState **cpu_by_vcpu_id;
/* the evtchn port for polling the notification, */
@@ -1253,6 +1254,9 @@ static void xen_exit_notifier(Notifier *n, void *data)
XenIOState *state = container_of(n, XenIOState, exit);
xen_destroy_ioreq_server(xen_domid, state->ioservid);
+ if (state->fres != NULL) {
+ xenforeignmemory_unmap_resource(xen_fmem, state->fres);
+ }
xenevtchn_close(state->xce_handle);
xs_daemon_close(state->xenstore);
@@ -1320,7 +1324,6 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
static int xen_map_ioreq_server(XenIOState *state)
{
void *addr = NULL;
- xenforeignmemory_resource_handle *fres;
xen_pfn_t ioreq_pfn;
xen_pfn_t bufioreq_pfn;
evtchn_port_t bufioreq_evtchn;
@@ -1332,12 +1335,12 @@ static int xen_map_ioreq_server(XenIOState *state)
*/
QEMU_BUILD_BUG_ON(XENMEM_resource_ioreq_server_frame_bufioreq != 0);
QEMU_BUILD_BUG_ON(XENMEM_resource_ioreq_server_frame_ioreq(0) != 1);
- fres = xenforeignmemory_map_resource(xen_fmem, xen_domid,
+ state->fres = xenforeignmemory_map_resource(xen_fmem, xen_domid,
XENMEM_resource_ioreq_server,
state->ioservid, 0, 2,
&addr,
PROT_READ | PROT_WRITE, 0);
- if (fres != NULL) {
+ if (state->fres != NULL) {
trace_xen_map_resource_ioreq(state->ioservid, addr);
state->buffered_io_page = addr;
state->shared_page = addr + TARGET_PAGE_SIZE;
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 82e56339dd..a8118b41ac 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -134,6 +134,12 @@ static inline xenforeignmemory_resource_handle *xenforeignmemory_map_resource(
return NULL;
}
+static inline int xenforeignmemory_unmap_resource(
+ xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
+{
+ return 0;
+}
+
#endif /* CONFIG_XEN_CTRL_INTERFACE_VERSION < 41100 */
#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
From 1898293990702c5601e225dac9afd2402fc46e2d Mon Sep 17 00:00:00 2001
From: Anthony PERARD <anthony.perard@citrix.com>
Date: Fri, 30 Apr 2021 17:34:32 +0100
Subject: [PATCH 3/3] xen-block: Use specific blockdev driver
... when a xen-block backend instance is created via xenstore.
Following 8d17adf34f50 ("block: remove support for using "file" driver
with block/char devices"), using the "file" blockdev driver for
everything doesn't work anymore, we need to use the "host_device"
driver when the disk image is a block device and "file" driver when it
is a regular file.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Paul Durrant <paul@xen.org>
Message-Id: <20210430163432.468894-1-anthony.perard@citrix.com>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
hw/block/xen-block.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 83754a4344..674953f1ad 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -728,6 +728,8 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
XenBlockDrive *drive = NULL;
QDict *file_layer;
QDict *driver_layer;
+ struct stat st;
+ int rc;
if (params) {
char **v = g_strsplit(params, ":", 2);
@@ -761,7 +763,17 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
file_layer = qdict_new();
driver_layer = qdict_new();
- qdict_put_str(file_layer, "driver", "file");
+ rc = stat(filename, &st);
+ if (rc) {
+ error_setg_errno(errp, errno, "Could not stat file '%s'", filename);
+ goto done;
+ }
+ if (S_ISBLK(st.st_mode)) {
+ qdict_put_str(file_layer, "driver", "host_device");
+ } else {
+ qdict_put_str(file_layer, "driver", "file");
+ }
+
qdict_put_str(file_layer, "filename", filename);
g_free(filename);