d13e3b46 | 10-Mar-2024 |
Zhao Liu <zhao1.liu@intel.com> |
block/qcow2: Fix missing ERRP_GUARD() for error_prepend()
As the comment in qapi/error, passing @errp to error_prepend() requires ERRP_GUARD():
* = Why, when and how to use ERRP_GUARD() = * * Witho
block/qcow2: Fix missing ERRP_GUARD() for error_prepend()
As the comment in qapi/error, passing @errp to error_prepend() requires ERRP_GUARD():
* = Why, when and how to use ERRP_GUARD() = * * Without ERRP_GUARD(), use of the @errp parameter is restricted: ... * - It should not be passed to error_prepend(), error_vprepend() or * error_append_hint(), because that doesn't work with &error_fatal. * ERRP_GUARD() lifts these restrictions. * * To use ERRP_GUARD(), add it right at the beginning of the function. * @errp can then be used without worrying about the argument being * NULL or &error_fatal.
ERRP_GUARD() could avoid the case when @errp is &error_fatal, the user can't see this additional information, because exit() happens in error_setg earlier than information is added [1].
In block/qcow2.c, there are 2 functions passing @errp to error_prepend() without ERRP_GUARD(): - qcow2_co_create() - qcow2_co_truncate()
There are too many possible callers to check the impact of the defect; it may or may not be harmless. Thus it is necessary to protect @errp with ERRP_GUARD().
Therefore, to avoid the issue like [1] said, add missing ERRP_GUARD() at their beginning.
[1]: Issue description in the commit message of commit ae7c80a7bd73 ("error: New macro ERRP_GUARD()").
Cc: Kevin Wolf <kwolf@redhat.com> Cc: Hanna Reitz <hreitz@redhat.com> Cc: qemu-block@nongnu.org Signed-off-by: Zhao Liu <zhao1.liu@intel.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-ID: <20240311033822.3142585-9-zhao1.liu@linux.intel.com> Signed-off-by: Thomas Huth <thuth@redhat.com>
show more ...
|
e1081e3b | 10-Mar-2024 |
Zhao Liu <zhao1.liu@intel.com> |
block/qcow2-bitmap: Fix missing ERRP_GUARD() for error_prepend()
As the comment in qapi/error, passing @errp to error_prepend() requires ERRP_GUARD():
* = Why, when and how to use ERRP_GUARD() = *
block/qcow2-bitmap: Fix missing ERRP_GUARD() for error_prepend()
As the comment in qapi/error, passing @errp to error_prepend() requires ERRP_GUARD():
* = Why, when and how to use ERRP_GUARD() = * * Without ERRP_GUARD(), use of the @errp parameter is restricted: ... * - It should not be passed to error_prepend(), error_vprepend() or * error_append_hint(), because that doesn't work with &error_fatal. * ERRP_GUARD() lifts these restrictions. * * To use ERRP_GUARD(), add it right at the beginning of the function. * @errp can then be used without worrying about the argument being * NULL or &error_fatal.
ERRP_GUARD() could avoid the case when @errp is &error_fatal, the user can't see this additional information, because exit() happens in error_setg earlier than information is added [1].
The qcow2_co_can_store_new_dirty_bitmap() passes @errp to error_prepend(). As a BlockDriver.bdrv_co_can_store_new_dirty_bitmap method, it's called by bdrv_co_can_store_new_dirty_bitmap().
Its caller is not being called anywhere, but as the API in include/block/block-io.h, we can't ensure what kind of @errp future users will pass in.
To avoid potential issues as [1] said, add missing ERRP_GUARD() at the beginning of qcow2_co_can_store_new_dirty_bitmap().
[1]: Issue description in the commit message of commit ae7c80a7bd73 ("error: New macro ERRP_GUARD()").
Cc: Kevin Wolf <kwolf@redhat.com> Cc: Hanna Reitz <hreitz@redhat.com> Cc: Eric Blake <eblake@redhat.com> Cc: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Cc: John Snow <jsnow@redhat.com> Cc: qemu-block@nongnu.org Signed-off-by: Zhao Liu <zhao1.liu@intel.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-ID: <20240311033822.3142585-8-zhao1.liu@linux.intel.com> Signed-off-by: Thomas Huth <thuth@redhat.com>
show more ...
|
ed46217d | 10-Mar-2024 |
Zhao Liu <zhao1.liu@intel.com> |
block/nvme: Fix missing ERRP_GUARD() for error_prepend()
As the comment in qapi/error, passing @errp to error_prepend() requires ERRP_GUARD():
* = Why, when and how to use ERRP_GUARD() = * * Withou
block/nvme: Fix missing ERRP_GUARD() for error_prepend()
As the comment in qapi/error, passing @errp to error_prepend() requires ERRP_GUARD():
* = Why, when and how to use ERRP_GUARD() = * * Without ERRP_GUARD(), use of the @errp parameter is restricted: ... * - It should not be passed to error_prepend(), error_vprepend() or * error_append_hint(), because that doesn't work with &error_fatal. * ERRP_GUARD() lifts these restrictions. * * To use ERRP_GUARD(), add it right at the beginning of the function. * @errp can then be used without worrying about the argument being * NULL or &error_fatal.
ERRP_GUARD() could avoid the case when @errp is &error_fatal, the user can't see this additional information, because exit() happens in error_setg earlier than information is added [1].
In nvme.c, there are 3 functions passing @errp to error_prepend() without ERRP_GUARD(): - nvme_init_queue() - nvme_create_queue_pair() - nvme_identify()
All these 3 functions take their @errp parameters from the nvme_file_open(), which is a BlockDriver.bdrv_nvme() method and its @errp points to its caller's local_err.
Though these 3 cases haven't trigger the issue like [1] said, to follow the requirement of @errp, add missing ERRP_GUARD() at their beginning.
[1]: Issue description in the commit message of commit ae7c80a7bd73 ("error: New macro ERRP_GUARD()").
Cc: Stefan Hajnoczi <stefanha@redhat.com> Cc: Fam Zheng <fam@euphon.net> Cc: Philippe Mathieu-Daudé <philmd@linaro.org> Cc: Kevin Wolf <kwolf@redhat.com> Cc: Hanna Reitz <hreitz@redhat.com> Cc: qemu-block@nongnu.org Signed-off-by: Zhao Liu <zhao1.liu@intel.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Message-ID: <20240311033822.3142585-7-zhao1.liu@linux.intel.com> Signed-off-by: Thomas Huth <thuth@redhat.com>
show more ...
|
c835ca00 | 10-Mar-2024 |
Zhao Liu <zhao1.liu@intel.com> |
block/nbd: Fix missing ERRP_GUARD() for error_prepend()
As the comment in qapi/error, passing @errp to error_prepend() requires ERRP_GUARD():
* = Why, when and how to use ERRP_GUARD() = * * Without
block/nbd: Fix missing ERRP_GUARD() for error_prepend()
As the comment in qapi/error, passing @errp to error_prepend() requires ERRP_GUARD():
* = Why, when and how to use ERRP_GUARD() = * * Without ERRP_GUARD(), use of the @errp parameter is restricted: ... * - It should not be passed to error_prepend(), error_vprepend() or * error_append_hint(), because that doesn't work with &error_fatal. * ERRP_GUARD() lifts these restrictions. * * To use ERRP_GUARD(), add it right at the beginning of the function. * @errp can then be used without worrying about the argument being * NULL or &error_fatal.
ERRP_GUARD() could avoid the case when @errp is &error_fatal, the user can't see this additional information, because exit() happens in error_setg earlier than information is added [1].
The nbd_co_do_receive_one_chunk() passes @errp to error_prepend() without ERRP_GUARD(), and though its @errp parameter points to its caller's local_err, to follow the requirement of @errp, add missing ERRP_GUARD() at the beginning of this function.
[1]: Issue description in the commit message of commit ae7c80a7bd73 ("error: New macro ERRP_GUARD()").
Cc: Eric Blake <eblake@redhat.com> Cc: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Cc: Kevin Wolf <kwolf@redhat.com> Cc: Hanna Reitz <hreitz@redhat.com> Cc: qemu-block@nongnu.org Signed-off-by: Zhao Liu <zhao1.liu@intel.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-ID: <20240311033822.3142585-6-zhao1.liu@linux.intel.com> Signed-off-by: Thomas Huth <thuth@redhat.com>
show more ...
|
35286dae | 29-Jan-2024 |
Hyman Huang <yong.huang@smartx.com> |
block: Support detached LUKS header creation using qemu-img
Even though a LUKS header might be created with cryptsetup, qemu-img should be enhanced to accommodate it as well.
Add the 'detached-head
block: Support detached LUKS header creation using qemu-img
Even though a LUKS header might be created with cryptsetup, qemu-img should be enhanced to accommodate it as well.
Add the 'detached-header' option to specify the creation of a detached LUKS header. This is how it is used: $ qemu-img create --object secret,id=sec0,data=abc123 -f luks > -o cipher-alg=aes-256,cipher-mode=xts -o key-secret=sec0 > -o detached-header=true header.luks
Using qemu-img or cryptsetup tools to query information of an LUKS header image as follows:
Assume a detached LUKS header image has been created by: $ dd if=/dev/zero of=test-header.img bs=1M count=32 $ dd if=/dev/zero of=test-payload.img bs=1M count=1000 $ cryptsetup luksFormat --header test-header.img test-payload.img > --force-password --type luks1
Header image information could be queried using cryptsetup: $ cryptsetup luksDump test-header.img
or qemu-img: $ qemu-img info 'json:{"driver":"luks","file":{"filename": > "test-payload.img"},"header":{"filename":"test-header.img"}}'
When using qemu-img, keep in mind that the entire disk information specified by the JSON-format string above must be supplied on the commandline; if not, an overlay check will reveal a problem with the LUKS volume check logic.
Signed-off-by: Hyman Huang <yong.huang@smartx.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> [changed to pass 'cflags' to block_crypto_co_create_generic] Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
show more ...
|
d0112eb4 | 29-Jan-2024 |
Hyman Huang <yong.huang@smartx.com> |
block: Support detached LUKS header creation using blockdev-create
Firstly, enable the ability to choose the block device containing a detachable LUKS header by adding the 'header' parameter to Bloc
block: Support detached LUKS header creation using blockdev-create
Firstly, enable the ability to choose the block device containing a detachable LUKS header by adding the 'header' parameter to BlockdevCreateOptionsLUKS.
Secondly, when formatting the LUKS volume with a detachable header, truncate the payload volume to length without a header size.
Using the qmp blockdev command, create the LUKS volume with a detachable header as follows:
1. add the secret to lock/unlock the cipher stored in the detached LUKS header $ virsh qemu-monitor-command vm '{"execute":"object-add", > "arguments":{"qom-type": "secret", "id": "sec0", "data": "foo"}}'
2. create a header img with 0 size $ virsh qemu-monitor-command vm '{"execute":"blockdev-create", > "arguments":{"job-id":"job0", "options":{"driver":"file", > "filename":"/path/to/detached_luks_header.img", "size":0 }}}'
3. add protocol blockdev node for header $ virsh qemu-monitor-command vm '{"execute":"blockdev-add", > "arguments": {"driver":"file", "filename": > "/path/to/detached_luks_header.img", "node-name": > "detached-luks-header-storage"}}'
4. create a payload img with 0 size $ virsh qemu-monitor-command vm '{"execute":"blockdev-create", > "arguments":{"job-id":"job1", "options":{"driver":"file", > "filename":"/path/to/detached_luks_payload_raw.img", "size":0}}}'
5. add protocol blockdev node for payload $ virsh qemu-monitor-command vm '{"execute":"blockdev-add", > "arguments": {"driver":"file", "filename": > "/path/to/detached_luks_payload_raw.img", "node-name": > "luks-payload-raw-storage"}}'
6. do the formatting with 128M size $ virsh qemu-monitor-command c81_node1 '{"execute":"blockdev-create", > "arguments":{"job-id":"job2", "options":{"driver":"luks", "header": > "detached-luks-header-storage", "file":"luks-payload-raw-storage", > "size":134217728, "preallocation":"full", "key-secret":"sec0" }}}'
Signed-off-by: Hyman Huang <yong.huang@smartx.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
show more ...
|
d74523a3 | 29-Jan-2024 |
Hyman Huang <yong.huang@smartx.com> |
crypto: Modify the qcrypto_block_create to support creation flags
Expand the signature of qcrypto_block_create to enable the formation of LUKS volumes with detachable headers. To accomplish that, in
crypto: Modify the qcrypto_block_create to support creation flags
Expand the signature of qcrypto_block_create to enable the formation of LUKS volumes with detachable headers. To accomplish that, introduce QCryptoBlockCreateFlags to instruct the creation process to set the payload_offset_sector to 0.
Signed-off-by: Hyman Huang <yong.huang@smartx.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
show more ...
|
433957bb | 29-Jan-2024 |
Hyman Huang <yong.huang@smartx.com> |
qapi: Make parameter 'file' optional for BlockdevCreateOptionsLUKS
To support detached LUKS header creation, make the existing 'file' field in BlockdevCreateOptionsLUKS optional.
Signed-off-by: Hym
qapi: Make parameter 'file' optional for BlockdevCreateOptionsLUKS
To support detached LUKS header creation, make the existing 'file' field in BlockdevCreateOptionsLUKS optional.
Signed-off-by: Hyman Huang <yong.huang@smartx.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
show more ...
|
10b2393e | 31-Jan-2024 |
Kevin Wolf <kwolf@redhat.com> |
blkio: Respect memory-alignment for bounce buffer allocations
blkio_alloc_mem_region() requires that the requested buffer size is a multiple of the memory-alignment property. If it isn't, the alloca
blkio: Respect memory-alignment for bounce buffer allocations
blkio_alloc_mem_region() requires that the requested buffer size is a multiple of the memory-alignment property. If it isn't, the allocation fails with a return value of -EINVAL.
Fix the call in blkio_resize_bounce_pool() to make sure the requested size is properly aligned.
I observed this problem with vhost-vdpa, which requires page aligned memory. As the virtio-blk device behind it still had 512 byte blocks, we got bs->bl.request_alignment = 512, but actually any request that needed a bounce buffer and was not aligned to 4k would fail without this fix.
Suggested-by: Stefano Garzarella <sgarzare@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20240131173140.42398-1-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
show more ...
|
615eaeab | 30-Jan-2024 |
Richard W.M. Jones <rjones@redhat.com> |
block/blkio: Make s->mem_region_alignment be 64 bits
With GCC 14 the code failed to compile on i686 (and was wrong for any version of GCC):
../block/blkio.c: In function ‘blkio_file_open’: ../block
block/blkio: Make s->mem_region_alignment be 64 bits
With GCC 14 the code failed to compile on i686 (and was wrong for any version of GCC):
../block/blkio.c: In function ‘blkio_file_open’: ../block/blkio.c:857:28: error: passing argument 3 of ‘blkio_get_uint64’ from incompatible pointer type [-Wincompatible-pointer-types] 857 | &s->mem_region_alignment); | ^~~~~~~~~~~~~~~~~~~~~~~~ | | | size_t * {aka unsigned int *} In file included from ../block/blkio.c:12: /usr/include/blkio.h:49:67: note: expected ‘uint64_t *’ {aka ‘long long unsigned int *’} but argument is of type ‘size_t *’ {aka ‘unsigned int *’} 49 | int blkio_get_uint64(struct blkio *b, const char *name, uint64_t *value); | ~~~~~~~~~~^~~~~
Signed-off-by: Richard W.M. Jones <rjones@redhat.com> Message-id: 20240130122006.2977938-1-rjones@redhat.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
show more ...
|
a9c8ea95 | 09-Jan-2024 |
Ari Sundholm <ari@tuxera.com> |
block/blklogwrites: Fix a bug when logging "write zeroes" operations.
There is a bug in the blklogwrites driver pertaining to logging "write zeroes" operations, causing log corruption. This can be e
block/blklogwrites: Fix a bug when logging "write zeroes" operations.
There is a bug in the blklogwrites driver pertaining to logging "write zeroes" operations, causing log corruption. This can be easily observed by setting detect-zeroes to something other than "off" for the driver.
The issue is caused by a concurrency bug pertaining to the fact that "write zeroes" operations have to be logged in two parts: first the log entry metadata, then the zeroed-out region. While the log entry metadata is being written by bdrv_co_pwritev(), another operation may begin in the meanwhile and modify the state of the blklogwrites driver. This is as intended by the coroutine-driven I/O model in QEMU, of course.
Unfortunately, this specific scenario is mishandled. A short example: 1. Initially, in the current operation (#1), the current log sector number in the driver state is only incremented by the number of sectors taken by the log entry metadata, after which the log entry metadata is written. The current operation yields. 2. Another operation (#2) may start while the log entry metadata is being written. It uses the current log position as the start offset for its log entry. This is in the sector right after the operation #1 log entry metadata, which is bad! 3. After bdrv_co_pwritev() returns (#1), the current log sector number is reread from the driver state in order to find out the start offset for bdrv_co_pwrite_zeroes(). This is an obvious blunder, as the offset will be the sector right after the (misplaced) operation #2 log entry, which means that the zeroed-out region begins at the wrong offset. 4. As a result of the above, the log is corrupt.
Fix this by only reading the driver metadata once, computing the offsets and sizes in one go (including the optional zeroed-out region) and setting the log sector number to the appropriate value for the next operation in line.
Signed-off-by: Ari Sundholm <ari@tuxera.com> Cc: qemu-stable@nongnu.org Message-ID: <20240109184646.1128475-1-megari@gmx.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
show more ...
|
8a9be799 | 16-Jan-2024 |
Fiona Ebner <f.ebner@proxmox.com> |
block/io: clear BDRV_BLOCK_RECURSE flag after recursing in bdrv_co_block_status
Using fleecing backup like in [0] on a qcow2 image (with metadata preallocation) can lead to the following assertion f
block/io: clear BDRV_BLOCK_RECURSE flag after recursing in bdrv_co_block_status
Using fleecing backup like in [0] on a qcow2 image (with metadata preallocation) can lead to the following assertion failure:
> bdrv_co_do_block_status: Assertion `!(ret & BDRV_BLOCK_ZERO)' failed.
In the reproducer [0], it happens because the BDRV_BLOCK_RECURSE flag will be set by the qcow2 driver, so the caller will recursively check the file child. Then the BDRV_BLOCK_ZERO set too. Later up the call chain, in bdrv_co_do_block_status() for the snapshot-access driver, the assertion failure will happen, because both flags are set.
To fix it, clear the recurse flag after the recursive check was done.
In detail:
> #0 qcow2_co_block_status
Returns 0x45 = BDRV_BLOCK_RECURSE | BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID.
> #1 bdrv_co_do_block_status
Because of the data flag, bdrv_co_do_block_status() will now also set BDRV_BLOCK_ALLOCATED. Because of the recurse flag, bdrv_co_do_block_status() for the bdrv_file child will be called, which returns 0x16 = BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_ZERO. Now the return value inherits the zero flag.
Returns 0x57 = BDRV_BLOCK_RECURSE | BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_ZERO.
> #2 bdrv_co_common_block_status_above > #3 bdrv_co_block_status_above > #4 bdrv_co_block_status > #5 cbw_co_snapshot_block_status > #6 bdrv_co_snapshot_block_status > #7 snapshot_access_co_block_status > #8 bdrv_co_do_block_status
Return value is propagated all the way up to here, where the assertion failure happens, because BDRV_BLOCK_RECURSE and BDRV_BLOCK_ZERO are both set.
> #9 bdrv_co_common_block_status_above > #10 bdrv_co_block_status_above > #11 block_copy_block_status > #12 block_copy_dirty_clusters > #13 block_copy_common > #14 block_copy_async_co_entry > #15 coroutine_trampoline
[0]:
> #!/bin/bash > rm /tmp/disk.qcow2 > ./qemu-img create /tmp/disk.qcow2 -o preallocation=metadata -f qcow2 1G > ./qemu-img create /tmp/fleecing.qcow2 -f qcow2 1G > ./qemu-img create /tmp/backup.qcow2 -f qcow2 1G > ./qemu-system-x86_64 --qmp stdio \ > --blockdev qcow2,node-name=node0,file.driver=file,file.filename=/tmp/disk.qcow2 \ > --blockdev qcow2,node-name=node1,file.driver=file,file.filename=/tmp/fleecing.qcow2 \ > --blockdev qcow2,node-name=node2,file.driver=file,file.filename=/tmp/backup.qcow2 \ > <<EOF > {"execute": "qmp_capabilities"} > {"execute": "blockdev-add", "arguments": { "driver": "copy-before-write", "file": "node0", "target": "node1", "node-name": "node3" } } > {"execute": "blockdev-add", "arguments": { "driver": "snapshot-access", "file": "node3", "node-name": "snap0" } } > {"execute": "blockdev-backup", "arguments": { "device": "snap0", "target": "node1", "sync": "full", "job-id": "backup0" } } > EOF
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-id: 20240116154839.401030-1-f.ebner@proxmox.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
show more ...
|
592d0bc0 | 13-Dec-2023 |
Paolo Bonzini <pbonzini@redhat.com> |
remove unnecessary casts from uintptr_t
uintptr_t, or unsigned long which is equivalent on Linux I32LP64 systems, is an unsigned type and there is no need to further cast to __u64 which is another u
remove unnecessary casts from uintptr_t
uintptr_t, or unsigned long which is equivalent on Linux I32LP64 systems, is an unsigned type and there is no need to further cast to __u64 which is another unsigned integer type; widening casts from unsigned integers zero-extend the value.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
show more ...
|