[PULL 0/3] xen queue 2021-05-10

[PULL 0/3] xen queue 2021-05-10

[Anthony PERARD via]

The following changes since commit 4cc10cae64c51e17844dc4358481c393d7bf1ed4:

  Merge remote-tracking branch 'remotes/bonzini-gitlab/tags/for-upstream' into staging (2021-05-06 18:56:17 +0100)

are available in the Git repository at:

  https://xenbits.xen.org/git-http/people/aperard/qemu-dm.git tags/pull-xen-20210510

for you to fetch changes up to 1898293990702c5601e225dac9afd2402fc46e2d:

  xen-block: Use specific blockdev driver (2021-05-10 13:43:58 +0100)

----------------------------------------------------------------
Xen patches

- Avoid mmap race involving Xen's mapcache
- Fix xenforeignmemory_resource leak at exit
- Fix xen-block to choose a driver for the disk image when created via
  xenstore.

----------------------------------------------------------------
Anthony PERARD (2):
      xen: Free xenforeignmemory_resource at exit
      xen-block: Use specific blockdev driver

Igor Druzhinin (1):
      xen-mapcache: avoid a race on memory map while using MAP_FIXED

 hw/block/xen-block.c        | 14 +++++++++++++-
 hw/i386/xen/xen-hvm.c       |  9 ++++++---
 hw/i386/xen/xen-mapcache.c  | 15 ++++++++++++++-
 include/hw/xen/xen_common.h |  6 ++++++
 4 files changed, 39 insertions(+), 5 deletions(-)

[PULL 1/3] xen-mapcache: avoid a race on memory map while using MAP_FIXED

[Anthony PERARD via]

From: Igor Druzhinin <igor.druzhinin@citrix.com>

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);
         }
-- 
Anthony PERARD

[PULL 2/3] xen: Free xenforeignmemory_resource at exit

[Anthony PERARD via]

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
-- 
Anthony PERARD

[PULL 3/3] xen-block: Use specific blockdev driver

[Anthony PERARD via]

... 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);
 
-- 
Anthony PERARD

Re: [PULL 0/3] xen queue 2021-05-10

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210510125340.903323-1-anthony.perard@citrix.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210510125340.903323-1-anthony.perard@citrix.com
Subject: [PULL 0/3] xen queue 2021-05-10

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20210510125340.903323-1-anthony.perard@citrix.com -> patchew/20210510125340.903323-1-anthony.perard@citrix.com
Switched to a new branch 'test'
43f67fa xen-block: Use specific blockdev driver
bf6b992 xen: Free xenforeignmemory_resource at exit
173cc95 xen-mapcache: avoid a race on memory map while using MAP_FIXED

=== OUTPUT BEGIN ===
1/3 Checking commit 173cc95989e6 (xen-mapcache: avoid a race on memory map while using MAP_FIXED)
2/3 Checking commit bf6b9921af27 (xen: Free xenforeignmemory_resource at exit)
ERROR: Author email address is mangled by the mailing list
#2: 
Author: Anthony PERARD via <qemu-devel@nongnu.org>

total: 1 errors, 0 warnings, 49 lines checked

Patch 2/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/3 Checking commit 43f67fa6266d (xen-block: Use specific blockdev driver)
ERROR: Author email address is mangled by the mailing list
#2: 
Author: Anthony PERARD via <qemu-devel@nongnu.org>

total: 1 errors, 0 warnings, 26 lines checked

Patch 3/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210510125340.903323-1-anthony.perard@citrix.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PULL 0/3] xen queue 2021-05-10

[Peter Maydell]

On Mon, 10 May 2021 at 13:53, Anthony PERARD <anthony.perard@citrix.com> wrote:
>
> The following changes since commit 4cc10cae64c51e17844dc4358481c393d7bf1ed4:
>
>   Merge remote-tracking branch 'remotes/bonzini-gitlab/tags/for-upstream' into staging (2021-05-06 18:56:17 +0100)
>
> are available in the Git repository at:
>
>   https://xenbits.xen.org/git-http/people/aperard/qemu-dm.git tags/pull-xen-20210510
>
> for you to fetch changes up to 1898293990702c5601e225dac9afd2402fc46e2d:
>
>   xen-block: Use specific blockdev driver (2021-05-10 13:43:58 +0100)
>
> ----------------------------------------------------------------
> Xen patches
>
> - Avoid mmap race involving Xen's mapcache
> - Fix xenforeignmemory_resource leak at exit
> - Fix xen-block to choose a driver for the disk image when created via
>   xenstore.
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/6.1
for any user-visible changes.

-- PMM

Re: [PULL 3/3] xen-block: Use specific blockdev driver

[Peter Maydell]

On Mon, 10 May 2021 at 13:53, Anthony PERARD <anthony.perard@citrix.com> wrote:
>
> ... 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>

Hi; Coverity points out (CID 1508722) that this introduces a
memory leak in the new error codepath:

> ---
>  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();

You can see here that we allocate file_layer and driver_layer
as new qdict objects...

>
> -    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;

...but here if the stat() fails we will bail out to
the 'done' label, and the code there does not dereference
these qdicts, so they will leak.

The easy fix is to move the two calls to qdict_new() to
below this if() rather than above it.

> +    }
> +    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);

thanks
-- PMM

Re: [PULL 3/3] xen-block: Use specific blockdev driver

[Peter Maydell]

On Fri, 2 Jun 2023 at 18:04, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 10 May 2021 at 13:53, Anthony PERARD <anthony.perard@citrix.com> wrote:
> >
> > ... 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>
>
> Hi; Coverity points out (CID 1508722) that this introduces a
> memory leak in the new error codepath:

I just realized I forgot to cc the current Xen maintainers,
so I'm doing that now. I think the fix for this leak should
be fairly straightforward.

> > ---
> >  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();
>
> You can see here that we allocate file_layer and driver_layer
> as new qdict objects...
>
> >
> > -    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;
>
> ...but here if the stat() fails we will bail out to
> the 'done' label, and the code there does not dereference
> these qdicts, so they will leak.
>
> The easy fix is to move the two calls to qdict_new() to
> below this if() rather than above it.
>
> > +    }
> > +    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);

thanks
-- PMM