date:20200510

Re: [PATCH v2 5/5] vhost: add device started check in migration set log

2020-05-10 Thread Jason Wang




On 2020/4/30 下午9:36, Dima Stepanov wrote:

If vhost-user daemon is used as a backend for the vhost device, then we
should consider a possibility of disconnect at any moment. If such
disconnect happened in the vhost_migration_log() routine the vhost
device structure will be clean up.
At the start of the vhost_migration_log() function there is a check:
   if (!dev->started) {
   dev->log_enabled = enable;
   return 0;
   }
To be consistent with this check add the same check after calling the
vhost_dev_set_log() routine. This in general help not to break a
migration due the assert() message. But it looks like that this code
should be revised to handle these errors more carefully.

In case of vhost-user device backend the fail paths should consider the
state of the device. In this case we should skip some function calls
during rollback on the error paths, so not to get the NULL dereference
errors.

Signed-off-by: Dima Stepanov 
---
  hw/virtio/vhost.c | 39 +++
  1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 3ee50c4..d5ab96d 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -787,6 +787,17 @@ static int vhost_dev_set_features(struct vhost_dev *dev,
  static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
  {
  int r, i, idx;
+
+if (!dev->started) {
+/*
+ * If vhost-user daemon is used as a backend for the
+ * device and the connection is broken, then the vhost_dev
+ * structure will be reset all its values to 0.
+ * Add additional check for the device state.
+ */
+return -1;
+}
+
  r = vhost_dev_set_features(dev, enable_log);
  if (r < 0) {
  goto err_features;
@@ -801,12 +812,19 @@ static int vhost_dev_set_log(struct vhost_dev *dev, bool 
enable_log)
  }
  return 0;
  err_vq:
-for (; i >= 0; --i) {
+/*
+ * Disconnect with the vhost-user daemon can lead to the
+ * vhost_dev_cleanup() call which will clean up vhost_dev
+ * structure.
+ */
+for (; dev->started && (i >= 0); --i) {
  idx = dev->vhost_ops->vhost_get_vq_index(



Why need the check of dev->started here, can started be modified outside 
mainloop? If yes, I don't get the check of !dev->started in the 
beginning of this function.




dev, dev->vq_index + i);
  vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
   dev->log_enabled);
  }
-vhost_dev_set_features(dev, dev->log_enabled);
+if (dev->started) {
+vhost_dev_set_features(dev, dev->log_enabled);
+}
  err_features:
  return r;
  }
@@ -832,7 +850,15 @@ static int vhost_migration_log(MemoryListener *listener, 
int enable)
  } else {
  vhost_dev_log_resize(dev, vhost_get_log_size(dev));
  r = vhost_dev_set_log(dev, true);
-if (r < 0) {
+/*
+ * The dev log resize can fail, because of disconnect
+ * with the vhost-user-blk daemon. Check the device
+ * state before calling the vhost_dev_set_log()
+ * function.
+ * Don't return error if device isn't started to be
+ * consistent with the check above.
+ */
+if (dev->started && r < 0) {
  return r;
  }
  }
@@ -1739,7 +1765,12 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice 
*vdev)
  fail_log:
  vhost_log_put(hdev, false);
  fail_vq:
-while (--i >= 0) {
+/*
+ * Disconnect with the vhost-user daemon can lead to the
+ * vhost_dev_cleanup() call which will clean up vhost_dev
+ * structure.
+ */
+while ((--i >= 0) && (hdev->started)) {
  vhost_virtqueue_stop(hdev,
   vdev,
   hdev->vqs + i,



This should be a separate patch.

Thanks

Re: [PATCH v2 4/5] vhost: check vring address before calling unmap

2020-05-10 Thread Jason Wang




On 2020/4/30 下午9:36, Dima Stepanov wrote:

Since disconnect can happen at any time during initialization not all
vring buffers (for instance used vring) can be intialized successfully.
If the buffer was not initialized then vhost_memory_unmap call will lead
to SIGSEGV. Add checks for the vring address value before calling unmap.
Also add assert() in the vhost_memory_unmap() routine.

Signed-off-by: Dima Stepanov 
---
  hw/virtio/vhost.c | 27 +--
  1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index ddbdc53..3ee50c4 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -314,6 +314,8 @@ static void vhost_memory_unmap(struct vhost_dev *dev, void 
*buffer,
 hwaddr len, int is_write,
 hwaddr access_len)
  {
+assert(buffer);
+
  if (!vhost_dev_has_iommu(dev)) {
  cpu_physical_memory_unmap(buffer, len, is_write, access_len);
  }
@@ -1132,12 +1134,25 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
  vhost_vq_index);
  }
  
-vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, idx),

-   1, virtio_queue_get_used_size(vdev, idx));
-vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vdev, idx),
-   0, virtio_queue_get_avail_size(vdev, idx));
-vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
-   0, virtio_queue_get_desc_size(vdev, idx));
+/*
+ * Since the vhost-user disconnect can happen during initialization
+ * check if vring was initialized, before making unmap.
+ */
+if (vq->used) {
+vhost_memory_unmap(dev, vq->used,
+   virtio_queue_get_used_size(vdev, idx),
+   1, virtio_queue_get_used_size(vdev, idx));
+}
+if (vq->avail) {
+vhost_memory_unmap(dev, vq->avail,
+   virtio_queue_get_avail_size(vdev, idx),
+   0, virtio_queue_get_avail_size(vdev, idx));
+}
+if (vq->desc) {
+vhost_memory_unmap(dev, vq->desc,
+   virtio_queue_get_desc_size(vdev, idx),
+   0, virtio_queue_get_desc_size(vdev, idx));
+}



Any reason not checking hdev->started instead? vhost_dev_start() will 
set it to true if virtqueues were correctly mapped.


Thanks



  }
  
  static void vhost_eventfd_add(MemoryListener *listener,

Re: [PATCH v2 2/5] vhost: introduce wrappers to set guest notifiers for virtio device

2020-05-10 Thread Jason Wang




On 2020/4/30 下午9:36, Dima Stepanov wrote:

Introduce new wrappers to set/reset guest notifiers for the virtio
device in the vhost device module:
   vhost_dev_assign_guest_notifiers
 ->set_guest_notifiers(..., ..., true);
   vhost_dev_drop_guest_notifiers
 ->set_guest_notifiers(..., ..., false);
This is a preliminary step to refactor code,



Maybe I miss something, I don't see any add-on patch to modify the new 
wrapper in this series?




  so the set_guest_notifiers
methods could be called based on the vhost device state.
Update all vhost used devices to use these wrappers instead of direct
method call.

Signed-off-by: Dima Stepanov 
---
  backends/cryptodev-vhost.c  | 26 +++---
  backends/vhost-user.c   | 16 +---
  hw/block/vhost-user-blk.c   | 15 +--
  hw/net/vhost_net.c  | 30 +-
  hw/scsi/vhost-scsi-common.c | 15 +--
  hw/virtio/vhost-user-fs.c   | 17 +++--
  hw/virtio/vhost-vsock.c | 18 --
  hw/virtio/vhost.c   | 38 ++
  hw/virtio/virtio.c  | 13 +
  include/hw/virtio/vhost.h   |  4 
  include/hw/virtio/virtio.h  |  1 +
  11 files changed, 118 insertions(+), 75 deletions(-)

diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
index 8337c9a..4522195 100644
--- a/backends/cryptodev-vhost.c
+++ b/backends/cryptodev-vhost.c
@@ -169,16 +169,13 @@ vhost_set_vring_enable(CryptoDevBackendClient *cc,
  int cryptodev_vhost_start(VirtIODevice *dev, int total_queues)
  {
  VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(dev);
-BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(dev)));
-VirtioBusState *vbus = VIRTIO_BUS(qbus);
-VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
  int r, e;
  int i;
  CryptoDevBackend *b = vcrypto->cryptodev;
  CryptoDevBackendVhost *vhost_crypto;
  CryptoDevBackendClient *cc;
  
-if (!k->set_guest_notifiers) {

+if (!virtio_device_guest_notifiers_initialized(dev)) {
  error_report("binding does not support guest notifiers");
  return -ENOSYS;
  }
@@ -198,9 +195,13 @@ int cryptodev_vhost_start(VirtIODevice *dev, int 
total_queues)
  }
   }
  
-r = k->set_guest_notifiers(qbus->parent, total_queues, true);

+/*
+ * Since all the states are handled by one vhost device,
+ * use the first one in array.
+ */
+vhost_crypto = cryptodev_get_vhost(b->conf.peers.ccs[0], b, 0);
+r = vhost_dev_assign_guest_notifiers(_crypto->dev, dev, 
total_queues);
  if (r < 0) {
-error_report("error binding guest notifier: %d", -r);
  goto err;
  }
  
@@ -232,7 +233,8 @@ err_start:

  vhost_crypto = cryptodev_get_vhost(cc, b, i);
  cryptodev_vhost_stop_one(vhost_crypto, dev);
  }
-e = k->set_guest_notifiers(qbus->parent, total_queues, false);
+vhost_crypto = cryptodev_get_vhost(b->conf.peers.ccs[0], b, 0);
+e = vhost_dev_drop_guest_notifiers(_crypto->dev, dev, total_queues);
  if (e < 0) {
  error_report("vhost guest notifier cleanup failed: %d", e);
  }
@@ -242,9 +244,6 @@ err:
  
  void cryptodev_vhost_stop(VirtIODevice *dev, int total_queues)

  {
-BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(dev)));
-VirtioBusState *vbus = VIRTIO_BUS(qbus);
-VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
  VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(dev);
  CryptoDevBackend *b = vcrypto->cryptodev;
  CryptoDevBackendVhost *vhost_crypto;
@@ -259,7 +258,12 @@ void cryptodev_vhost_stop(VirtIODevice *dev, int 
total_queues)
  cryptodev_vhost_stop_one(vhost_crypto, dev);
  }
  
-r = k->set_guest_notifiers(qbus->parent, total_queues, false);

+/*
+ * Since all the states are handled by one vhost device,
+ * use the first one in array.
+ */
+vhost_crypto = cryptodev_get_vhost(b->conf.peers.ccs[0], b, 0);
+r = vhost_dev_drop_guest_notifiers(_crypto->dev, dev, total_queues);
  if (r < 0) {
  error_report("vhost guest notifier cleanup failed: %d", r);
  }
diff --git a/backends/vhost-user.c b/backends/vhost-user.c
index 2bf3406..e116bc6 100644
--- a/backends/vhost-user.c
+++ b/backends/vhost-user.c
@@ -60,15 +60,13 @@ vhost_user_backend_dev_init(VhostUserBackend *b, 
VirtIODevice *vdev,
  void
  vhost_user_backend_start(VhostUserBackend *b)
  {
-BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(b->vdev)));
-VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
  int ret, i ;
  
  if (b->started) {

  return;
  }
  
-if (!k->set_guest_notifiers) {

+if (!virtio_device_guest_notifiers_initialized(b->vdev)) {
  error_report("binding does not support guest notifiers");
  return;
  }
@@ -78,9 +76,8 @@ vhost_user_backend_start(VhostUserBackend *b)
  return;
  }
  
-ret = k->set_guest_notifiers(qbus->parent, b->dev.nvqs,

Re: [PATCH 05/10] exec: Move qemu_minrampagesize/qemu_maxrampagesize to 'qemu-common.h'

2020-05-10 Thread David Gibson

On Thu, May 07, 2020 at 07:39:53PM +0200, Philippe Mathieu-Daudé wrote:
> Move these generic functions to a more common place, with other
> functions related to host page size. Document a little.
> 
> Cc: Alexey Kardashevskiy 
> Signed-off-by: Philippe Mathieu-Daudé 

ppc parts
Acked-by: David Gibson 

> ---
>  include/exec/ram_addr.h|  3 ---
>  include/qemu-common.h  | 10 ++
>  hw/ppc/spapr_caps.c|  2 +-
>  hw/s390x/s390-virtio-ccw.c |  1 +
>  hw/vfio/spapr.c|  2 +-
>  5 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 06096e8c6a..195b67d3c8 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -93,9 +93,6 @@ static inline unsigned long int 
> ramblock_recv_bitmap_offset(void *host_addr,
>  
>  bool ramblock_is_pmem(RAMBlock *rb);
>  
> -long qemu_minrampagesize(void);
> -long qemu_maxrampagesize(void);
> -
>  /**
>   * qemu_ram_alloc_from_file,
>   * qemu_ram_alloc_from_fd:  Allocate a ram block from the specified backing
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index d0142f29ac..2821a6ef76 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -80,6 +80,16 @@ bool set_preferred_target_page_bits(int bits);
>   */
>  void finalize_target_page_bits(void);
>  
> +/**
> + * qemu_minrampagesize:
> + * qemu_maxrampagesize:
> + *
> + * If backed via -memdev, return the device page size,
> + * else return the host kernel page size.
> + */
> +long qemu_minrampagesize(void);
> +long qemu_maxrampagesize(void);
> +
>  /**
>   * Sends a (part of) iovec down a socket, yielding when the socket is full, 
> or
>   * Receives data into a (part of) iovec from a socket,
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index eb54f94227..33a802a103 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -23,11 +23,11 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu-common.h"
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "qapi/visitor.h"
>  #include "sysemu/hw_accel.h"
> -#include "exec/ram_addr.h"
>  #include "target/ppc/cpu.h"
>  #include "target/ppc/mmu-hash64.h"
>  #include "cpu-models.h"
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index f660070d22..c009384505 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -12,6 +12,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu-common.h"
>  #include "qapi/error.h"
>  #include "cpu.h"
>  #include "hw/boards.h"
> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
> index 2900bd1941..c64db940a7 100644
> --- a/hw/vfio/spapr.c
> +++ b/hw/vfio/spapr.c
> @@ -9,13 +9,13 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu-common.h"
>  #include "cpu.h"
>  #include 
>  #include 
>  
>  #include "hw/vfio/vfio-common.h"
>  #include "hw/hw.h"
> -#include "exec/ram_addr.h"
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "trace.h"

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH 10/10] exec: Move cpu_physical_memory_* functions to 'exec/memory-internal.h'

2020-05-10 Thread David Gibson

On Thu, May 07, 2020 at 07:39:58PM +0200, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé 

ppc parts
Acked-by: David Gibson 

> ---
>  include/exec/memory-internal.h | 305 -
>  include/exec/ram_addr.h| 303 +---
>  accel/tcg/cputlb.c |   1 -
>  hw/ppc/spapr.c |   1 -
>  hw/ppc/spapr_pci.c |   1 -
>  memory.c   |   1 -
>  6 files changed, 305 insertions(+), 307 deletions(-)
> 
> diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
> index b2b7c1e78a..4abb3bbd85 100644
> --- a/include/exec/memory-internal.h
> +++ b/include/exec/memory-internal.h
> @@ -21,8 +21,13 @@
>  #define MEMORY_INTERNAL_H
>  
>  #include "cpu.h"
> +#include "sysemu/tcg.h"
> +#include "sysemu/xen.h"
> +#include "exec/ramlist.h"
> +#include "exec/ramblock.h"
>  
>  #ifdef CONFIG_SOFTMMU
> +
>  static inline AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv)
>  {
>  return fv->dispatch;
> @@ -49,5 +54,303 @@ void address_space_dispatch_free(AddressSpaceDispatch *d);
>  
>  void mtree_print_dispatch(struct AddressSpaceDispatch *d,
>MemoryRegion *root);
> -#endif
> +
> +#define DIRTY_CLIENTS_ALL ((1 << DIRTY_MEMORY_NUM) - 1)
> +#define DIRTY_CLIENTS_NOCODE  (DIRTY_CLIENTS_ALL & ~(1 << DIRTY_MEMORY_CODE))
> +
> +static inline bool cpu_physical_memory_get_dirty(ram_addr_t start,
> + ram_addr_t length,
> + unsigned client)
> +{
> +DirtyMemoryBlocks *blocks;
> +unsigned long end, page;
> +unsigned long idx, offset, base;
> +bool dirty = false;
> +
> +assert(client < DIRTY_MEMORY_NUM);
> +
> +end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
> +page = start >> TARGET_PAGE_BITS;
> +
> +WITH_RCU_READ_LOCK_GUARD() {
> +blocks = atomic_rcu_read(_list.dirty_memory[client]);
> +
> +idx = page / DIRTY_MEMORY_BLOCK_SIZE;
> +offset = page % DIRTY_MEMORY_BLOCK_SIZE;
> +base = page - offset;
> +while (page < end) {
> +unsigned long next = MIN(end, base + DIRTY_MEMORY_BLOCK_SIZE);
> +unsigned long num = next - base;
> +unsigned long found = find_next_bit(blocks->blocks[idx],
> +num, offset);
> +if (found < num) {
> +dirty = true;
> +break;
> +}
> +
> +page = next;
> +idx++;
> +offset = 0;
> +base += DIRTY_MEMORY_BLOCK_SIZE;
> +}
> +}
> +
> +return dirty;
> +}
> +
> +static inline bool cpu_physical_memory_all_dirty(ram_addr_t start,
> + ram_addr_t length,
> + unsigned client)
> +{
> +DirtyMemoryBlocks *blocks;
> +unsigned long end, page;
> +unsigned long idx, offset, base;
> +bool dirty = true;
> +
> +assert(client < DIRTY_MEMORY_NUM);
> +
> +end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
> +page = start >> TARGET_PAGE_BITS;
> +
> +RCU_READ_LOCK_GUARD();
> +
> +blocks = atomic_rcu_read(_list.dirty_memory[client]);
> +
> +idx = page / DIRTY_MEMORY_BLOCK_SIZE;
> +offset = page % DIRTY_MEMORY_BLOCK_SIZE;
> +base = page - offset;
> +while (page < end) {
> +unsigned long next = MIN(end, base + DIRTY_MEMORY_BLOCK_SIZE);
> +unsigned long num = next - base;
> +unsigned long found = find_next_zero_bit(blocks->blocks[idx],
> + num, offset);
> +if (found < num) {
> +dirty = false;
> +break;
> +}
> +
> +page = next;
> +idx++;
> +offset = 0;
> +base += DIRTY_MEMORY_BLOCK_SIZE;
> +}
> +
> +return dirty;
> +}
> +
> +static inline bool cpu_physical_memory_get_dirty_flag(ram_addr_t addr,
> +  unsigned client)
> +{
> +return cpu_physical_memory_get_dirty(addr, 1, client);
> +}
> +
> +static inline bool cpu_physical_memory_is_clean(ram_addr_t addr)
> +{
> +bool vga = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_VGA);
> +bool code = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_CODE);
> +bool migration = cpu_physical_memory_get_dirty_flag(addr,
> +
> DIRTY_MEMORY_MIGRATION);
> +return !(vga && code && migration);
> +}
> +
> +static inline uint8_t cpu_physical_memory_range_includes_clean(ram_addr_t 
> start,
> +ram_addr_t 
> length,
> +uint8_t mask)
> +{
> +uint8_t ret = 0;
> +
> +if (mask & (1 << DIRTY_MEMORY_VGA)

Re: [PATCH 07/10] exec: Move all RAMBlock functions to 'exec/ramblock.h'

2020-05-10 Thread David Gibson

On Thu, May 07, 2020 at 07:39:55PM +0200, Philippe Mathieu-Daudé wrote:
> The RAMBlock API was dispersed in 3 different headers.
> One of these headers, "exec/ram_addr.h", is restricted
> to target dependent code. However these functions are
> not target specific. Move all functions into a single
> place.  Now all these functions can be accessed by
> target-agnostic code.
> 
> Signed-off-by: Philippe Mathieu-Daudé 

ppc parts
Acked-by: David Gibson 

> ---
>  include/exec/cpu-common.h|  24 ---
>  include/exec/ram_addr.h  | 105 ---
>  include/exec/ramblock.h  | 134 +++
>  migration/migration.h|   1 +
>  accel/tcg/translate-all.c|   2 -
>  hw/block/nvme.c  |   2 +-
>  hw/s390x/s390-stattrib-kvm.c |   1 -
>  hw/s390x/s390-stattrib.c |   1 -
>  hw/s390x/s390-virtio-ccw.c   |   1 -
>  hw/virtio/vhost-user.c   |   1 +
>  hw/virtio/vhost.c|   1 +
>  hw/virtio/virtio-balloon.c   |   1 +
>  memory.c |   1 +
>  migration/migration.c|   1 +
>  migration/postcopy-ram.c |   1 +
>  migration/savevm.c   |   1 +
>  stubs/ram-block.c|   2 +-
>  target/ppc/kvm.c |   1 -
>  target/s390x/kvm.c   |   1 -
>  util/vfio-helpers.c  |   2 +-
>  20 files changed, 145 insertions(+), 139 deletions(-)
> 
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index b47e5630e7..347ceb603b 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -49,25 +49,6 @@ typedef uint32_t CPUReadMemoryFunc(void *opaque, hwaddr 
> addr);
>  void qemu_ram_remap(ram_addr_t addr, ram_addr_t length);
>  /* This should not be used by devices.  */
>  ram_addr_t qemu_ram_addr_from_host(void *ptr);
> -RAMBlock *qemu_ram_block_by_name(const char *name);
> -RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
> -   ram_addr_t *offset);
> -ram_addr_t qemu_ram_block_host_offset(RAMBlock *rb, void *host);
> -void qemu_ram_set_idstr(RAMBlock *block, const char *name, DeviceState *dev);
> -void qemu_ram_unset_idstr(RAMBlock *block);
> -const char *qemu_ram_get_idstr(RAMBlock *rb);
> -void *qemu_ram_get_host_addr(RAMBlock *rb);
> -ram_addr_t qemu_ram_get_offset(RAMBlock *rb);
> -ram_addr_t qemu_ram_get_used_length(RAMBlock *rb);
> -bool qemu_ram_is_shared(RAMBlock *rb);
> -bool qemu_ram_is_uf_zeroable(RAMBlock *rb);
> -void qemu_ram_set_uf_zeroable(RAMBlock *rb);
> -bool qemu_ram_is_migratable(RAMBlock *rb);
> -void qemu_ram_set_migratable(RAMBlock *rb);
> -void qemu_ram_unset_migratable(RAMBlock *rb);
> -
> -size_t qemu_ram_pagesize(RAMBlock *block);
> -size_t qemu_ram_pagesize_largest(void);
>  
>  void cpu_physical_memory_rw(hwaddr addr, void *buf,
>  hwaddr len, bool is_write);
> @@ -100,11 +81,6 @@ void qemu_flush_coalesced_mmio_buffer(void);
>  
>  void cpu_flush_icache_range(hwaddr start, hwaddr len);
>  
> -typedef int (RAMBlockIterFunc)(RAMBlock *rb, void *opaque);
> -
> -int qemu_ram_foreach_block(RAMBlockIterFunc func, void *opaque);
> -int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t length);
> -
>  #endif
>  
>  #endif /* CPU_COMMON_H */
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index c61d5188f7..0deffad66f 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -26,112 +26,7 @@
>  #include "exec/ramlist.h"
>  #include "exec/ramblock.h"
>  
> -/**
> - * clear_bmap_size: calculate clear bitmap size
> - *
> - * @pages: number of guest pages
> - * @shift: guest page number shift
> - *
> - * Returns: number of bits for the clear bitmap
> - */
> -static inline long clear_bmap_size(uint64_t pages, uint8_t shift)
> -{
> -return DIV_ROUND_UP(pages, 1UL << shift);
> -}
>  
> -/**
> - * clear_bmap_set: set clear bitmap for the page range
> - *
> - * @rb: the ramblock to operate on
> - * @start: the start page number
> - * @size: number of pages to set in the bitmap
> - *
> - * Returns: None
> - */
> -static inline void clear_bmap_set(RAMBlock *rb, uint64_t start,
> -  uint64_t npages)
> -{
> -uint8_t shift = rb->clear_bmap_shift;
> -
> -bitmap_set_atomic(rb->clear_bmap, start >> shift,
> -  clear_bmap_size(npages, shift));
> -}
> -
> -/**
> - * clear_bmap_test_and_clear: test clear bitmap for the page, clear if set
> - *
> - * @rb: the ramblock to operate on
> - * @page: the page number to check
> - *
> - * Returns: true if the bit was set, false otherwise
> - */
> -static inline bool clear_bmap_test_and_clear(RAMBlock *rb, uint64_t page)
> -{
> -uint8_t shift = rb->clear_bmap_shift;
> -
> -return bitmap_test_and_clear_atomic(rb->clear_bmap, page >> shift, 1);
> -}
> -
> -static inline bool offset_in_ramblock(RAMBlock *b, ram_addr_t offset)
> -{
> -return (b && b->host && offset < b->used_length) ? true : false;
> -}
> -
>

Re: [PATCH v2 5/5] vhost: add device started check in migration set log

2020-05-10 Thread Raphael Norwitz

On Thu, May 7, 2020 at 11:35 AM Dima Stepanov  wrote:
>
> What do you think?
>

Apologies - I tripped over the if (dev->started && r < 0) check.
Never-mind my point with race conditions and failing migrations.

Rather than modifying vhost_dev_set_log(), it may be clearer to put a
check after vhost_dev_log_resize()? Something like:

--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -829,11 +829,22 @@ static int vhost_migration_log(MemoryListener
*listener, int enable)
 vhost_log_put(dev, false);
 } else {
 vhost_dev_log_resize(dev, vhost_get_log_size(dev));
+/*
+ * A device can be stopped because of backend disconnect inside
+ * vhost_dev_log_resize(). In this case we should mark logging
+ * enabled and return without attempting to set the backend
+ * logging state.
+ */
+if (!dev->started) {
+goto out_success;
+}
 r = vhost_dev_set_log(dev, true);
 if (r < 0) {
 return r;
 }
 }
+
+out_success:
 dev->log_enabled = enable;
 return 0;
 }

This seems harmless enough to me, and I see how it fixes your
particular crash, but I would still prefer we worked towards a more
robust solution. In particular I think we could handle this inside
vhost-user-blk if we let the device state persist between connections
(i.e. call vhost_dev_cleanup() inside vhost_user_blk_connect() before
vhost_dev_init() on reconnect). This should also fix some of the
crashes Li Feng has hit, and probably others which haven’t been
reported yet. What do you think?

If that’s unworkable I guess we will need to add these vhost level
checks. In that case I would still prefer we add a “disconnected” flag
in struct vhost_dev struct, and make sure it isn’t cleared by
vhost_dev_cleanup(). That way we don’t conflate stopping a device with
backend disconnect at the vhost level and potentially regress behavior
for other device types.

Re: [PATCH v6 00/14] LUKS: encryption slot management using amend interface

2020-05-10 Thread no-reply

Patchew URL: 
https://patchew.org/QEMU/20200510134037.18487-1-mlevi...@redhat.com/



Hi,

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

Message-id: 20200510134037.18487-1-mlevi...@redhat.com
Subject: [PATCH v6 00/14] LUKS: encryption slot management using amend interface
Type: series

=== 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
Switched to a new branch 'test'
3101e5b iotests: add tests for blockdev-amend
b70ec78 block/qcow2: implement blockdev-amend
9e35a2e block/crypto: implement blockdev-amend
82ebd45 block/core: add generic infrastructure for x-blockdev-amend qmp command
e9127d8 iotests: qemu-img tests for luks key management
8094f01 iotests: filter few more luks specific create options
8194c2e block/qcow2: extend qemu-img amend interface with crypto options
879b332 block/crypto: implement the encryption key management
31d1394 block/crypto: rename two functions
8fe4f7c block/amend: refactor qcow2 amend options
44b5732 block/amend: separate amend and create options for qemu-img
48f04a1 block/amend: add 'force' option
ad0086e qcrypto/luks: implement encryption key management
e646655 qcrypto/core: add generic infrastructure for crypto options amendment

=== OUTPUT BEGIN ===
1/14 Checking commit e6466555db82 (qcrypto/core: add generic infrastructure for 
crypto options amendment)
2/14 Checking commit ad0086e3aa1a (qcrypto/luks: implement encryption key 
management)
3/14 Checking commit 48f04a11fd2f (block/amend: add 'force' option)
4/14 Checking commit 44b57321140d (block/amend: separate amend and create 
options for qemu-img)
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#31: FILE: block/qcow2.c:5523:
+#define QCOW_COMMON_OPTIONS \
+{   \
+.name = BLOCK_OPT_SIZE, \
+.type = QEMU_OPT_SIZE,  \
+.help = "Virtual disk size" \
+},  \
+{   \
+.name = BLOCK_OPT_COMPAT_LEVEL, \
+.type = QEMU_OPT_STRING,\
+.help = "Compatibility level (v2 [0.10] or v3 [1.1])"   \
+},  \
+{   \
+.name = BLOCK_OPT_BACKING_FILE, \
+.type = QEMU_OPT_STRING,\
+.help = "File name of a base image" \
+},  \
+{   \
+.name = BLOCK_OPT_BACKING_FMT,  \
+.type = QEMU_OPT_STRING,\
+.help = "Image format of the base image"\
+},  \
+{   \
+.name = BLOCK_OPT_DATA_FILE,\
+.type = QEMU_OPT_STRING,\
+.help = "File name of an external data file"\
+},  \
+{   \
+.name = BLOCK_OPT_DATA_FILE_RAW,\
+.type = QEMU_OPT_BOOL,  \
+.help = "The external data file must stay valid "   \
+"as a raw image"\
+},  \
+{   \
+.name = BLOCK_OPT_ENCRYPT,  \
+.type = QEMU_OPT_BOOL,  \
+.help = "Encrypt the image with format 'aes'. (Deprecated " \
+"in favor of " BLOCK_OPT_ENCRYPT_FORMAT "=aes)",\
+},  \
+{   \
+.name = BLOCK_OPT_ENCRYPT_FORMAT,   \
+.type = QEMU_OPT_STRING,\
+.help = "Encrypt the image, format choices: 'aes', 'luks'", \
+},

Re: [PATCH v6 00/14] LUKS: encryption slot management using amend interface

2020-05-10 Thread no-reply

Patchew URL: 
https://patchew.org/QEMU/20200510134037.18487-1-mlevi...@redhat.com/



Hi,

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

Message-id: 20200510134037.18487-1-mlevi...@redhat.com
Subject: [PATCH v6 00/14] LUKS: encryption slot management using amend interface
Type: series

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

Switched to a new branch 'test'
948c6e4 iotests: add tests for blockdev-amend
9c4cd87 block/qcow2: implement blockdev-amend
6d655b7 block/crypto: implement blockdev-amend
42181d1 block/core: add generic infrastructure for x-blockdev-amend qmp command
11f963d iotests: qemu-img tests for luks key management
b70f7c5 iotests: filter few more luks specific create options
4026156 block/qcow2: extend qemu-img amend interface with crypto options
76f48dd block/crypto: implement the encryption key management
e9f1cd7 block/crypto: rename two functions
6c6fd56 block/amend: refactor qcow2 amend options
0329c9b block/amend: separate amend and create options for qemu-img
327303a block/amend: add 'force' option
c18fcdb qcrypto/luks: implement encryption key management
9cde8e3 qcrypto/core: add generic infrastructure for crypto options amendment

=== OUTPUT BEGIN ===
1/14 Checking commit 9cde8e3d217a (qcrypto/core: add generic infrastructure for 
crypto options amendment)
2/14 Checking commit c18fcdb9afa4 (qcrypto/luks: implement encryption key 
management)
3/14 Checking commit 327303abdfaa (block/amend: add 'force' option)
4/14 Checking commit 0329c9bb273e (block/amend: separate amend and create 
options for qemu-img)
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#31: FILE: block/qcow2.c:5523:
+#define QCOW_COMMON_OPTIONS \
+{   \
+.name = BLOCK_OPT_SIZE, \
+.type = QEMU_OPT_SIZE,  \
+.help = "Virtual disk size" \
+},  \
+{   \
+.name = BLOCK_OPT_COMPAT_LEVEL, \
+.type = QEMU_OPT_STRING,\
+.help = "Compatibility level (v2 [0.10] or v3 [1.1])"   \
+},  \
+{   \
+.name = BLOCK_OPT_BACKING_FILE, \
+.type = QEMU_OPT_STRING,\
+.help = "File name of a base image" \
+},  \
+{   \
+.name = BLOCK_OPT_BACKING_FMT,  \
+.type = QEMU_OPT_STRING,\
+.help = "Image format of the base image"\
+},  \
+{   \
+.name = BLOCK_OPT_DATA_FILE,\
+.type = QEMU_OPT_STRING,\
+.help = "File name of an external data file"\
+},  \
+{   \
+.name = BLOCK_OPT_DATA_FILE_RAW,\
+.type = QEMU_OPT_BOOL,  \
+.help = "The external data file must stay valid "   \
+"as a raw image"\
+},  \
+{   \
+.name = BLOCK_OPT_ENCRYPT,  \
+.type = QEMU_OPT_BOOL,  \
+.help = "Encrypt the image with format 'aes'. (Deprecated " \
+"in favor of " BLOCK_OPT_ENCRYPT_FORMAT "=aes)",\
+},  \
+{   \
+.name = BLOCK_OPT_ENCRYPT_FORMAT,   \
+.type = QEMU_OPT_STRING,\
+.help = "Encrypt the image, format choices: 'aes', 'luks'", \
+},  \
+

[PATCH v6 14/14] iotests: add tests for blockdev-amend

2020-05-10 Thread Maxim Levitsky

This commit adds two tests that cover the
new blockdev-amend functionality of luks and qcow2 driver

Signed-off-by: Maxim Levitsky 
Reviewed-by: Daniel P. Berrangé 
---
 tests/qemu-iotests/295 | 279 +
 tests/qemu-iotests/295.out |  40 ++
 tests/qemu-iotests/296 | 234 +++
 tests/qemu-iotests/296.out |  33 +
 tests/qemu-iotests/group   |   2 +
 5 files changed, 588 insertions(+)
 create mode 100755 tests/qemu-iotests/295
 create mode 100644 tests/qemu-iotests/295.out
 create mode 100755 tests/qemu-iotests/296
 create mode 100644 tests/qemu-iotests/296.out

diff --git a/tests/qemu-iotests/295 b/tests/qemu-iotests/295
new file mode 100755
index 00..be5dfe5063
--- /dev/null
+++ b/tests/qemu-iotests/295
@@ -0,0 +1,279 @@
+#!/usr/bin/env python3
+#
+# Test case QMP's encrypted key management
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import iotests
+import os
+import time
+import json
+
+test_img = os.path.join(iotests.test_dir, 'test.img')
+
+class Secret:
+def __init__(self, index):
+self._id = "keysec" + str(index)
+# you are not supposed to see the password...
+self._secret = "hunter" + str(index)
+
+def id(self):
+return self._id
+
+def secret(self):
+return self._secret
+
+def to_cmdline_object(self):
+return  [ "secret,id=" + self._id + ",data=" + self._secret]
+
+def to_qmp_object(self):
+return { "qom_type" : "secret", "id": self.id(),
+ "props": { "data": self.secret() } }
+
+
+class EncryptionSetupTestCase(iotests.QMPTestCase):
+
+# test case startup
+def setUp(self):
+# start the VM
+self.vm = iotests.VM()
+self.vm.launch()
+
+# create the secrets and load 'em into the VM
+self.secrets = [ Secret(i) for i in range(0, 6) ]
+for secret in self.secrets:
+result = self.vm.qmp("object-add", **secret.to_qmp_object())
+self.assert_qmp(result, 'return', {})
+
+if iotests.imgfmt == "qcow2":
+self.pfx = "encrypt."
+self.img_opts = [ '-o', "encrypt.format=luks" ]
+else:
+self.pfx = ""
+self.img_opts = []
+
+# test case shutdown
+def tearDown(self):
+# stop the VM
+self.vm.shutdown()
+
+###
+# create the encrypted block device
+def createImg(self, file, secret):
+
+iotests.qemu_img(
+'create',
+'--object', *secret.to_cmdline_object(),
+'-f', iotests.imgfmt,
+'-o', self.pfx + 'key-secret=' + secret.id(),
+'-o', self.pfx + 'iter-time=10',
+*self.img_opts,
+file,
+'1M')
+
+###
+# open an encrypted block device
+def openImageQmp(self, id, file, secret, read_only = False):
+
+encrypt_options = {
+'key-secret' : secret.id()
+}
+
+if iotests.imgfmt == "qcow2":
+encrypt_options = {
+'encrypt': {
+'format':'luks',
+**encrypt_options
+}
+}
+
+result = self.vm.qmp('blockdev-add', **
+{
+'driver': iotests.imgfmt,
+'node-name': id,
+'read-only': read_only,
+
+**encrypt_options,
+
+'file': {
+'driver': 'file',
+'filename': test_img,
+}
+}
+)
+self.assert_qmp(result, 'return', {})
+
+# close the encrypted block device
+def closeImageQmp(self, id):
+result = self.vm.qmp('blockdev-del', **{ 'node-name': id })
+self.assert_qmp(result, 'return', {})
+
+###
+# add a key to an encrypted block device
+def addKeyQmp(self, id, new_secret, secret = None,
+  slot = None, force = False):
+
+crypt_options = {
+'state'  : 'active',
+'new-secret' :

[PATCH v6 12/14] block/crypto: implement blockdev-amend

2020-05-10 Thread Maxim Levitsky

Signed-off-by: Maxim Levitsky 
Reviewed-by: Daniel P. Berrangé 
---
 block/crypto.c   | 72 
 qapi/block-core.json | 14 -
 2 files changed, 66 insertions(+), 20 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index b14cb0ff06..61cbe45b22 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -791,32 +791,21 @@ block_crypto_get_specific_info_luks(BlockDriverState *bs, 
Error **errp)
 }
 
 static int
-block_crypto_amend_options_luks(BlockDriverState *bs,
-QemuOpts *opts,
-BlockDriverAmendStatusCB *status_cb,
-void *cb_opaque,
-bool force,
-Error **errp)
+block_crypto_amend_options_generic_luks(BlockDriverState *bs,
+QCryptoBlockAmendOptions 
*amend_options,
+bool force,
+Error **errp)
 {
 BlockCrypto *crypto = bs->opaque;
-QDict *cryptoopts = NULL;
-QCryptoBlockAmendOptions *amend_options = NULL;
 int ret;
 
 assert(crypto);
 assert(crypto->block);
-crypto->updating_keys = true;
 
+/* apply for exclusive read/write permissions to the underlying file*/
+crypto->updating_keys = true;
 ret = bdrv_child_refresh_perms(bs, bs->file, errp);
-if (ret < 0) {
-goto cleanup;
-}
-
-cryptoopts = qemu_opts_to_qdict(opts, NULL);
-qdict_put_str(cryptoopts, "format", "luks");
-amend_options = block_crypto_amend_opts_init(cryptoopts, errp);
-if (!amend_options) {
-ret = -EINVAL;
+if (ret) {
 goto cleanup;
 }
 
@@ -828,13 +817,57 @@ block_crypto_amend_options_luks(BlockDriverState *bs,
   force,
   errp);
 cleanup:
+/* release exclusive read/write permissions to the underlying file*/
 crypto->updating_keys = false;
 bdrv_child_refresh_perms(bs, bs->file, errp);
-qapi_free_QCryptoBlockAmendOptions(amend_options);
+return ret;
+}
+
+static int
+block_crypto_amend_options_luks(BlockDriverState *bs,
+QemuOpts *opts,
+BlockDriverAmendStatusCB *status_cb,
+void *cb_opaque,
+bool force,
+Error **errp)
+{
+BlockCrypto *crypto = bs->opaque;
+QDict *cryptoopts = NULL;
+QCryptoBlockAmendOptions *amend_options = NULL;
+int ret = -EINVAL;
+
+assert(crypto);
+assert(crypto->block);
+
+cryptoopts = qemu_opts_to_qdict(opts, NULL);
+qdict_put_str(cryptoopts, "format", "luks");
+amend_options = block_crypto_amend_opts_init(cryptoopts, errp);
 qobject_unref(cryptoopts);
+if (!amend_options) {
+goto cleanup;
+}
+ret = block_crypto_amend_options_generic_luks(bs, amend_options,
+  force, errp);
+cleanup:
+qapi_free_QCryptoBlockAmendOptions(amend_options);
 return ret;
 }
 
+static int
+coroutine_fn block_crypto_co_amend_luks(BlockDriverState *bs,
+BlockdevAmendOptions *opts,
+bool force,
+Error **errp)
+{
+QCryptoBlockAmendOptions amend_opts;
+
+amend_opts = (QCryptoBlockAmendOptions) {
+.format = Q_CRYPTO_BLOCK_FORMAT_LUKS,
+.u.luks = *qapi_BlockdevAmendOptionsLUKS_base(>u.luks),
+};
+return block_crypto_amend_options_generic_luks(bs, _opts,
+   force, errp);
+}
 
 static void
 block_crypto_child_perms(BlockDriverState *bs, BdrvChild *c,
@@ -907,6 +940,7 @@ static BlockDriver bdrv_crypto_luks = {
 .bdrv_get_info  = block_crypto_get_info_luks,
 .bdrv_get_specific_info = block_crypto_get_specific_info_luks,
 .bdrv_amend_options = block_crypto_amend_options_luks,
+.bdrv_co_amend  = block_crypto_co_amend_luks,
 
 .strong_runtime_opts = block_crypto_strong_runtime_opts,
 };
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 74db515414..df710fa597 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4649,6 +4649,18 @@
   'data': { 'job-id': 'str',
 'options': 'BlockdevCreateOptions' } }
 
+##
+# @BlockdevAmendOptionsLUKS:
+#
+# Driver specific image amend options for LUKS.
+#
+# Since: 5.1
+##
+{ 'struct': 'BlockdevAmendOptionsLUKS',
+  'base': 'QCryptoBlockAmendOptionsLUKS',
+  'data': { }
+}
+
 ##
 # @BlockdevAmendOptions:
 #
@@ -4663,7 +4675,7 @@
   'driver': 'BlockdevDriver' },
   'discriminator': 'driver',
   'data': {
-  } }
+  'luks':   'BlockdevAmendOptionsLUKS' } }
 
 ##
 # @x-blockdev-amend:
-- 
2.17.2

[PATCH v6 07/14] block/crypto: implement the encryption key management

2020-05-10 Thread Maxim Levitsky

This implements the encryption key management using the generic code in
qcrypto layer and exposes it to the user via qemu-img

This code adds another 'write_func' because the initialization
write_func works directly on the underlying file, and amend
works on instance of luks device.

This commit also adds a 'hack/workaround' I and Kevin Wolf (thanks)
made to make the driver both support write sharing (to avoid breaking the 
users),
and be safe against concurrent  metadata update (the keyslots)

Eventually the write sharing for luks driver will be deprecated
and removed together with this hack.

The hack is that we ask (as a format driver) for BLK_PERM_CONSISTENT_READ
and then when we want to update the keys, we unshare that permission.
So if someone else has the image open, even readonly, encryption
key update will fail gracefully.

Also thanks to Daniel Berrange for the idea of
unsharing read, rather that write permission which allows
to avoid cases when the other user had opened the image read-only.

Signed-off-by: Maxim Levitsky 
Reviewed-by: Daniel P. Berrangé 
---
 block/crypto.c | 127 +++--
 block/crypto.h |  34 +
 2 files changed, 158 insertions(+), 3 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index 2e16b62bdc..b14cb0ff06 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -37,6 +37,7 @@ typedef struct BlockCrypto BlockCrypto;
 
 struct BlockCrypto {
 QCryptoBlock *block;
+bool updating_keys;
 };
 
 
@@ -71,6 +72,24 @@ static ssize_t block_crypto_read_func(QCryptoBlock *block,
 return ret;
 }
 
+static ssize_t block_crypto_write_func(QCryptoBlock *block,
+   size_t offset,
+   const uint8_t *buf,
+   size_t buflen,
+   void *opaque,
+   Error **errp)
+{
+BlockDriverState *bs = opaque;
+ssize_t ret;
+
+ret = bdrv_pwrite(bs->file, offset, buf, buflen);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Could not write encryption header");
+return ret;
+}
+return ret;
+}
+
 
 struct BlockCryptoCreateData {
 BlockBackend *blk;
@@ -166,6 +185,19 @@ static QemuOptsList block_crypto_create_opts_luks = {
 };
 
 
+static QemuOptsList block_crypto_amend_opts_luks = {
+.name = "crypto",
+.head = QTAILQ_HEAD_INITIALIZER(block_crypto_create_opts_luks.head),
+.desc = {
+BLOCK_CRYPTO_OPT_DEF_LUKS_STATE(""),
+BLOCK_CRYPTO_OPT_DEF_LUKS_KEYSLOT(""),
+BLOCK_CRYPTO_OPT_DEF_LUKS_OLD_SECRET(""),
+BLOCK_CRYPTO_OPT_DEF_LUKS_NEW_SECRET(""),
+BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME(""),
+{ /* end of list */ }
+},
+};
+
 QCryptoBlockOpenOptions *
 block_crypto_open_opts_init(QDict *opts, Error **errp)
 {
@@ -758,6 +790,95 @@ block_crypto_get_specific_info_luks(BlockDriverState *bs, 
Error **errp)
 return spec_info;
 }
 
+static int
+block_crypto_amend_options_luks(BlockDriverState *bs,
+QemuOpts *opts,
+BlockDriverAmendStatusCB *status_cb,
+void *cb_opaque,
+bool force,
+Error **errp)
+{
+BlockCrypto *crypto = bs->opaque;
+QDict *cryptoopts = NULL;
+QCryptoBlockAmendOptions *amend_options = NULL;
+int ret;
+
+assert(crypto);
+assert(crypto->block);
+crypto->updating_keys = true;
+
+ret = bdrv_child_refresh_perms(bs, bs->file, errp);
+if (ret < 0) {
+goto cleanup;
+}
+
+cryptoopts = qemu_opts_to_qdict(opts, NULL);
+qdict_put_str(cryptoopts, "format", "luks");
+amend_options = block_crypto_amend_opts_init(cryptoopts, errp);
+if (!amend_options) {
+ret = -EINVAL;
+goto cleanup;
+}
+
+ret = qcrypto_block_amend_options(crypto->block,
+  block_crypto_read_func,
+  block_crypto_write_func,
+  bs,
+  amend_options,
+  force,
+  errp);
+cleanup:
+crypto->updating_keys = false;
+bdrv_child_refresh_perms(bs, bs->file, errp);
+qapi_free_QCryptoBlockAmendOptions(amend_options);
+qobject_unref(cryptoopts);
+return ret;
+}
+
+
+static void
+block_crypto_child_perms(BlockDriverState *bs, BdrvChild *c,
+ const BdrvChildRole *role,
+ BlockReopenQueue *reopen_queue,
+ uint64_t perm, uint64_t shared,
+ uint64_t *nperm, uint64_t *nshared)
+{
+
+BlockCrypto *crypto = bs->opaque;
+
+bdrv_filter_default_perms(bs, c, role, reopen_queue,
+perm, shared, nperm, nshared);
+/*
+ * Ask for

[PATCH v6 06/14] block/crypto: rename two functions

2020-05-10 Thread Maxim Levitsky

rename the write_func to create_write_func, and init_func to create_init_func.
This is preparation for other write_func that will be used to update the 
encryption keys.

No functional changes

Signed-off-by: Maxim Levitsky 
Reviewed-by: Daniel P. Berrangé 
---
 block/crypto.c | 25 -
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index 9d95329d7f..2e16b62bdc 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -79,12 +79,12 @@ struct BlockCryptoCreateData {
 };
 
 
-static ssize_t block_crypto_write_func(QCryptoBlock *block,
-   size_t offset,
-   const uint8_t *buf,
-   size_t buflen,
-   void *opaque,
-   Error **errp)
+static ssize_t block_crypto_create_write_func(QCryptoBlock *block,
+  size_t offset,
+  const uint8_t *buf,
+  size_t buflen,
+  void *opaque,
+  Error **errp)
 {
 struct BlockCryptoCreateData *data = opaque;
 ssize_t ret;
@@ -97,11 +97,10 @@ static ssize_t block_crypto_write_func(QCryptoBlock *block,
 return ret;
 }
 
-
-static ssize_t block_crypto_init_func(QCryptoBlock *block,
-  size_t headerlen,
-  void *opaque,
-  Error **errp)
+static ssize_t block_crypto_create_init_func(QCryptoBlock *block,
+ size_t headerlen,
+ void *opaque,
+ Error **errp)
 {
 struct BlockCryptoCreateData *data = opaque;
 Error *local_error = NULL;
@@ -313,8 +312,8 @@ static int block_crypto_co_create_generic(BlockDriverState 
*bs,
 };
 
 crypto = qcrypto_block_create(opts, NULL,
-  block_crypto_init_func,
-  block_crypto_write_func,
+  block_crypto_create_init_func,
+  block_crypto_create_write_func,
   ,
   errp);
 
-- 
2.17.2

[PATCH v6 05/14] block/amend: refactor qcow2 amend options

2020-05-10 Thread Maxim Levitsky

Some qcow2 create options can't be used for amend.
Remove them from the qcow2 create options and add generic logic to detect
such options in qemu-img

Signed-off-by: Maxim Levitsky 
Reviewed-by: Daniel P. Berrangé 
---
 block/qcow2.c  | 108 ++---
 qemu-img.c |  18 +++-
 tests/qemu-iotests/049.out | 102 ++--
 tests/qemu-iotests/061.out |  12 ++-
 tests/qemu-iotests/079.out |  18 ++--
 tests/qemu-iotests/082.out | 149 
 tests/qemu-iotests/085.out |  38 
 tests/qemu-iotests/087.out |   6 +-
 tests/qemu-iotests/115.out |   2 +-
 tests/qemu-iotests/121.out |   4 +-
 tests/qemu-iotests/125.out | 192 ++---
 tests/qemu-iotests/134.out |   2 +-
 tests/qemu-iotests/144.out |   4 +-
 tests/qemu-iotests/158.out |   4 +-
 tests/qemu-iotests/182.out |   2 +-
 tests/qemu-iotests/185.out |   8 +-
 tests/qemu-iotests/188.out |   2 +-
 tests/qemu-iotests/189.out |   4 +-
 tests/qemu-iotests/198.out |   4 +-
 tests/qemu-iotests/243.out |  16 ++--
 tests/qemu-iotests/250.out |   2 +-
 tests/qemu-iotests/255.out |   8 +-
 tests/qemu-iotests/259.out |   2 +-
 tests/qemu-iotests/263.out |   4 +-
 tests/qemu-iotests/280.out |   2 +-
 25 files changed, 284 insertions(+), 429 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index fc494c7591..db86500839 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2971,17 +2971,6 @@ static int qcow2_change_backing_file(BlockDriverState 
*bs,
 return qcow2_update_header(bs);
 }
 
-static int qcow2_crypt_method_from_format(const char *encryptfmt)
-{
-if (g_str_equal(encryptfmt, "luks")) {
-return QCOW_CRYPT_LUKS;
-} else if (g_str_equal(encryptfmt, "aes")) {
-return QCOW_CRYPT_AES;
-} else {
-return -EINVAL;
-}
-}
-
 static int qcow2_set_up_encryption(BlockDriverState *bs,
QCryptoBlockCreateOptions *cryptoopts,
Error **errp)
@@ -5237,9 +5226,6 @@ static int qcow2_amend_options(BlockDriverState *bs, 
QemuOpts *opts,
 bool lazy_refcounts = s->use_lazy_refcounts;
 bool data_file_raw = data_file_is_raw(bs);
 const char *compat = NULL;
-uint64_t cluster_size = s->cluster_size;
-bool encrypt;
-int encformat;
 int refcount_bits = s->refcount_bits;
 int ret;
 QemuOptDesc *desc = opts->list->desc;
@@ -5264,44 +5250,12 @@ static int qcow2_amend_options(BlockDriverState *bs, 
QemuOpts *opts,
 error_setg(errp, "Unknown compatibility level %s", compat);
 return -EINVAL;
 }
-} else if (!strcmp(desc->name, BLOCK_OPT_PREALLOC)) {
-error_setg(errp, "Cannot change preallocation mode");
-return -ENOTSUP;
 } else if (!strcmp(desc->name, BLOCK_OPT_SIZE)) {
 new_size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
 } else if (!strcmp(desc->name, BLOCK_OPT_BACKING_FILE)) {
 backing_file = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE);
 } else if (!strcmp(desc->name, BLOCK_OPT_BACKING_FMT)) {
 backing_format = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT);
-} else if (!strcmp(desc->name, BLOCK_OPT_ENCRYPT)) {
-encrypt = qemu_opt_get_bool(opts, BLOCK_OPT_ENCRYPT,
-!!s->crypto);
-
-if (encrypt != !!s->crypto) {
-error_setg(errp,
-   "Changing the encryption flag is not supported");
-return -ENOTSUP;
-}
-} else if (!strcmp(desc->name, BLOCK_OPT_ENCRYPT_FORMAT)) {
-encformat = qcow2_crypt_method_from_format(
-qemu_opt_get(opts, BLOCK_OPT_ENCRYPT_FORMAT));
-
-if (encformat != s->crypt_method_header) {
-error_setg(errp,
-   "Changing the encryption format is not supported");
-return -ENOTSUP;
-}
-} else if (g_str_has_prefix(desc->name, "encrypt.")) {
-error_setg(errp,
-   "Changing the encryption parameters is not supported");
-return -ENOTSUP;
-} else if (!strcmp(desc->name, BLOCK_OPT_CLUSTER_SIZE)) {
-cluster_size = qemu_opt_get_size(opts, BLOCK_OPT_CLUSTER_SIZE,
- cluster_size);
-if (cluster_size != s->cluster_size) {
-error_setg(errp, "Changing the cluster size is not supported");
-return -ENOTSUP;
-}
 } else if (!strcmp(desc->name, BLOCK_OPT_LAZY_REFCOUNTS)) {
 lazy_refcounts = qemu_opt_get_bool(opts, BLOCK_OPT_LAZY_REFCOUNTS,
lazy_refcounts);
@@ -5552,37 +5506,6 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool 
fatal, int64_t offset,
 .help = "The external data file must stay valid "   \

[PATCH v6 09/14] iotests: filter few more luks specific create options

2020-05-10 Thread Maxim Levitsky

This allows more tests to be able to have same output on both qcow2 luks 
encrypted images
and raw luks images

Signed-off-by: Maxim Levitsky 
Reviewed-by: Daniel P. Berrangé 
---
 tests/qemu-iotests/087.out   |  6 ++---
 tests/qemu-iotests/134.out   |  2 +-
 tests/qemu-iotests/158.out   |  4 +--
 tests/qemu-iotests/188.out   |  2 +-
 tests/qemu-iotests/189.out   |  4 +--
 tests/qemu-iotests/198.out   |  4 +--
 tests/qemu-iotests/263.out   |  4 +--
 tests/qemu-iotests/274.out   | 46 
 tests/qemu-iotests/284.out   |  6 ++---
 tests/qemu-iotests/common.filter |  6 +++--
 10 files changed, 43 insertions(+), 41 deletions(-)

diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out
index f23bffbbf1..d5ff53302e 100644
--- a/tests/qemu-iotests/087.out
+++ b/tests/qemu-iotests/087.out
@@ -34,7 +34,7 @@ QMP_VERSION
 
 === Encrypted image QCow ===
 
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT encryption=on 
encrypt.key-secret=sec0 size=134217728
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT encryption=on size=134217728
 Testing:
 QMP_VERSION
 {"return": {}}
@@ -46,7 +46,7 @@ QMP_VERSION
 
 === Encrypted image LUKS ===
 
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT encrypt.format=luks 
encrypt.key-secret=sec0 size=134217728
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
 Testing:
 QMP_VERSION
 {"return": {}}
@@ -58,7 +58,7 @@ QMP_VERSION
 
 === Missing driver ===
 
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT encryption=on 
encrypt.key-secret=sec0 size=134217728
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT encryption=on size=134217728
 Testing: -S
 QMP_VERSION
 {"return": {}}
diff --git a/tests/qemu-iotests/134.out b/tests/qemu-iotests/134.out
index f2878f5f3a..e4733c0b81 100644
--- a/tests/qemu-iotests/134.out
+++ b/tests/qemu-iotests/134.out
@@ -1,5 +1,5 @@
 QA output created by 134
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT encryption=on 
encrypt.key-secret=sec0 size=134217728
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT encryption=on size=134217728
 
 == reading whole image ==
 read 134217728/134217728 bytes at offset 0
diff --git a/tests/qemu-iotests/158.out b/tests/qemu-iotests/158.out
index fa2294bb85..52ea9a488f 100644
--- a/tests/qemu-iotests/158.out
+++ b/tests/qemu-iotests/158.out
@@ -1,6 +1,6 @@
 QA output created by 158
 == create base ==
-Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT encryption=on 
encrypt.key-secret=sec0 size=134217728
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT encryption=on size=134217728
 
 == writing whole image ==
 wrote 134217728/134217728 bytes at offset 0
@@ -10,7 +10,7 @@ wrote 134217728/134217728 bytes at offset 0
 read 134217728/134217728 bytes at offset 0
 128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 == create overlay ==
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT encryption=on 
encrypt.key-secret=sec0 size=134217728 backing_file=TEST_DIR/t.IMGFMT.base
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT encryption=on size=134217728 
backing_file=TEST_DIR/t.IMGFMT.base
 
 == writing part of a cluster ==
 wrote 1024/1024 bytes at offset 0
diff --git a/tests/qemu-iotests/188.out b/tests/qemu-iotests/188.out
index 4b9aadd51c..5426861b18 100644
--- a/tests/qemu-iotests/188.out
+++ b/tests/qemu-iotests/188.out
@@ -1,5 +1,5 @@
 QA output created by 188
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT encrypt.format=luks 
encrypt.key-secret=sec0 encrypt.iter-time=10 size=16777216
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=16777216
 
 == reading whole image ==
 read 16777216/16777216 bytes at offset 0
diff --git a/tests/qemu-iotests/189.out b/tests/qemu-iotests/189.out
index e536d95d53..bc213cbe14 100644
--- a/tests/qemu-iotests/189.out
+++ b/tests/qemu-iotests/189.out
@@ -1,6 +1,6 @@
 QA output created by 189
 == create base ==
-Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT encrypt.format=luks 
encrypt.key-secret=sec0 encrypt.iter-time=10 size=16777216
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=16777216
 
 == writing whole image ==
 wrote 16777216/16777216 bytes at offset 0
@@ -10,7 +10,7 @@ wrote 16777216/16777216 bytes at offset 0
 read 16777216/16777216 bytes at offset 0
 16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 == create overlay ==
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT encrypt.format=luks 
encrypt.key-secret=sec1 encrypt.iter-time=10 size=16777216 
backing_file=TEST_DIR/t.IMGFMT.base
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=16777216 
backing_file=TEST_DIR/t.IMGFMT.base
 
 == writing part of a cluster ==
 wrote 1024/1024 bytes at offset 0
diff --git a/tests/qemu-iotests/198.out b/tests/qemu-iotests/198.out
index b0f2d417af..acfdf96b0c 100644
--- a/tests/qemu-iotests/198.out
+++ b/tests/qemu-iotests/198.out
@@ -1,12 +1,12 @@
 QA output created by 198
 == create base ==
-Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT encrypt.format=luks 
encrypt.key-secret=sec0 encrypt.iter-time=10 size=16777216
+Formatting 'TEST_DIR/t.IMGFMT.base',

[PATCH v6 08/14] block/qcow2: extend qemu-img amend interface with crypto options

2020-05-10 Thread Maxim Levitsky

Now that we have all the infrastructure in place,
wire it in the qcow2 driver and expose this to the user.

Signed-off-by: Maxim Levitsky 
Reviewed-by: Daniel P. Berrangé 
---
 block/qcow2.c  | 72 +-
 tests/qemu-iotests/082.out | 45 
 2 files changed, 108 insertions(+), 9 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index db86500839..4bb6e3fc8f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -176,6 +176,19 @@ static ssize_t qcow2_crypto_hdr_write_func(QCryptoBlock 
*block, size_t offset,
 return ret;
 }
 
+static QDict*
+qcow2_extract_crypto_opts(QemuOpts *opts, const char *fmt, Error **errp)
+{
+QDict *cryptoopts_qdict;
+QDict *opts_qdict;
+
+/* Extract "encrypt." options into a qdict */
+opts_qdict = qemu_opts_to_qdict(opts, NULL);
+qdict_extract_subqdict(opts_qdict, _qdict, "encrypt.");
+qobject_unref(opts_qdict);
+qdict_put_str(cryptoopts_qdict, "format", fmt);
+return cryptoopts_qdict;
+}
 
 /*
  * read qcow2 extension and fill bs
@@ -4744,17 +4757,11 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, 
BlockDriverState *in_bs,
 g_free(optstr);
 
 if (has_luks) {
+
 g_autoptr(QCryptoBlockCreateOptions) create_opts = NULL;
-QDict *opts_qdict;
-QDict *cryptoopts;
+QDict *cryptoopts = qcow2_extract_crypto_opts(opts, "luks", errp);
 size_t headerlen;
 
-opts_qdict = qemu_opts_to_qdict(opts, NULL);
-qdict_extract_subqdict(opts_qdict, , "encrypt.");
-qobject_unref(opts_qdict);
-
-qdict_put_str(cryptoopts, "format", "luks");
-
 create_opts = block_crypto_create_opts_init(cryptoopts, errp);
 qobject_unref(cryptoopts);
 if (!create_opts) {
@@ -5149,6 +5156,7 @@ typedef enum Qcow2AmendOperation {
 QCOW2_NO_OPERATION = 0,
 
 QCOW2_UPGRADING,
+QCOW2_UPDATING_ENCRYPTION,
 QCOW2_CHANGING_REFCOUNT_ORDER,
 QCOW2_DOWNGRADING,
 } Qcow2AmendOperation;
@@ -5230,6 +5238,7 @@ static int qcow2_amend_options(BlockDriverState *bs, 
QemuOpts *opts,
 int ret;
 QemuOptDesc *desc = opts->list->desc;
 Qcow2AmendHelperCBInfo helper_cb_info;
+bool encryption_update = false;
 
 while (desc && desc->name) {
 if (!qemu_opt_find(opts, desc->name)) {
@@ -5256,6 +5265,18 @@ static int qcow2_amend_options(BlockDriverState *bs, 
QemuOpts *opts,
 backing_file = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE);
 } else if (!strcmp(desc->name, BLOCK_OPT_BACKING_FMT)) {
 backing_format = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT);
+} else if (g_str_has_prefix(desc->name, "encrypt.")) {
+if (!s->crypto) {
+error_setg(errp,
+   "Can't amend encryption options - encryption not 
present");
+return -EINVAL;
+}
+if (s->crypt_method_header != QCOW_CRYPT_LUKS) {
+error_setg(errp,
+   "Only LUKS encryption options can be amended");
+return -ENOTSUP;
+}
+encryption_update = true;
 } else if (!strcmp(desc->name, BLOCK_OPT_LAZY_REFCOUNTS)) {
 lazy_refcounts = qemu_opt_get_bool(opts, BLOCK_OPT_LAZY_REFCOUNTS,
lazy_refcounts);
@@ -5298,7 +5319,8 @@ static int qcow2_amend_options(BlockDriverState *bs, 
QemuOpts *opts,
 .original_status_cb = status_cb,
 .original_cb_opaque = cb_opaque,
 .total_operations = (new_version != old_version)
-  + (s->refcount_bits != refcount_bits)
+  + (s->refcount_bits != refcount_bits) +
+(encryption_update == true)
 };
 
 /* Upgrade first (some features may require compat=1.1) */
@@ -5311,6 +5333,33 @@ static int qcow2_amend_options(BlockDriverState *bs, 
QemuOpts *opts,
 }
 }
 
+if (encryption_update) {
+QDict *amend_opts_dict;
+QCryptoBlockAmendOptions *amend_opts;
+
+helper_cb_info.current_operation = QCOW2_UPDATING_ENCRYPTION;
+amend_opts_dict = qcow2_extract_crypto_opts(opts, "luks", errp);
+if (!amend_opts_dict) {
+return -EINVAL;
+}
+amend_opts = block_crypto_amend_opts_init(amend_opts_dict, errp);
+qobject_unref(amend_opts_dict);
+if (!amend_opts) {
+return -EINVAL;
+}
+ret = qcrypto_block_amend_options(s->crypto,
+  qcow2_crypto_hdr_read_func,
+  qcow2_crypto_hdr_write_func,
+  bs,
+  amend_opts,
+  force,
+  errp);
+qapi_free_QCryptoBlockAmendOptions(amend_opts);
+if (ret < 0) {
+

[PATCH v6 04/14] block/amend: separate amend and create options for qemu-img

2020-05-10 Thread Maxim Levitsky

Some options are only useful for creation
(or hard to be amended, like cluster size for qcow2), while some other
options are only useful for amend, like upcoming keyslot management
options for luks

Since currently only qcow2 supports amend, move all its options
to a common macro and then include it in each action option list.

In future it might be useful to remove some options which are
not supported anyway from amend list, which currently
cause an error message if amended.

Signed-off-by: Maxim Levitsky 
Reviewed-by: Daniel P. Berrangé 
---
 block/qcow2.c | 160 +-
 include/block/block_int.h |   4 +
 qemu-img.c|  18 ++---
 3 files changed, 100 insertions(+), 82 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 79fbad9d76..fc494c7591 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5520,83 +5520,96 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool 
fatal, int64_t offset,
 s->signaled_corruption = true;
 }
 
+#define QCOW_COMMON_OPTIONS \
+{   \
+.name = BLOCK_OPT_SIZE, \
+.type = QEMU_OPT_SIZE,  \
+.help = "Virtual disk size" \
+},  \
+{   \
+.name = BLOCK_OPT_COMPAT_LEVEL, \
+.type = QEMU_OPT_STRING,\
+.help = "Compatibility level (v2 [0.10] or v3 [1.1])"   \
+},  \
+{   \
+.name = BLOCK_OPT_BACKING_FILE, \
+.type = QEMU_OPT_STRING,\
+.help = "File name of a base image" \
+},  \
+{   \
+.name = BLOCK_OPT_BACKING_FMT,  \
+.type = QEMU_OPT_STRING,\
+.help = "Image format of the base image"\
+},  \
+{   \
+.name = BLOCK_OPT_DATA_FILE,\
+.type = QEMU_OPT_STRING,\
+.help = "File name of an external data file"\
+},  \
+{   \
+.name = BLOCK_OPT_DATA_FILE_RAW,\
+.type = QEMU_OPT_BOOL,  \
+.help = "The external data file must stay valid "   \
+"as a raw image"\
+},  \
+{   \
+.name = BLOCK_OPT_ENCRYPT,  \
+.type = QEMU_OPT_BOOL,  \
+.help = "Encrypt the image with format 'aes'. (Deprecated " \
+"in favor of " BLOCK_OPT_ENCRYPT_FORMAT "=aes)",\
+},  \
+{   \
+.name = BLOCK_OPT_ENCRYPT_FORMAT,   \
+.type = QEMU_OPT_STRING,\
+.help = "Encrypt the image, format choices: 'aes', 'luks'", \
+},  \
+BLOCK_CRYPTO_OPT_DEF_KEY_SECRET("encrypt.", \
+"ID of secret providing qcow AES key or LUKS passphrase"),  \
+BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG("encrypt."),   \
+BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE("encrypt."),  \
+BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG("encrypt."),\
+BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG("encrypt."),   \
+BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG("encrypt."), \
+BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME("encrypt."),\
+{   \
+.name = BLOCK_OPT_CLUSTER_SIZE, \
+.type = QEMU_OPT_SIZE,  \
+.help = "qcow2 cluster size",   \
+.def_value_str = stringify(DEFAULT_CLUSTER_SIZE)\
+},

[PATCH v6 10/14] iotests: qemu-img tests for luks key management

2020-05-10 Thread Maxim Levitsky

This commit adds two tests, which test the new amend interface
of both luks raw images and qcow2 luks encrypted images.

Signed-off-by: Maxim Levitsky 
Reviewed-by: Daniel P. Berrangé 
---
 tests/qemu-iotests/293 | 207 +
 tests/qemu-iotests/293.out |  99 ++
 tests/qemu-iotests/294 |  90 
 tests/qemu-iotests/294.out |  30 ++
 tests/qemu-iotests/group   |   2 +
 5 files changed, 428 insertions(+)
 create mode 100755 tests/qemu-iotests/293
 create mode 100644 tests/qemu-iotests/293.out
 create mode 100755 tests/qemu-iotests/294
 create mode 100644 tests/qemu-iotests/294.out

diff --git a/tests/qemu-iotests/293 b/tests/qemu-iotests/293
new file mode 100755
index 00..aa1a77690f
--- /dev/null
+++ b/tests/qemu-iotests/293
@@ -0,0 +1,207 @@
+#!/usr/bin/env bash
+#
+# Test encryption key management with luks
+# Based on 134
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=mlevi...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2 luks
+_supported_proto file #TODO
+
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
+
+if [ "$IMGFMT" = "qcow2" ] ; then
+   PR="encrypt."
+   EXTRA_IMG_ARGS="-o encrypt.format=luks"
+fi
+
+
+# secrets: you are supposed to see the password as ***, see :-)
+S0="--object secret,id=sec0,data=hunter0"
+S1="--object secret,id=sec1,data=hunter1"
+S2="--object secret,id=sec2,data=hunter2"
+S3="--object secret,id=sec3,data=hunter3"
+S4="--object secret,id=sec4,data=hunter4"
+SECRETS="$S0 $S1 $S2 $S3 $S4"
+
+# image with given secret
+IMGS0="--image-opts 
driver=$IMGFMT,file.filename=$TEST_IMG,${PR}key-secret=sec0"
+IMGS1="--image-opts 
driver=$IMGFMT,file.filename=$TEST_IMG,${PR}key-secret=sec1"
+IMGS2="--image-opts 
driver=$IMGFMT,file.filename=$TEST_IMG,${PR}key-secret=sec2"
+IMGS3="--image-opts 
driver=$IMGFMT,file.filename=$TEST_IMG,${PR}key-secret=sec3"
+IMGS4="--image-opts 
driver=$IMGFMT,file.filename=$TEST_IMG,${PR}key-secret=sec4"
+
+
+echo "== creating a test image =="
+_make_test_img $S0 $EXTRA_IMG_ARGS -o ${PR}key-secret=sec0,${PR}iter-time=10 
32M
+
+echo
+echo "== test that key 0 opens the image =="
+$QEMU_IO $S0 -c "read 0 4096" $IMGS0 | _filter_qemu_io | _filter_testdir
+
+echo
+echo "== adding a password to slot 4 =="
+$QEMU_IMG amend $SECRETS $IMGS0 -o 
${PR}state=active,${PR}new-secret=sec4,${PR}iter-time=10,${PR}keyslot=4
+echo "== adding a password to slot 1 =="
+$QEMU_IMG amend $SECRETS $IMGS0 -o 
${PR}state=active,${PR}new-secret=sec1,${PR}iter-time=10
+echo "== adding a password to slot 3 =="
+$QEMU_IMG amend $SECRETS $IMGS1 -o 
${PR}state=active,${PR}new-secret=sec3,${PR}iter-time=10,${PR}keyslot=3
+
+echo "== adding a password to slot 2 =="
+$QEMU_IMG amend $SECRETS $IMGS3 -o 
${PR}state=active,${PR}new-secret=sec2,${PR}iter-time=10
+
+
+echo "== erase slot 4 =="
+$QEMU_IMG amend $SECRETS $IMGS1 -o ${PR}state=inactive,${PR}keyslot=4 | 
_filter_img_create
+
+
+echo
+echo "== all secrets should work =="
+for IMG in "$IMGS0" "$IMGS1" "$IMGS2" "$IMGS3"; do
+   $QEMU_IO $SECRETS -c "read 0 4096" $IMG | _filter_qemu_io | 
_filter_testdir
+done
+
+echo
+echo "== erase slot 0 and try it =="
+$QEMU_IMG amend $SECRETS $IMGS1 -o ${PR}state=inactive,${PR}old-secret=sec0 | 
_filter_img_create
+$QEMU_IO $SECRETS -c "read 0 4096" $IMGS0 | _filter_qemu_io | _filter_testdir
+
+echo
+echo "== erase slot 2 and try it =="
+$QEMU_IMG amend $SECRETS $IMGS1 -o ${PR}state=inactive,${PR}keyslot=2 | 
_filter_img_create
+$QEMU_IO $SECRETS -c "read 0 4096" $IMGS2 | _filter_qemu_io | _filter_testdir
+
+
+# at this point slots 1 and 3 should be active
+
+echo
+echo "== filling  4 slots with secret 2 =="
+for i in $(seq 0 3) ; do
+   $QEMU_IMG amend $SECRETS $IMGS3 -o 
${PR}state=active,${PR}new-secret=sec2,${PR}iter-time=10
+done
+
+echo
+echo "== adding secret 0 =="
+   $QEMU_IMG amend $SECRETS $IMGS3 -o 
${PR}state=active,${PR}new-secret=sec0,${PR}iter-time=10
+
+echo
+echo "== adding secret 3 (last slot) =="
+   $QEMU_IMG amend $SECRETS $IMGS3 -o

[PATCH v6 11/14] block/core: add generic infrastructure for x-blockdev-amend qmp command

2020-05-10 Thread Maxim Levitsky

blockdev-amend will be used similiar to blockdev-create
to allow on the fly changes of the structure of the format based block devices.

Current plan is to first support encryption keyslot management for luks
based formats (raw and embedded in qcow2)

Signed-off-by: Maxim Levitsky 
Reviewed-by: Daniel P. Berrangé 
---
 block/Makefile.objs   |   2 +-
 block/amend.c | 108 ++
 include/block/block_int.h |  21 +---
 qapi/block-core.json  |  42 +++
 qapi/job.json |   4 +-
 5 files changed, 169 insertions(+), 8 deletions(-)
 create mode 100644 block/amend.c

diff --git a/block/Makefile.objs b/block/Makefile.objs
index 3635b6b4c1..a0988638d5 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -19,7 +19,7 @@ block-obj-$(CONFIG_WIN32) += file-win32.o win32-aio.o
 block-obj-$(CONFIG_POSIX) += file-posix.o
 block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
 block-obj-$(CONFIG_LINUX_IO_URING) += io_uring.o
-block-obj-y += null.o mirror.o commit.o io.o create.o
+block-obj-y += null.o mirror.o commit.o io.o create.o amend.o
 block-obj-y += throttle-groups.o
 block-obj-$(CONFIG_LINUX) += nvme.o
 
diff --git a/block/amend.c b/block/amend.c
new file mode 100644
index 00..4840c0ffef
--- /dev/null
+++ b/block/amend.c
@@ -0,0 +1,108 @@
+/*
+ * Block layer code related to image options amend
+ *
+ * Copyright (c) 2018 Kevin Wolf 
+ * Copyright (c) 2020 Red Hat. Inc
+ *
+ * Heavily based on create.c
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "block/block_int.h"
+#include "qemu/job.h"
+#include "qemu/main-loop.h"
+#include "qapi/qapi-commands-block-core.h"
+#include "qapi/qapi-visit-block-core.h"
+#include "qapi/clone-visitor.h"
+#include "qapi/error.h"
+
+typedef struct BlockdevAmendJob {
+Job common;
+BlockdevAmendOptions *opts;
+BlockDriverState *bs;
+bool force;
+} BlockdevAmendJob;
+
+static int coroutine_fn blockdev_amend_run(Job *job, Error **errp)
+{
+BlockdevAmendJob *s = container_of(job, BlockdevAmendJob, common);
+int ret;
+
+job_progress_set_remaining(>common, 1);
+ret = s->bs->drv->bdrv_co_amend(s->bs, s->opts, s->force, errp);
+job_progress_update(>common, 1);
+qapi_free_BlockdevAmendOptions(s->opts);
+return ret;
+}
+
+static const JobDriver blockdev_amend_job_driver = {
+.instance_size = sizeof(BlockdevAmendJob),
+.job_type  = JOB_TYPE_AMEND,
+.run   = blockdev_amend_run,
+};
+
+void qmp_x_blockdev_amend(const char *job_id,
+  const char *node_name,
+  BlockdevAmendOptions *options,
+  bool has_force,
+  bool force,
+  Error **errp)
+{
+BlockdevAmendJob *s;
+const char *fmt = BlockdevDriver_str(options->driver);
+BlockDriver *drv = bdrv_find_format(fmt);
+BlockDriverState *bs = bdrv_find_node(node_name);
+
+/*
+ * If the driver is in the schema, we know that it exists. But it may not
+ * be whitelisted.
+ */
+assert(drv);
+if (bdrv_uses_whitelist() && !bdrv_is_whitelisted(drv, false)) {
+error_setg(errp, "Driver is not whitelisted");
+return;
+}
+
+if (bs->drv != drv) {
+error_setg(errp,
+   "x-blockdev-amend doesn't support changing the block 
driver");
+return;
+}
+
+/* Error out if the driver doesn't support .bdrv_co_amend */
+if (!drv->bdrv_co_amend) {
+error_setg(errp, "Driver does not support x-blockdev-amend");
+return;
+}
+
+/* Create the block job */
+s = job_create(job_id, _amend_job_driver, NULL,
+   bdrv_get_aio_context(bs), JOB_DEFAULT | JOB_MANUAL_DISMISS,
+   NULL, NULL, errp);
+if (!s) {
+return;
+}
+
+s->bs = bs,
+s->opts =

[PATCH v6 13/14] block/qcow2: implement blockdev-amend

2020-05-10 Thread Maxim Levitsky

Currently the implementation only supports amending the encryption
options, unlike the qemu-img version

Signed-off-by: Maxim Levitsky 
Reviewed-by: Daniel P. Berrangé 
---
 block/qcow2.c| 39 +++
 qapi/block-core.json | 16 +++-
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 4bb6e3fc8f..90fe43918f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5473,6 +5473,44 @@ static int qcow2_amend_options(BlockDriverState *bs, 
QemuOpts *opts,
 return 0;
 }
 
+static int coroutine_fn qcow2_co_amend(BlockDriverState *bs,
+   BlockdevAmendOptions *opts,
+   bool force,
+   Error **errp)
+{
+BlockdevAmendOptionsQcow2 *qopts = >u.qcow2;
+BDRVQcow2State *s = bs->opaque;
+int ret = 0;
+
+if (qopts->has_encrypt) {
+if (!s->crypto) {
+error_setg(errp, "image is not encrypted, can't amend");
+return -EOPNOTSUPP;
+}
+
+if (qopts->encrypt->format != Q_CRYPTO_BLOCK_FORMAT_LUKS) {
+error_setg(errp,
+   "Amend can't be used to change the qcow2 encryption 
format");
+return -EOPNOTSUPP;
+}
+
+if (s->crypt_method_header != QCOW_CRYPT_LUKS) {
+error_setg(errp,
+   "Only LUKS encryption options can be amended for qcow2 
with blockdev-amend");
+return -EOPNOTSUPP;
+}
+
+ret = qcrypto_block_amend_options(s->crypto,
+  qcow2_crypto_hdr_read_func,
+  qcow2_crypto_hdr_write_func,
+  bs,
+  qopts->encrypt,
+  force,
+  errp);
+}
+return ret;
+}
+
 /*
  * If offset or size are negative, respectively, they will not be included in
  * the BLOCK_IMAGE_CORRUPTED event emitted.
@@ -5682,6 +5720,7 @@ BlockDriver bdrv_qcow2 = {
 .mutable_opts= mutable_opts,
 .bdrv_co_check   = qcow2_co_check,
 .bdrv_amend_options  = qcow2_amend_options,
+.bdrv_co_amend   = qcow2_co_amend,
 
 .bdrv_detach_aio_context  = qcow2_detach_aio_context,
 .bdrv_attach_aio_context  = qcow2_attach_aio_context,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index df710fa597..0faff6c675 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4661,6 +4661,19 @@
   'data': { }
 }
 
+##
+# @BlockdevAmendOptionsQcow2:
+#
+# Driver specific image amend options for qcow2.
+# For now, only encryption options can be amended
+#
+# @encrypt  Encryption options to be amended
+#
+# Since: 5.1
+##
+{ 'struct': 'BlockdevAmendOptionsQcow2',
+  'data': { '*encrypt': 'QCryptoBlockAmendOptions' } }
+
 ##
 # @BlockdevAmendOptions:
 #
@@ -4675,7 +4688,8 @@
   'driver': 'BlockdevDriver' },
   'discriminator': 'driver',
   'data': {
-  'luks':   'BlockdevAmendOptionsLUKS' } }
+  'luks':   'BlockdevAmendOptionsLUKS',
+  'qcow2':  'BlockdevAmendOptionsQcow2' } }
 
 ##
 # @x-blockdev-amend:
-- 
2.17.2

[PATCH v6 03/14] block/amend: add 'force' option

2020-05-10 Thread Maxim Levitsky

'force' option will be used for some unsafe amend operations.

This includes things like erasing last keyslot in luks based formats
(which destroys the data, unless the master key is backed up
by external means), but that _might_ be desired result.

Signed-off-by: Maxim Levitsky 
Reviewed-by: Daniel P. Berrangé 
---
 block.c   | 4 +++-
 block/qcow2.c | 1 +
 docs/tools/qemu-img.rst   | 5 -
 include/block/block.h | 1 +
 include/block/block_int.h | 1 +
 qemu-img-cmds.hx  | 4 ++--
 qemu-img.c| 8 +++-
 7 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 0653ccb913..94a6e851b4 100644
--- a/block.c
+++ b/block.c
@@ -6356,6 +6356,7 @@ void bdrv_remove_aio_context_notifier(BlockDriverState 
*bs,
 
 int bdrv_amend_options(BlockDriverState *bs, QemuOpts *opts,
BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
+   bool force,
Error **errp)
 {
 if (!bs->drv) {
@@ -6367,7 +6368,8 @@ int bdrv_amend_options(BlockDriverState *bs, QemuOpts 
*opts,
bs->drv->format_name);
 return -ENOTSUP;
 }
-return bs->drv->bdrv_amend_options(bs, opts, status_cb, cb_opaque, errp);
+return bs->drv->bdrv_amend_options(bs, opts, status_cb,
+   cb_opaque, force, errp);
 }
 
 /*
diff --git a/block/qcow2.c b/block/qcow2.c
index 1ad95ff048..79fbad9d76 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5227,6 +5227,7 @@ static void qcow2_amend_helper_cb(BlockDriverState *bs,
 static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
BlockDriverAmendStatusCB *status_cb,
void *cb_opaque,
+   bool force,
Error **errp)
 {
 BDRVQcow2State *s = bs->opaque;
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 0080f83a76..fc2dca6649 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -249,11 +249,14 @@ Command description:
 
 .. program:: qemu-img-commands
 
-.. option:: amend [--object OBJECTDEF] [--image-opts] [-p] [-q] [-f FMT] [-t 
CACHE] -o OPTIONS FILENAME
+.. option:: amend [--object OBJECTDEF] [--image-opts] [-p] [-q] [-f FMT] [-t 
CACHE] [--force] -o OPTIONS FILENAME
 
   Amends the image format specific *OPTIONS* for the image file
   *FILENAME*. Not all file formats support this operation.
 
+  --force allows some unsafe operations. Currently for -f luks, it allows to
+  erase last encryption key, and to overwrite an active encryption key.
+
 .. option:: bench [-c COUNT] [-d DEPTH] [-f FMT] 
[--flush-interval=FLUSH_INTERVAL] [-i AIO] [-n] [--no-drain] [-o OFFSET] 
[--pattern=PATTERN] [-q] [-s BUFFER_SIZE] [-S STEP_SIZE] [-t CACHE] [-w] [-U] 
FILENAME
 
   Run a simple sequential I/O benchmark on the specified image. If ``-w`` is
diff --git a/include/block/block.h b/include/block/block.h
index 4de8d8f8a6..5b44a838d9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -392,6 +392,7 @@ typedef void BlockDriverAmendStatusCB(BlockDriverState *bs, 
int64_t offset,
   int64_t total_work_size, void *opaque);
 int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts,
BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
+   bool force,
Error **errp);
 
 /* check if a named node can be replaced when doing drive-mirror */
diff --git a/include/block/block_int.h b/include/block/block_int.h
index df6d0273d6..952b2f033a 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -432,6 +432,7 @@ struct BlockDriver {
 int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts,
   BlockDriverAmendStatusCB *status_cb,
   void *cb_opaque,
+  bool force,
   Error **errp);
 
 void (*bdrv_debug_event)(BlockDriverState *bs, BlkdebugEvent event);
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index c9c54de1df..9920f1f9d4 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -10,9 +10,9 @@ HXCOMM When amending the rST sections, please remember to 
copy the usage
 HXCOMM over to the per-command sections in qemu-img.texi.
 
 DEF("amend", img_amend,
-"amend [--object objectdef] [--image-opts] [-p] [-q] [-f fmt] [-t cache] 
-o options filename")
+"amend [--object objectdef] [--image-opts] [-p] [-q] [-f fmt] [-t cache] 
[--force] -o options filename")
 SRST
-.. option:: amend [--object OBJECTDEF] [--image-opts] [-p] [-q] [-f FMT] [-t 
CACHE] -o OPTIONS FILENAME
+.. option:: amend [--object OBJECTDEF] [--image-opts] [-p] [-q] [-f FMT] [-t 
CACHE] [--force] -o OPTIONS FILENAME
 ERST
 
 DEF("bench", img_bench,
diff --git a/qemu-img.c b/qemu-img.c
index 6a4327aaba..ef422d5471 100644
---

[PATCH v6 01/14] qcrypto/core: add generic infrastructure for crypto options amendment

2020-05-10 Thread Maxim Levitsky

This will be used first to implement luks keyslot management.

block_crypto_amend_opts_init will be used to convert
qemu-img cmdline to QCryptoBlockAmendOptions

Signed-off-by: Maxim Levitsky 
Reviewed-by: Daniel P. Berrangé 
---
 block/crypto.c | 17 +
 block/crypto.h |  3 +++
 crypto/block.c | 29 +
 crypto/blockpriv.h |  8 
 include/crypto/block.h | 22 ++
 qapi/crypto.json   | 16 
 6 files changed, 95 insertions(+)

diff --git a/block/crypto.c b/block/crypto.c
index 6b21d6bf6c..9d95329d7f 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -202,6 +202,23 @@ block_crypto_create_opts_init(QDict *opts, Error **errp)
 return ret;
 }
 
+QCryptoBlockAmendOptions *
+block_crypto_amend_opts_init(QDict *opts, Error **errp)
+{
+Visitor *v;
+QCryptoBlockAmendOptions *ret;
+
+v = qobject_input_visitor_new_flat_confused(opts, errp);
+if (!v) {
+return NULL;
+}
+
+visit_type_QCryptoBlockAmendOptions(v, NULL, , errp);
+
+visit_free(v);
+return ret;
+}
+
 
 static int block_crypto_open_generic(QCryptoBlockFormat format,
  QemuOptsList *opts_spec,
diff --git a/block/crypto.h b/block/crypto.h
index b935695e79..06e044c9be 100644
--- a/block/crypto.h
+++ b/block/crypto.h
@@ -91,6 +91,9 @@
 QCryptoBlockCreateOptions *
 block_crypto_create_opts_init(QDict *opts, Error **errp);
 
+QCryptoBlockAmendOptions *
+block_crypto_amend_opts_init(QDict *opts, Error **errp);
+
 QCryptoBlockOpenOptions *
 block_crypto_open_opts_init(QDict *opts, Error **errp);
 
diff --git a/crypto/block.c b/crypto/block.c
index 6f42b32f1e..eb057948b5 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -150,6 +150,35 @@ 
qcrypto_block_calculate_payload_offset(QCryptoBlockCreateOptions *create_opts,
 return crypto != NULL;
 }
 
+int qcrypto_block_amend_options(QCryptoBlock *block,
+QCryptoBlockReadFunc readfunc,
+QCryptoBlockWriteFunc writefunc,
+void *opaque,
+QCryptoBlockAmendOptions *options,
+bool force,
+Error **errp)
+{
+if (options->format != block->format) {
+error_setg(errp,
+   "Cannot amend encryption format");
+return -1;
+}
+
+if (!block->driver->amend) {
+error_setg(errp,
+   "Crypto format %s doesn't support format options amendment",
+   QCryptoBlockFormat_str(block->format));
+return -1;
+}
+
+return block->driver->amend(block,
+readfunc,
+writefunc,
+opaque,
+options,
+force,
+errp);
+}
 
 QCryptoBlockInfo *qcrypto_block_get_info(QCryptoBlock *block,
  Error **errp)
diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
index 71c59cb542..3c7ccea504 100644
--- a/crypto/blockpriv.h
+++ b/crypto/blockpriv.h
@@ -62,6 +62,14 @@ struct QCryptoBlockDriver {
   void *opaque,
   Error **errp);
 
+int (*amend)(QCryptoBlock *block,
+ QCryptoBlockReadFunc readfunc,
+ QCryptoBlockWriteFunc writefunc,
+ void *opaque,
+ QCryptoBlockAmendOptions *options,
+ bool force,
+ Error **errp);
+
 int (*get_info)(QCryptoBlock *block,
 QCryptoBlockInfo *info,
 Error **errp);
diff --git a/include/crypto/block.h b/include/crypto/block.h
index c77ccaf9c0..d274819791 100644
--- a/include/crypto/block.h
+++ b/include/crypto/block.h
@@ -144,6 +144,28 @@ QCryptoBlock 
*qcrypto_block_create(QCryptoBlockCreateOptions *options,
void *opaque,
Error **errp);
 
+/**
+ * qcrypto_block_amend_options:
+ * @block: the block encryption object
+ *
+ * @readfunc: callback for reading data from the volume header
+ * @writefunc: callback for writing data to the volume header
+ * @opaque: data to pass to @readfunc and @writefunc
+ * @options: the new/amended encryption options
+ * @force: hint for the driver to allow unsafe operation
+ * @errp: error pointer
+ *
+ * Changes the crypto options of the encryption format
+ *
+ */
+int qcrypto_block_amend_options(QCryptoBlock *block,
+QCryptoBlockReadFunc readfunc,
+QCryptoBlockWriteFunc writefunc,
+void *opaque,
+QCryptoBlockAmendOptions *options,
+bool force,
+Error **errp);
+
 
 /**
  *

[PATCH v6 00/14] LUKS: encryption slot management using amend interface

2020-05-10 Thread Maxim Levitsky

Hi!
Here is the updated series of my patches, incorporating all the feedback I 
received.

This implements the API interface that we agreed upon except that I merged the
LUKSKeyslotActive/LUKSKeyslotInactive union into a struct because otherwise
I need nested unions which are not supported currently by QAPI parser.
This didn't change the API and thus once support for nested unions is there,
it can always be implemented in backward compatible way.

I hope that this series will finally be considered for merging, since I am 
somewhat running
out of time to finish this task.

Patches are strictly divided by topic to 3 groups, and each group depends on 
former groups.

* Patches 1,2 implement qcrypto generic amend interface, including definition
  of structs used in crypto.json and implement this in luks crypto driver
  Nothing is exposed to the user at this stage

* Patches 3-9 use the code from patches 1,2 to implement qemu-img amend based 
encryption slot management
  for luks and for qcow2, and add a bunch of iotests to cover that.

* Patches 10-13 add x-blockdev-amend (I'll drop the -x prefix if you like), and 
wire it
  to luks and qcow2 driver to implement qmp based encryption slot management 
also using
  the code from patches 1,2, and also add a bunch of iotests to cover this.

Tested with -raw,-qcow2,-nbd and -luks iotests and 'make check'

Changes from V4:
  * Addresed feedback on patch 2 from Daniel (thanks!)
  * Gave real numbers to the tests
  * Updated the copyright in amend.c to RedHat
  * Rebased and adjusted the python iotests to latest changes in iotest 
infrastructure

Changes from V5:
  * Updated all QMP docs to state that all added QMP features are since 5.1
  * Rebased to latest master

Best regards,
Maxim Levitsky

clone of "luks-keymgmnt-v2"

Maxim Levitsky (14):
  qcrypto/core: add generic infrastructure for crypto options amendment
  qcrypto/luks: implement encryption key management
  block/amend: add 'force' option
  block/amend: separate amend and create options for qemu-img
  block/amend: refactor qcow2 amend options
  block/crypto: rename two functions
  block/crypto: implement the encryption key management
  block/qcow2: extend qemu-img amend interface with crypto options
  iotests: filter few more luks specific create options
  iotests: qemu-img tests for luks key management
  block/core: add generic infrastructure for x-blockdev-amend qmp
command
  block/crypto: implement blockdev-amend
  block/qcow2: implement blockdev-amend
  iotests: add tests for blockdev-amend

 block.c  |   4 +-
 block/Makefile.objs  |   2 +-
 block/amend.c| 108 +
 block/crypto.c   | 203 ++--
 block/crypto.h   |  37 +++
 block/qcow2.c| 306 ++--
 crypto/block-luks.c  | 395 ++-
 crypto/block.c   |  29 +++
 crypto/blockpriv.h   |   8 +
 docs/tools/qemu-img.rst  |   5 +-
 include/block/block.h|   1 +
 include/block/block_int.h|  24 +-
 include/crypto/block.h   |  22 ++
 qapi/block-core.json |  68 ++
 qapi/crypto.json |  75 +-
 qapi/job.json|   4 +-
 qemu-img-cmds.hx |   4 +-
 qemu-img.c   |  44 +++-
 tests/qemu-iotests/049.out   | 102 
 tests/qemu-iotests/061.out   |  12 +-
 tests/qemu-iotests/079.out   |  18 +-
 tests/qemu-iotests/082.out   | 176 --
 tests/qemu-iotests/085.out   |  38 +--
 tests/qemu-iotests/087.out   |   6 +-
 tests/qemu-iotests/115.out   |   2 +-
 tests/qemu-iotests/121.out   |   4 +-
 tests/qemu-iotests/125.out   | 192 +++
 tests/qemu-iotests/134.out   |   2 +-
 tests/qemu-iotests/144.out   |   4 +-
 tests/qemu-iotests/158.out   |   4 +-
 tests/qemu-iotests/182.out   |   2 +-
 tests/qemu-iotests/185.out   |   8 +-
 tests/qemu-iotests/188.out   |   2 +-
 tests/qemu-iotests/189.out   |   4 +-
 tests/qemu-iotests/198.out   |   4 +-
 tests/qemu-iotests/243.out   |  16 +-
 tests/qemu-iotests/250.out   |   2 +-
 tests/qemu-iotests/255.out   |   8 +-
 tests/qemu-iotests/259.out   |   2 +-
 tests/qemu-iotests/263.out   |   4 +-
 tests/qemu-iotests/274.out   |  46 ++--
 tests/qemu-iotests/280.out   |   2 +-
 tests/qemu-iotests/284.out   |   6 +-
 tests/qemu-iotests/293   | 207 
 tests/qemu-iotests/293.out   |  99 
 tests/qemu-iotests/294   |  90 +++
 tests/qemu-iotests/294.out   |  30 +++
 tests/qemu-iotests/295   | 279 ++
 tests/qemu-iotests/295.out   |  40 
 tests/qemu-iotests/296   | 234 ++
 tests/qemu-iotests/296.out   |  33 +++
 tests/qemu-iotests/common.filter |   6 +-
 tests/qemu-iotests/group

[PATCH v6 02/14] qcrypto/luks: implement encryption key management

2020-05-10 Thread Maxim Levitsky

Next few patches will expose that functionality to the user.

Signed-off-by: Maxim Levitsky 
Reviewed-by: Daniel P. Berrangé 
---
 crypto/block-luks.c | 395 +++-
 qapi/crypto.json|  61 ++-
 2 files changed, 452 insertions(+), 4 deletions(-)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 4861db810c..967d382882 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -32,6 +32,7 @@
 #include "qemu/uuid.h"
 
 #include "qemu/coroutine.h"
+#include "qemu/bitmap.h"
 
 /*
  * Reference for the LUKS format implemented here is
@@ -70,6 +71,9 @@ typedef struct QCryptoBlockLUKSKeySlot 
QCryptoBlockLUKSKeySlot;
 
 #define QCRYPTO_BLOCK_LUKS_SECTOR_SIZE 512LL
 
+#define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS 2000
+#define QCRYPTO_BLOCK_LUKS_ERASE_ITERATIONS 40
+
 static const char qcrypto_block_luks_magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN] = {
 'L', 'U', 'K', 'S', 0xBA, 0xBE
 };
@@ -219,6 +223,9 @@ struct QCryptoBlockLUKS {
 
 /* Hash algorithm used in pbkdf2 function */
 QCryptoHashAlgorithm hash_alg;
+
+/* Name of the secret that was used to open the image */
+char *secret;
 };
 
 
@@ -1069,6 +1076,119 @@ qcrypto_block_luks_find_key(QCryptoBlock *block,
 return -1;
 }
 
+/*
+ * Returns true if a slot i is marked as active
+ * (contains encrypted copy of the master key)
+ */
+static bool
+qcrypto_block_luks_slot_active(const QCryptoBlockLUKS *luks,
+   unsigned int slot_idx)
+{
+uint32_t val = luks->header.key_slots[slot_idx].active;
+return val ==  QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED;
+}
+
+/*
+ * Returns the number of slots that are marked as active
+ * (slots that contain encrypted copy of the master key)
+ */
+static unsigned int
+qcrypto_block_luks_count_active_slots(const QCryptoBlockLUKS *luks)
+{
+size_t i = 0;
+unsigned int ret = 0;
+
+for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
+if (qcrypto_block_luks_slot_active(luks, i)) {
+ret++;
+}
+}
+return ret;
+}
+
+/*
+ * Finds first key slot which is not active
+ * Returns the key slot index, or -1 if it doesn't exist
+ */
+static int
+qcrypto_block_luks_find_free_keyslot(const QCryptoBlockLUKS *luks)
+{
+size_t i;
+
+for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
+if (!qcrypto_block_luks_slot_active(luks, i)) {
+return i;
+}
+}
+return -1;
+}
+
+/*
+ * Erases an keyslot given its index
+ * Returns:
+ *0 if the keyslot was erased successfully
+ *   -1 if a error occurred while erasing the keyslot
+ *
+ */
+static int
+qcrypto_block_luks_erase_key(QCryptoBlock *block,
+ unsigned int slot_idx,
+ QCryptoBlockWriteFunc writefunc,
+ void *opaque,
+ Error **errp)
+{
+QCryptoBlockLUKS *luks = block->opaque;
+QCryptoBlockLUKSKeySlot *slot = >header.key_slots[slot_idx];
+g_autofree uint8_t *garbagesplitkey = NULL;
+size_t splitkeylen = luks->header.master_key_len * slot->stripes;
+size_t i;
+Error *local_err = NULL;
+int ret;
+
+assert(slot_idx < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
+assert(splitkeylen > 0);
+garbagesplitkey = g_new0(uint8_t, splitkeylen);
+
+/* Reset the key slot header */
+memset(slot->salt, 0, QCRYPTO_BLOCK_LUKS_SALT_LEN);
+slot->iterations = 0;
+slot->active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
+
+ret = qcrypto_block_luks_store_header(block,  writefunc,
+  opaque, _err);
+
+if (ret < 0) {
+error_propagate(errp, local_err);
+}
+/*
+ * Now try to erase the key material, even if the header
+ * update failed
+ */
+for (i = 0; i < QCRYPTO_BLOCK_LUKS_ERASE_ITERATIONS; i++) {
+if (qcrypto_random_bytes(garbagesplitkey,
+ splitkeylen, _err) < 0) {
+/*
+ * If we failed to get the random data, still write
+ * at least zeros to the key slot at least once
+ */
+error_propagate(errp, local_err);
+
+if (i > 0) {
+return -1;
+}
+}
+if (writefunc(block,
+  slot->key_offset_sector * QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
+  garbagesplitkey,
+  splitkeylen,
+  opaque,
+  _err) != splitkeylen) {
+error_propagate(errp, local_err);
+return -1;
+}
+}
+return ret;
+}
 
 static int
 qcrypto_block_luks_open(QCryptoBlock *block,
@@ -1099,6 +1219,7 @@ qcrypto_block_luks_open(QCryptoBlock *block,
 
 luks = g_new0(QCryptoBlockLUKS, 1);
 block->opaque = luks;
+luks->secret = g_strdup(options->u.luks.key_secret);
 
 if (qcrypto_block_luks_load_header(block, readfunc, opaque, errp) < 0) {
 goto

Re: [PATCH v2 5/5] vhost: add device started check in migration set log

Re: [PATCH v2 4/5] vhost: check vring address before calling unmap

Re: [PATCH v2 2/5] vhost: introduce wrappers to set guest notifiers for virtio device

Re: [PATCH 05/10] exec: Move qemu_minrampagesize/qemu_maxrampagesize to 'qemu-common.h'

Re: [PATCH 10/10] exec: Move cpu_physical_memory_* functions to 'exec/memory-internal.h'

Re: [PATCH 07/10] exec: Move all RAMBlock functions to 'exec/ramblock.h'

Re: [PATCH v2 5/5] vhost: add device started check in migration set log

Re: [PATCH v6 00/14] LUKS: encryption slot management using amend interface

Re: [PATCH v6 00/14] LUKS: encryption slot management using amend interface

[PATCH v6 14/14] iotests: add tests for blockdev-amend

[PATCH v6 12/14] block/crypto: implement blockdev-amend

[PATCH v6 07/14] block/crypto: implement the encryption key management

[PATCH v6 06/14] block/crypto: rename two functions

[PATCH v6 05/14] block/amend: refactor qcow2 amend options

[PATCH v6 09/14] iotests: filter few more luks specific create options

[PATCH v6 08/14] block/qcow2: extend qemu-img amend interface with crypto options

[PATCH v6 04/14] block/amend: separate amend and create options for qemu-img

[PATCH v6 10/14] iotests: qemu-img tests for luks key management

[PATCH v6 11/14] block/core: add generic infrastructure for x-blockdev-amend qmp command

[PATCH v6 13/14] block/qcow2: implement blockdev-amend

[PATCH v6 03/14] block/amend: add 'force' option

[PATCH v6 01/14] qcrypto/core: add generic infrastructure for crypto options amendment

[PATCH v6 00/14] LUKS: encryption slot management using amend interface

[PATCH v6 02/14] qcrypto/luks: implement encryption key management

24 matches

Site Navigation

Mail list logo

Footer information