[Qemu-devel] [PULL 0/5] virtio,vhost: fixes

[Qemu-devel] [PULL 0/5] virtio,vhost: fixes

[Michael S. Tsirkin]

The following changes since commit 0e87fdc966d05f4e5ad868034fcd8ee2a08ca62d:

  Update version for v2.12.0-rc2 release (2018-04-04 20:37:20 +0100)

are available in the git repository at:

  git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream

for you to fetch changes up to d434e5ac5d70e9da7d20e50246af9251a125bdad:

  virtio-serial: fix heap-over-flow (2018-04-09 17:35:46 +0300)

----------------------------------------------------------------
virtio,vhost: fixes

Add a feature flag for new protocol messages.
Misc fixes.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

----------------------------------------------------------------
Changpeng Liu (1):
      contrib/libvhost-user: add the protocol feature used for SET/GET message

Dr. David Alan Gilbert (1):
      vhost: Allow adjoining regions

Maxime Coquelin (2):
      vhost-user-blk: set config ops before vhost-user init
      vhost-user: back SET/GET_CONFIG requests with a protocol feature

linzhecheng (1):
      virtio-serial: fix heap-over-flow

 docs/interop/vhost-user.txt           | 21 ++++++++++++---------
 contrib/libvhost-user/libvhost-user.h |  1 +
 hw/block/vhost-user-blk.c             |  4 ++--
 hw/char/virtio-serial-bus.c           |  7 +++++--
 hw/virtio/vhost-user.c                | 22 ++++++++++++++++++++++
 hw/virtio/vhost.c                     | 14 +++++++++-----
 6 files changed, 51 insertions(+), 18 deletions(-)

[Qemu-devel] [PULL 1/5] vhost-user-blk: set config ops before vhost-user init

[Michael S. Tsirkin]

From: Maxime Coquelin <maxime.coquelin@redhat.com>

As soon as vhost-user init is done, the backend may send
VHOST_USER_SLAVE_CONFIG_CHANGE_MSG, so let's set the
notification callback before it.

Also, it will be used to know whether the device supports
the config feature to advertize it or not.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Changpeng Liu <changpeng.liu@intel.com>
---
 hw/block/vhost-user-blk.c | 4 ++--
 hw/virtio/vhost.c         | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index f840f07..262baca 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -259,6 +259,8 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
     s->dev.vq_index = 0;
     s->dev.backend_features = 0;
 
+    vhost_dev_set_config_notifier(&s->dev, &blk_ops);
+
     ret = vhost_dev_init(&s->dev, &s->chardev, VHOST_BACKEND_TYPE_USER, 0);
     if (ret < 0) {
         error_setg(errp, "vhost-user-blk: vhost initialization failed: %s",
@@ -277,8 +279,6 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
         s->blkcfg.num_queues = s->num_queues;
     }
 
-    vhost_dev_set_config_notifier(&s->dev, &blk_ops);
-
     return;
 
 vhost_err:
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 250f886..b6c314e 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1451,7 +1451,6 @@ int vhost_dev_set_config(struct vhost_dev *hdev, const uint8_t *data,
 void vhost_dev_set_config_notifier(struct vhost_dev *hdev,
                                    const VhostDevConfigOps *ops)
 {
-    assert(hdev->vhost_ops);
     hdev->config_ops = ops;
 }
 
-- 
MST

[Qemu-devel] [PULL 2/5] vhost-user: back SET/GET_CONFIG requests with a protocol feature

[Michael S. Tsirkin]

From: Maxime Coquelin <maxime.coquelin@redhat.com>

Without a dedicated protocol feature, QEMU cannot know whether
the backend can handle VHOST_USER_SET_CONFIG and
VHOST_USER_GET_CONFIG messages.

This patch adds a protocol feature that is only advertised by
QEMU if the device implements the config ops. Vhost user init
fails if the device support the feature but the backend doesn't.

The backend should only send VHOST_USER_SLAVE_CONFIG_CHANGE_MSG
requests if the protocol feature has been negotiated.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Changpeng Liu <changpeng.liu@intel.com>
---
 docs/interop/vhost-user.txt | 21 ++++++++++++---------
 hw/virtio/vhost-user.c      | 22 ++++++++++++++++++++++
 2 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
index c058c40..534caab 100644
--- a/docs/interop/vhost-user.txt
+++ b/docs/interop/vhost-user.txt
@@ -379,6 +379,7 @@ Protocol features
 #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN   6
 #define VHOST_USER_PROTOCOL_F_CRYPTO_SESSION 7
 #define VHOST_USER_PROTOCOL_F_PAGEFAULT      8
+#define VHOST_USER_PROTOCOL_F_CONFIG         9
 
 Master message types
 --------------------
@@ -664,7 +665,8 @@ Master message types
       Master payload: virtio device config space
       Slave payload: virtio device config space
 
-      Submitted by the vhost-user master to fetch the contents of the virtio
+      When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, this message is
+      submitted by the vhost-user master to fetch the contents of the virtio
       device configuration space, vhost-user slave's payload size MUST match
       master's request, vhost-user slave uses zero length of payload to
       indicate an error to vhost-user master. The vhost-user master may
@@ -677,7 +679,8 @@ Master message types
       Master payload: virtio device config space
       Slave payload: N/A
 
-      Submitted by the vhost-user master when the Guest changes the virtio
+      When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, this message is
+      submitted by the vhost-user master when the Guest changes the virtio
       device configuration space and also can be used for live migration
       on the destination host. The vhost-user slave must check the flags
       field, and slaves MUST NOT accept SET_CONFIG for read-only
@@ -766,13 +769,13 @@ Slave message types
      Slave payload: N/A
      Master payload: N/A
 
-     Vhost-user slave sends such messages to notify that the virtio device's
-     configuration space has changed, for those host devices which can support
-     such feature, host driver can send VHOST_USER_GET_CONFIG message to slave
-     to get the latest content. If VHOST_USER_PROTOCOL_F_REPLY_ACK is
-     negotiated, and slave set the VHOST_USER_NEED_REPLY flag, master must
-     respond with zero when operation is successfully completed, or non-zero
-     otherwise.
+     When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, vhost-user slave sends
+     such messages to notify that the virtio device's configuration space has
+     changed, for those host devices which can support such feature, host
+     driver can send VHOST_USER_GET_CONFIG message to slave to get the latest
+     content. If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, and slave set
+     the VHOST_USER_NEED_REPLY flag, master must respond with zero when
+     operation is successfully completed, or non-zero otherwise.
 
 VHOST_USER_PROTOCOL_F_REPLY_ACK:
 -------------------------------
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 44aea5c..38da869 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -46,6 +46,7 @@ enum VhostUserProtocolFeature {
     VHOST_USER_PROTOCOL_F_CROSS_ENDIAN = 6,
     VHOST_USER_PROTOCOL_F_CRYPTO_SESSION = 7,
     VHOST_USER_PROTOCOL_F_PAGEFAULT = 8,
+    VHOST_USER_PROTOCOL_F_CONFIG = 9,
     VHOST_USER_PROTOCOL_F_MAX
 };
 
@@ -1211,6 +1212,17 @@ static int vhost_user_init(struct vhost_dev *dev, void *opaque)
 
         dev->protocol_features =
             protocol_features & VHOST_USER_PROTOCOL_FEATURE_MASK;
+
+        if (!dev->config_ops || !dev->config_ops->vhost_dev_config_notifier) {
+            /* Don't acknowledge CONFIG feature if device doesn't support it */
+            dev->protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_CONFIG);
+        } else if (!(protocol_features &
+                    (1ULL << VHOST_USER_PROTOCOL_F_CONFIG))) {
+            error_report("Device expects VHOST_USER_PROTOCOL_F_CONFIG "
+                    "but backend does not support it.");
+            return -1;
+        }
+
         err = vhost_user_set_protocol_features(dev, dev->protocol_features);
         if (err < 0) {
             return err;
@@ -1405,6 +1417,11 @@ static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
         .hdr.size = VHOST_USER_CONFIG_HDR_SIZE + config_len,
     };
 
+    if (!virtio_has_feature(dev->protocol_features,
+                VHOST_USER_PROTOCOL_F_CONFIG)) {
+        return -1;
+    }
+
     if (config_len > VHOST_USER_MAX_CONFIG_SIZE) {
         return -1;
     }
@@ -1448,6 +1465,11 @@ static int vhost_user_set_config(struct vhost_dev *dev, const uint8_t *data,
         .hdr.size = VHOST_USER_CONFIG_HDR_SIZE + size,
     };
 
+    if (!virtio_has_feature(dev->protocol_features,
+                VHOST_USER_PROTOCOL_F_CONFIG)) {
+        return -1;
+    }
+
     if (reply_supported) {
         msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
     }
-- 
MST

[Qemu-devel] [PULL 3/5] contrib/libvhost-user: add the protocol feature used for SET/GET message

[Michael S. Tsirkin]

From: Changpeng Liu <changpeng.liu@intel.com>

Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 contrib/libvhost-user/libvhost-user.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/contrib/libvhost-user/libvhost-user.h b/contrib/libvhost-user/libvhost-user.h
index 79f7a53..b27075e 100644
--- a/contrib/libvhost-user/libvhost-user.h
+++ b/contrib/libvhost-user/libvhost-user.h
@@ -50,6 +50,7 @@ enum VhostUserProtocolFeature {
     VHOST_USER_PROTOCOL_F_CROSS_ENDIAN = 6,
     VHOST_USER_PROTOCOL_F_CRYPTO_SESSION = 7,
     VHOST_USER_PROTOCOL_F_PAGEFAULT = 8,
+    VHOST_USER_PROTOCOL_F_CONFIG = 9,
 
     VHOST_USER_PROTOCOL_F_MAX
 };
-- 
MST

[Qemu-devel] [PULL 4/5] vhost: Allow adjoining regions

[Michael S. Tsirkin]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

My rework of section adding combines overlapping or adjoining regions,
but checks they're actually the same underlying RAM block.
Fix the case where two blocks adjoin but don't overlap; that new region
should get added (but not combined), but my previous patch was disallowing it.

Fixes: c1ece84e7c9

Reported-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Tested-by: Alex Williamson <alex.williamson@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/vhost.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index b6c314e..a21a5a2 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -595,10 +595,15 @@ static void vhost_region_add_section(struct vhost_dev *dev,
                                         prev_sec->offset_within_address_space,
                                         prev_sec->offset_within_region);
             } else {
-                error_report("%s: Overlapping but not coherent sections "
-                             "at %"PRIx64,
-                             __func__, mrs_gpa);
-                return;
+                /* adjoining regions are fine, but overlapping ones with
+                 * different blocks/offsets shouldn't happen
+                 */
+                if (mrs_gpa != prev_gpa_end + 1) {
+                    error_report("%s: Overlapping but not coherent sections "
+                                 "at %"PRIx64,
+                                 __func__, mrs_gpa);
+                    return;
+                }
             }
         }
     }
-- 
MST

[Qemu-devel] [PULL 5/5] virtio-serial: fix heap-over-flow

[Michael S. Tsirkin]

From: linzhecheng <linzhecheng@huawei.com>

Check device having the feature of VIRTIO_CONSOLE_F_EMERG_WRITE before
get config->emerg_wr. It is neccessary because sizeof(virtio_console_config)
is 8 byte if VirtIOSerial doesn't have the feature of
VIRTIO_CONSOLE_F_EMERG_WRITE(see virtio_serial_device_realize),
read/write emerg_wr will lead to heap-over-flow.

Signed-off-by: linzhecheng <linzhecheng@huawei.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/char/virtio-serial-bus.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 9470bd7..d2dd8ab 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -580,13 +580,16 @@ static void set_config(VirtIODevice *vdev, const uint8_t *config_data)
     VirtIOSerial *vser = VIRTIO_SERIAL(vdev);
     struct virtio_console_config *config =
         (struct virtio_console_config *)config_data;
-    uint8_t emerg_wr_lo = le32_to_cpu(config->emerg_wr);
     VirtIOSerialPort *port = find_first_connected_console(vser);
     VirtIOSerialPortClass *vsc;
+    uint8_t emerg_wr_lo;
 
-    if (!config->emerg_wr) {
+    if (!virtio_has_feature(vser->host_features,
+        VIRTIO_CONSOLE_F_EMERG_WRITE) || !config->emerg_wr) {
         return;
     }
+
+    emerg_wr_lo = le32_to_cpu(config->emerg_wr);
     /* Make sure we don't misdetect an emergency write when the guest
      * does a short config write after an emergency write. */
     config->emerg_wr = 0;
-- 
MST

Re: [Qemu-devel] [PULL 0/5] virtio,vhost: fixes

[Peter Maydell]

On 9 April 2018 at 15:42, Michael S. Tsirkin <mst@redhat.com> wrote:
> The following changes since commit 0e87fdc966d05f4e5ad868034fcd8ee2a08ca62d:
>
>   Update version for v2.12.0-rc2 release (2018-04-04 20:37:20 +0100)
>
> are available in the git repository at:
>
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to d434e5ac5d70e9da7d20e50246af9251a125bdad:
>
>   virtio-serial: fix heap-over-flow (2018-04-09 17:35:46 +0300)
>
> ----------------------------------------------------------------
> virtio,vhost: fixes
>
> Add a feature flag for new protocol messages.
> Misc fixes.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ----------------------------------------------------------------

Applied, thanks.

-- PMM