[Qemu-devel] [PATCH v2 0/3] block: enforce constraints on block size properties

[Qemu-devel] [PATCH v2 0/3] block: enforce constraints on block size properties

[Stefan Hajnoczi]

This series ensures we only accept valid block sizes.  Although in theory block
sizes can vary a lot, the storage protocols (ATA, SCSI) as well as the QEMU
block layer implementation impose constraints.  Valid QEMU block sizes today
must meet:

 * Power of 2
 * Multiple of 512 bytes
 * Fits into uint16_t

Nicolae Mogoreanu <mogo@google.com> found that QEMU allows invalid block sizes
to be specified and this can cause it to crash when I/O is performed.

Stefan Hajnoczi (3):
  qerror: fix QERR_PROPERTY_VALUE_OUT_OF_RANGE description
  qdev: add blocksize property type
  block: enforce constraints on block size properties

 block.h              |    8 ++++----
 hw/qdev-properties.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h            |    3 +++
 qerror.c             |    7 ++++++-
 qerror.h             |    4 ++++
 5 files changed, 63 insertions(+), 5 deletions(-)

-- 
1.7.9.1

[Qemu-devel] [PATCH v2 1/3] qerror: fix QERR_PROPERTY_VALUE_OUT_OF_RANGE description

[Stefan Hajnoczi]

Fix a typo in the description for QERR_PROPERTY_VALUE_OUT_OF_RANGE where
"'" was used instead of ")".

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 qerror.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/qerror.c b/qerror.c
index f55d435..ecc5259 100644
--- a/qerror.c
+++ b/qerror.c
@@ -235,7 +235,7 @@ static const QErrorStringTable qerror_table[] = {
     {
         .error_fmt = QERR_PROPERTY_VALUE_OUT_OF_RANGE,
         .desc      = "Property '%(device).%(property)' doesn't take "
-                     "value %(value) (minimum: %(min), maximum: %(max)'",
+                     "value %(value) (minimum: %(min), maximum: %(max))",
     },
     {
         .error_fmt = QERR_QGA_COMMAND_FAILED,
-- 
1.7.9.1

[Qemu-devel] [PATCH v2 2/3] qdev: add blocksize property type

[Stefan Hajnoczi]

Storage interfaces like virtio-blk can be configured with block size
information so that the guest can take advantage of efficient I/O
request sizes.

According to the SCSI Block Commands (SBC) standard a device's block
size is "almost always greater than one byte and may be a multiple of
512 bytes".  QEMU currently has a 512 byte minimum block size because
the block layer functions work at that granularity.  Furthermore, the
block size should be a power of 2 because QEMU calculates bitmasks from
the value.

Introduce a "blocksize" property type so devices can enforce these
constraints on block size values.  If the constraints are relaxed in the
future then this property can be updated.

Introduce the new PropertyValueNotPowerOf2 QError so QMP clients know
exactly why a block size value was rejected.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 hw/qdev-properties.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h            |    3 +++
 qerror.c             |    5 +++++
 qerror.h             |    4 ++++
 4 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index bff9152..98dd06a 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -877,6 +877,52 @@ PropertyInfo qdev_prop_pci_devfn = {
     .max   = 0xFFFFFFFFULL,
 };
 
+/* --- blocksize --- */
+
+static void set_blocksize(Object *obj, Visitor *v, void *opaque,
+                          const char *name, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    int16_t *ptr = qdev_get_prop_ptr(dev, prop);
+    Error *local_err = NULL;
+    int64_t value;
+
+    if (dev->state != DEV_STATE_CREATED) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    }
+
+    visit_type_int(v, &value, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    if (value < prop->info->min || value > prop->info->max) {
+        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
+                  dev->id?:"", name, value, prop->info->min,
+                  prop->info->max);
+        return;
+    }
+
+    /* We rely on power-of-2 blocksizes for bitmasks */
+    if ((value & (value - 1)) != 0) {
+        error_set(errp, QERR_PROPERTY_VALUE_NOT_POWER_OF_2,
+                  dev->id?:"", name, value);
+        return;
+    }
+
+    *ptr = value;
+}
+
+PropertyInfo qdev_prop_blocksize = {
+    .name  = "blocksize",
+    .get   = get_int16,
+    .set   = set_blocksize,
+    .min   = 512,
+    .max   = 65024,
+};
+
 /* --- public helpers --- */
 
 static Property *qdev_prop_walk(Property *props, const char *name)
diff --git a/hw/qdev.h b/hw/qdev.h
index 9cc3f98..f8a5ddf 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -223,6 +223,7 @@ extern PropertyInfo qdev_prop_drive;
 extern PropertyInfo qdev_prop_netdev;
 extern PropertyInfo qdev_prop_vlan;
 extern PropertyInfo qdev_prop_pci_devfn;
+extern PropertyInfo qdev_prop_blocksize;
 
 #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
         .name      = (_name),                                    \
@@ -284,6 +285,8 @@ extern PropertyInfo qdev_prop_pci_devfn;
 #define DEFINE_PROP_LOSTTICKPOLICY(_n, _s, _f, _d) \
     DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_losttickpolicy, \
                         LostTickPolicy)
+#define DEFINE_PROP_BLOCKSIZE(_n, _s, _f, _d) \
+    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_blocksize, uint16_t)
 
 #define DEFINE_PROP_END_OF_LIST()               \
     {}
diff --git a/qerror.c b/qerror.c
index ecc5259..75c9a5c 100644
--- a/qerror.c
+++ b/qerror.c
@@ -233,6 +233,11 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Property '%(device).%(property)' can't find value '%(value)'",
     },
     {
+        .error_fmt = QERR_PROPERTY_VALUE_NOT_POWER_OF_2,
+        .desc      = "Property '%(device).%(property)' doesn't take "
+                     "value '%(value)', it's not a power of 2",
+    },
+    {
         .error_fmt = QERR_PROPERTY_VALUE_OUT_OF_RANGE,
         .desc      = "Property '%(device).%(property)' doesn't take "
                      "value %(value) (minimum: %(min), maximum: %(max))",
diff --git a/qerror.h b/qerror.h
index e26c635..a3b9a59 100644
--- a/qerror.h
+++ b/qerror.h
@@ -196,6 +196,10 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_PROPERTY_VALUE_NOT_FOUND \
     "{ 'class': 'PropertyValueNotFound', 'data': { 'device': %s, 'property': %s, 'value': %s } }"
 
+#define QERR_PROPERTY_VALUE_NOT_POWER_OF_2 \
+    "{ 'class': 'PropertyValueNotPowerOf2', 'data': { " \
+    "'device': %s, 'property': %s, 'value': %"PRId64" } }"
+
 #define QERR_PROPERTY_VALUE_OUT_OF_RANGE \
     "{ 'class': 'PropertyValueOutOfRange', 'data': { 'device': %s, 'property': %s, 'value': %"PRId64", 'min': %"PRId64", 'max': %"PRId64" } }"
 
-- 
1.7.9.1

[Qemu-devel] [PATCH v2 3/3] block: enforce constraints on block size properties

[Stefan Hajnoczi]

Nicolae Mogoreanu <mogo@google.com> noticed that I/O requests can lead
to QEMU crashes when the logical_block_size property is smaller than 512
bytes.

Using the new "blocksize" property we can properly enforce constraints
on the block size such that QEMU's block layer is able to operate
correctly.

Reported-by: Nicolae Mogoreanu <mogo@google.com>
Reported-by: Michael Halcrow <mhalcrow@google.com>
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block.h b/block.h
index 415bb17..3df6c30 100644
--- a/block.h
+++ b/block.h
@@ -437,10 +437,10 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
 
 #define DEFINE_BLOCK_PROPERTIES(_state, _conf)                          \
     DEFINE_PROP_DRIVE("drive", _state, _conf.bs),                       \
-    DEFINE_PROP_UINT16("logical_block_size", _state,                    \
-                       _conf.logical_block_size, 512),                  \
-    DEFINE_PROP_UINT16("physical_block_size", _state,                   \
-                       _conf.physical_block_size, 512),                 \
+    DEFINE_PROP_BLOCKSIZE("logical_block_size", _state,                 \
+                          _conf.logical_block_size, 512),               \
+    DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,                \
+                          _conf.physical_block_size, 512),              \
     DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),  \
     DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0),    \
     DEFINE_PROP_INT32("bootindex", _state, _conf.bootindex, -1),        \
-- 
1.7.9.1

Re: [Qemu-devel] [PATCH v2 1/3] qerror: fix QERR_PROPERTY_VALUE_OUT_OF_RANGE description

[Paolo Bonzini]

Il 14/03/2012 16:57, Stefan Hajnoczi ha scritto:
> Fix a typo in the description for QERR_PROPERTY_VALUE_OUT_OF_RANGE where
> "'" was used instead of ")".
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  qerror.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/qerror.c b/qerror.c
> index f55d435..ecc5259 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -235,7 +235,7 @@ static const QErrorStringTable qerror_table[] = {
>      {
>          .error_fmt = QERR_PROPERTY_VALUE_OUT_OF_RANGE,
>          .desc      = "Property '%(device).%(property)' doesn't take "
> -                     "value %(value) (minimum: %(min), maximum: %(max)'",
> +                     "value %(value) (minimum: %(min), maximum: %(max))",
>      },
>      {
>          .error_fmt = QERR_QGA_COMMAND_FAILED,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v2 2/3] qdev: add blocksize property type

[Paolo Bonzini]

Il 14/03/2012 16:57, Stefan Hajnoczi ha scritto:
> Storage interfaces like virtio-blk can be configured with block size
> information so that the guest can take advantage of efficient I/O
> request sizes.
> 
> According to the SCSI Block Commands (SBC) standard a device's block
> size is "almost always greater than one byte and may be a multiple of
> 512 bytes".  QEMU currently has a 512 byte minimum block size because
> the block layer functions work at that granularity.  Furthermore, the
> block size should be a power of 2 because QEMU calculates bitmasks from
> the value.
> 
> Introduce a "blocksize" property type so devices can enforce these
> constraints on block size values.  If the constraints are relaxed in the
> future then this property can be updated.
> 
> Introduce the new PropertyValueNotPowerOf2 QError so QMP clients know
> exactly why a block size value was rejected.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  hw/qdev-properties.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  hw/qdev.h            |    3 +++
>  qerror.c             |    5 +++++
>  qerror.h             |    4 ++++
>  4 files changed, 58 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index bff9152..98dd06a 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -877,6 +877,52 @@ PropertyInfo qdev_prop_pci_devfn = {
>      .max   = 0xFFFFFFFFULL,
>  };
>  
> +/* --- blocksize --- */
> +
> +static void set_blocksize(Object *obj, Visitor *v, void *opaque,
> +                          const char *name, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    int16_t *ptr = qdev_get_prop_ptr(dev, prop);
> +    Error *local_err = NULL;
> +    int64_t value;
> +
> +    if (dev->state != DEV_STATE_CREATED) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +
> +    visit_type_int(v, &value, name, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    if (value < prop->info->min || value > prop->info->max) {
> +        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
> +                  dev->id?:"", name, value, prop->info->min,
> +                  prop->info->max);
> +        return;
> +    }
> +
> +    /* We rely on power-of-2 blocksizes for bitmasks */
> +    if ((value & (value - 1)) != 0) {
> +        error_set(errp, QERR_PROPERTY_VALUE_NOT_POWER_OF_2,
> +                  dev->id?:"", name, value);
> +        return;
> +    }
> +
> +    *ptr = value;
> +}
> +
> +PropertyInfo qdev_prop_blocksize = {
> +    .name  = "blocksize",
> +    .get   = get_int16,
> +    .set   = set_blocksize,
> +    .min   = 512,
> +    .max   = 65024,
> +};
> +
>  /* --- public helpers --- */
>  
>  static Property *qdev_prop_walk(Property *props, const char *name)
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 9cc3f98..f8a5ddf 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -223,6 +223,7 @@ extern PropertyInfo qdev_prop_drive;
>  extern PropertyInfo qdev_prop_netdev;
>  extern PropertyInfo qdev_prop_vlan;
>  extern PropertyInfo qdev_prop_pci_devfn;
> +extern PropertyInfo qdev_prop_blocksize;
>  
>  #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
>          .name      = (_name),                                    \
> @@ -284,6 +285,8 @@ extern PropertyInfo qdev_prop_pci_devfn;
>  #define DEFINE_PROP_LOSTTICKPOLICY(_n, _s, _f, _d) \
>      DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_losttickpolicy, \
>                          LostTickPolicy)
> +#define DEFINE_PROP_BLOCKSIZE(_n, _s, _f, _d) \
> +    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_blocksize, uint16_t)
>  
>  #define DEFINE_PROP_END_OF_LIST()               \
>      {}
> diff --git a/qerror.c b/qerror.c
> index ecc5259..75c9a5c 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -233,6 +233,11 @@ static const QErrorStringTable qerror_table[] = {
>          .desc      = "Property '%(device).%(property)' can't find value '%(value)'",
>      },
>      {
> +        .error_fmt = QERR_PROPERTY_VALUE_NOT_POWER_OF_2,
> +        .desc      = "Property '%(device).%(property)' doesn't take "
> +                     "value '%(value)', it's not a power of 2",
> +    },
> +    {
>          .error_fmt = QERR_PROPERTY_VALUE_OUT_OF_RANGE,
>          .desc      = "Property '%(device).%(property)' doesn't take "
>                       "value %(value) (minimum: %(min), maximum: %(max))",
> diff --git a/qerror.h b/qerror.h
> index e26c635..a3b9a59 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -196,6 +196,10 @@ QError *qobject_to_qerror(const QObject *obj);
>  #define QERR_PROPERTY_VALUE_NOT_FOUND \
>      "{ 'class': 'PropertyValueNotFound', 'data': { 'device': %s, 'property': %s, 'value': %s } }"
>  
> +#define QERR_PROPERTY_VALUE_NOT_POWER_OF_2 \
> +    "{ 'class': 'PropertyValueNotPowerOf2', 'data': { " \
> +    "'device': %s, 'property': %s, 'value': %"PRId64" } }"
> +
>  #define QERR_PROPERTY_VALUE_OUT_OF_RANGE \
>      "{ 'class': 'PropertyValueOutOfRange', 'data': { 'device': %s, 'property': %s, 'value': %"PRId64", 'min': %"PRId64", 'max': %"PRId64" } }"
>  

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH v2 3/3] block: enforce constraints on block size properties

[Paolo Bonzini]

Il 14/03/2012 16:57, Stefan Hajnoczi ha scritto:
> Nicolae Mogoreanu <mogo@google.com> noticed that I/O requests can lead
> to QEMU crashes when the logical_block_size property is smaller than 512
> bytes.
> 
> Using the new "blocksize" property we can properly enforce constraints
> on the block size such that QEMU's block layer is able to operate
> correctly.
> 
> Reported-by: Nicolae Mogoreanu <mogo@google.com>
> Reported-by: Michael Halcrow <mhalcrow@google.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  block.h |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/block.h b/block.h
> index 415bb17..3df6c30 100644
> --- a/block.h
> +++ b/block.h
> @@ -437,10 +437,10 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
>  
>  #define DEFINE_BLOCK_PROPERTIES(_state, _conf)                          \
>      DEFINE_PROP_DRIVE("drive", _state, _conf.bs),                       \
> -    DEFINE_PROP_UINT16("logical_block_size", _state,                    \
> -                       _conf.logical_block_size, 512),                  \
> -    DEFINE_PROP_UINT16("physical_block_size", _state,                   \
> -                       _conf.physical_block_size, 512),                 \
> +    DEFINE_PROP_BLOCKSIZE("logical_block_size", _state,                 \
> +                          _conf.logical_block_size, 512),               \
> +    DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,                \
> +                          _conf.physical_block_size, 512),              \
>      DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),  \
>      DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0),    \
>      DEFINE_PROP_INT32("bootindex", _state, _conf.bootindex, -1),        \

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH v2 3/3] block: enforce constraints on block size properties

[Kevin Wolf]

Am 14.03.2012 17:04, schrieb Paolo Bonzini:
> Il 14/03/2012 16:57, Stefan Hajnoczi ha scritto:
>> Nicolae Mogoreanu <mogo@google.com> noticed that I/O requests can lead
>> to QEMU crashes when the logical_block_size property is smaller than 512
>> bytes.
>>
>> Using the new "blocksize" property we can properly enforce constraints
>> on the block size such that QEMU's block layer is able to operate
>> correctly.
>>
>> Reported-by: Nicolae Mogoreanu <mogo@google.com>
>> Reported-by: Michael Halcrow <mhalcrow@google.com>
>> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>> ---
>>  block.h |    8 ++++----
>>  1 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/block.h b/block.h
>> index 415bb17..3df6c30 100644
>> --- a/block.h
>> +++ b/block.h
>> @@ -437,10 +437,10 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
>>  
>>  #define DEFINE_BLOCK_PROPERTIES(_state, _conf)                          \
>>      DEFINE_PROP_DRIVE("drive", _state, _conf.bs),                       \
>> -    DEFINE_PROP_UINT16("logical_block_size", _state,                    \
>> -                       _conf.logical_block_size, 512),                  \
>> -    DEFINE_PROP_UINT16("physical_block_size", _state,                   \
>> -                       _conf.physical_block_size, 512),                 \
>> +    DEFINE_PROP_BLOCKSIZE("logical_block_size", _state,                 \
>> +                          _conf.logical_block_size, 512),               \
>> +    DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,                \
>> +                          _conf.physical_block_size, 512),              \
>>      DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),  \
>>      DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0),    \
>>      DEFINE_PROP_INT32("bootindex", _state, _conf.bootindex, -1),        \
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks, applied all.

Kevin

Re: [Qemu-devel] [PATCH v2 0/3] block: enforce constraints on block size properties

[Stefan Hajnoczi]

On Wed, Mar 14, 2012 at 3:57 PM, Stefan Hajnoczi
<stefanha@linux.vnet.ibm.com> wrote:
> This series ensures we only accept valid block sizes.  Although in theory block
> sizes can vary a lot, the storage protocols (ATA, SCSI) as well as the QEMU
> block layer implementation impose constraints.  Valid QEMU block sizes today
> must meet:
>
>  * Power of 2
>  * Multiple of 512 bytes
>  * Fits into uint16_t
>
> Nicolae Mogoreanu <mogo@google.com> found that QEMU allows invalid block sizes
> to be specified and this can cause it to crash when I/O is performed.
>
> Stefan Hajnoczi (3):
>  qerror: fix QERR_PROPERTY_VALUE_OUT_OF_RANGE description
>  qdev: add blocksize property type
>  block: enforce constraints on block size properties
>
>  block.h              |    8 ++++----
>  hw/qdev-properties.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  hw/qdev.h            |    3 +++
>  qerror.c             |    7 ++++++-
>  qerror.h             |    4 ++++
>  5 files changed, 63 insertions(+), 5 deletions(-)

Apologies for missing you off the CC list and thanks for pointing out
the issue!  These patches have just been reviewed and accepted - they
will be merged into qemu.git/master in the future.

Stefan