Re: [PATCH v2 13/13] blockdev: Drop unused drive_get_next()
On 17.11.21 17:34, Markus Armbruster wrote: drive_get_next() is basically a bad idea. It returns the "next" block backend of a certain interface type. "Next" means bus=0,unit=N, where subsequent calls count N up from zero, per interface type. This lets you define unit numbers implicitly by execution order. If the order changes, or new calls appear "in the middle", unit numbers change. ABI break. Hard to spot in review. The previous commits eliminated all uses. Drop the function. Cc: Kevin Wolf Cc: Hanna Reitz Signed-off-by: Markus Armbruster --- include/sysemu/blockdev.h | 1 - blockdev.c| 10 -- 2 files changed, 11 deletions(-) Reviewed-by: Hanna Reitz
Re: [PATCH] block/vvfat.c fix leak when failure occurs
On 16.11.21 13:57, Daniella Lee wrote:
Function vvfat_open called function enable_write_target and init_directories,
and these functions malloc new memory for BDRVVVFATState::qcow_filename,
BDRVVVFATState::used_clusters, and BDRVVVFATState::cluster_buff.
When the specified folder does not exist ,it may contains memory leak.
After init_directories function is executed, the vvfat_open return -EIO,
and bdrv_open_driver goto label open_failed,
the program use g_free(bs->opaque) to release BDRVVVFATState struct
without members mentioned.
command line:
qemu-system-x86_64 -hdb -usb -device usb-storage,drive=fat16
-drive file=fat:rw:fat-type=16:"",
id=fat16,format=raw,if=none
enable_write_target called:
(gdb) bt
#0 enable_write_target (bs=0x56f9f000, errp=0x7fffd780)
at ../block/vvfat.c:3114
#1 vvfat_open (bs=0x56f9f000, options=0x56fa45d0,
flags=155650, errp=0x7fffd780) at ../block/vvfat.c:1236
#2 bdrv_open_driver (bs=0x56f9f000, drv=0x56c47920 ,
node_name=0x0, options=0x56fa45d0, open_flags=155650,
errp=0x7fffd890) at ../block.c:1558
#3 bdrv_open_common (bs=0x56f9f000, file=0x0, options=0x56fa45d0,
errp=0x7fffd890) at ../block.c:1852
#4 bdrv_open_inherit (filename=0x56f73310 "fat:rw:",
reference=0x0, options=0x56fa45d0, flags=40962, parent=0x56f98cd0,
child_class=0x56b1d6a0 , child_role=19,
errp=0x7fffda90) at ../block.c:3779
#5 bdrv_open_child_bs (filename=0x56f73310 "fat:rw:",
options=0x56f9cfc0, bdref_key=0x56239bb8 "file",
parent=0x56f98cd0, child_class=0x56b1d6a0 ,
child_role=19, allow_none=true, errp=0x7fffda90) at ../block.c:3419
#6 bdrv_open_inherit (filename=0x56f73310 "fat:rw:",
reference=0x0, options=0x56f9cfc0, flags=8194, parent=0x0,
child_class=0x0, child_role=0, errp=0x56c98c40 )
at ../block.c:3726
#7 bdrv_open (filename=0x56f73310 "fat:rw:", reference=0x0,
options=0x56f757b0, flags=0, errp=0x56c98c40 )
at ../block.c:3872
#8 blk_new_open (filename=0x56f73310 "fat:rw:", reference=0x0,
options=0x56f757b0, flags=0, errp=0x56c98c40 )
at ../block/block-backend.c:436
#9 blockdev_init (file=0x56f73310 "fat:rw:",
bs_opts=0x56f757b0, errp=0x56c98c40 )
at ../blockdev.c:608
#10 drive_new (all_opts=0x56d2b700, block_default_type=IF_IDE,
errp=0x56c98c40 ) at ../blockdev.c:992
..
Signed-off-by: Daniella Lee
---
block/vvfat.c | 15 +++
1 file changed, 15 insertions(+)
Hi,
Thanks for your patch! Yes, that makes sense.
I believe there are some issues that should be addressed, though:
diff --git a/block/vvfat.c b/block/vvfat.c
index 05e78e3c27..454a74c5d5 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1280,7 +1280,22 @@ static int vvfat_open(BlockDriverState *bs, QDict
*options, int flags,
qemu_co_mutex_init(&s->lock);
ret = 0;
+
+qemu_opts_del(opts);
+return ret;
Optional: I’d drop the `ret = 0;` line and just `return 0;` here.
fail:
+if(s->qcow_filename) {
Our coding style requires a space between `if` and the opening parenthesis.
+g_free(s->qcow_filename);
`g_free()` checks whether the parameter given to it is `NULL`, and if
so, performs a no-op. So checking whether `s->qcow_filename != NULL`
before calling `g_free()` is not necessary.
We have a script under scripts/checkpatch.pl that takes patch files as
input and checks whether they conform to our coding style. It’s really
helpful, for example in these two cases it does report the issues.
+s->qcow_filename = NULL;
+}
+if(s->cluster_buffer) {
+g_free(s->cluster_buffer);
+s->cluster_buffer = NULL;
+}
+if(s->used_clusters) {
+g_free(s->used_clusters);
`s->used_clusters` is allocated with `calloc()`, so it can’t be freed
with `g_free()`. But you’re right, it should be `g_free()`-able, so the
fix is to have `enable_write_target()` allocate it with `g_malloc0(size)`.
(And this made me notice that we free neither `s->used_clusters` nor
`s->qcow_filename` in vvfat_close()... Oops.)
+s->used_clusters = NULL;
+}
qemu_opts_del(opts);
return ret;
}
Finally, `enable_write_target()` frees `s->qcow_filename` on error.
That seems unnecessary now, though not wrong. (It’s just weird that it
frees that one, but not `s->used_clusters`...)
Hanna
Re: [PATCH v4 08/25] block: introduce assert_bdrv_graph_writable
On 12/11/2021 15:40, Hanna Reitz wrote:
On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:
We want to be sure that the functions that write the child and
parent list of a bs are under BQL and drain.
BQL prevents from concurrent writings from the GS API, while
drains protect from I/O.
TODO: drains are missing in some functions using this assert.
Therefore a proper assertion will fail. Because adding drains
requires additional discussions, they will be added in future
series.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Paolo Bonzini
Reviewed-by: Stefan Hajnoczi
---
block.c | 5 +
block/io.c | 11 +++
include/block/block_int-global-state.h | 10 +-
3 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 41c5883c5c..94bff5c757 100644
--- a/block.c
+++ b/block.c
@@ -2734,12 +2734,14 @@ static void
bdrv_replace_child_noperm(BdrvChild *child,
if (child->klass->detach) {
child->klass->detach(child);
}
+ assert_bdrv_graph_writable(old_bs);
QLIST_REMOVE(child, next_parent);
I think this belongs above the .detach() call (and the QLIST_REMOVE()
belongs into the .detach() implementation, as done in
https://lists.nongnu.org/archive/html/qemu-block/2021-11/msg00240.html,
which has been merged to Kevin’s block branch).
Yes, I rebased on kwolf/block branch. Thank you for pointing that out.
}
child->bs = new_bs;
if (new_bs) {
+ assert_bdrv_graph_writable(new_bs);
QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
In both these places it’s a bit strange that the assertion is done on
the child nodes. The subgraph starting from them isn’t modified after
all, so their subgraph technically doesn’t need to be writable. I think
a single assertion on the parent node would be preferable.
I presume the problem with that is that we don’t have the parent node
here? Do we need a new BdrvChildClass method that performs this
assertion on the parent node?
Uhm I am not sure what you mean here.
Just to recap on how I see this: the assertion
assert_bdrv_graph_writable(bs) is basically used to make sure we are
protecting the write on some fields (childrens and parents lists in this
patch) of a given @bs. It should work like a rwlock: reading is allowed
to be concurrent, but a write should stop all readers to prevent
concurrency issues. This is achieved by draining.
Let's use the first case that you point out, old_bs (it's specular for
new_bs):
>> +assert_bdrv_graph_writable(old_bs);
>> QLIST_REMOVE(child, next_parent);
So old_bs should be the child "son" (child->bs), meaning old_bs->parents
contains the child. Therefore when a child is removed by old_bs, we need
to be sure we are doing it safely.
So we should check that if old_bs exists, old_bs should be drained, to
prevent any other iothread from reading the ->parents list that is being
updated.
The only thing to keep in mind in this case is that just wrapping a
drain around that won't be enough, because then the child won't be
included in the drain_end(old_bs). Therefore the right way to cover this
drain-wise once the assertion also checks for drains is:
drain_begin(old_bs)
assert_bdrv_graph_writable(old_bs)
QLIST_REMOVE(child, next_parent)
/* old_bs will be under drain_end, but not the child */
bdrv_parent_drained_end_single(child);
bdrv_drained_end(old_bs);
I think you agree on this so far.
Now I think your concern is related to the child "parent", namely
child->opaque. The problem is that in the .detach and .attach callbacks
we are firstly adding/removing the child from the list, and then calling
drain on the subtree. We would ideally need to do the opposite:
assert_bdrv_graph_writable(bs);
QLIST_REMOVE(child, next);
bdrv_unapply_subtree_drain(child, bs);
In this case I think this would actually work, because removing/adding
the child from the ->children list beforehand just prevents an
additional recursion call (I think, and the fact that tests are passing
seems to confirm my theory).
Of course you know this stuff better than me, so let me know if
something here is wrong.
/*
@@ -2940,6 +2942,7 @@ static int
bdrv_attach_child_noperm(BlockDriverState *parent_bs,
return ret;
}
+ assert_bdrv_graph_writable(parent_bs);
QLIST_INSERT_HEAD(&parent_bs->children, *child, next);
/*
* child is removed in bdrv_attach_child_common_abort(), so
don't care to
@@ -3140,6 +3143,7 @@ static void
bdrv_unset_inherits_from(BlockDriverState *root, BdrvChild *child,
void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child)
{
assert(qemu_in_main_thread());
+ assert_bdrv_graph_writable(parent);
It looks to me like we have this assertion mainly because
bdrv_replace_child_noperm() doesn’t have a pointer to this parent node.
I
Re: [PATCH v4 08/25] block: introduce assert_bdrv_graph_writable
On 18/11/2021 10:55, Emanuele Giuseppe Esposito wrote:
On 12/11/2021 15:40, Hanna Reitz wrote:
On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:
We want to be sure that the functions that write the child and
parent list of a bs are under BQL and drain.
BQL prevents from concurrent writings from the GS API, while
drains protect from I/O.
TODO: drains are missing in some functions using this assert.
Therefore a proper assertion will fail. Because adding drains
requires additional discussions, they will be added in future
series.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Paolo Bonzini
Reviewed-by: Stefan Hajnoczi
---
block.c | 5 +
block/io.c | 11 +++
include/block/block_int-global-state.h | 10 +-
3 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 41c5883c5c..94bff5c757 100644
--- a/block.c
+++ b/block.c
@@ -2734,12 +2734,14 @@ static void
bdrv_replace_child_noperm(BdrvChild *child,
if (child->klass->detach) {
child->klass->detach(child);
}
+ assert_bdrv_graph_writable(old_bs);
QLIST_REMOVE(child, next_parent);
I think this belongs above the .detach() call (and the QLIST_REMOVE()
belongs into the .detach() implementation, as done in
https://lists.nongnu.org/archive/html/qemu-block/2021-11/msg00240.html, which
has been merged to Kevin’s block branch).
Yes, I rebased on kwolf/block branch. Thank you for pointing that out.
}
child->bs = new_bs;
if (new_bs) {
+ assert_bdrv_graph_writable(new_bs);
QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
In both these places it’s a bit strange that the assertion is done on
the child nodes. The subgraph starting from them isn’t modified after
all, so their subgraph technically doesn’t need to be writable. I
think a single assertion on the parent node would be preferable.
I presume the problem with that is that we don’t have the parent node
here? Do we need a new BdrvChildClass method that performs this
assertion on the parent node?
Uhm I am not sure what you mean here.
Just to recap on how I see this: the assertion
assert_bdrv_graph_writable(bs) is basically used to make sure we are
protecting the write on some fields (childrens and parents lists in this
patch) of a given @bs. It should work like a rwlock: reading is allowed
to be concurrent, but a write should stop all readers to prevent
concurrency issues. This is achieved by draining.
I am thinking to add an additional explanation to
assert_bdrv_graph_writable header comment by saying
"Drains act as a rwlock: while reading is allowed to be concurrent from
all iothreads, when a write needs to be performed we need to stop
(drain) all involved iothreads from reading the graph, to avoid race
conditions."
Somethink like that.
Emanuele
Let's use the first case that you point out, old_bs (it's specular for
new_bs):
>> + assert_bdrv_graph_writable(old_bs);
>> QLIST_REMOVE(child, next_parent);
So old_bs should be the child "son" (child->bs), meaning old_bs->parents
contains the child. Therefore when a child is removed by old_bs, we need
to be sure we are doing it safely.
So we should check that if old_bs exists, old_bs should be drained, to
prevent any other iothread from reading the ->parents list that is being
updated.
The only thing to keep in mind in this case is that just wrapping a
drain around that won't be enough, because then the child won't be
included in the drain_end(old_bs). Therefore the right way to cover this
drain-wise once the assertion also checks for drains is:
drain_begin(old_bs)
assert_bdrv_graph_writable(old_bs)
QLIST_REMOVE(child, next_parent)
/* old_bs will be under drain_end, but not the child */
bdrv_parent_drained_end_single(child);
bdrv_drained_end(old_bs);
I think you agree on this so far.
Now I think your concern is related to the child "parent", namely
child->opaque. The problem is that in the .detach and .attach callbacks
we are firstly adding/removing the child from the list, and then calling
drain on the subtree. We would ideally need to do the opposite:
assert_bdrv_graph_writable(bs);
QLIST_REMOVE(child, next);
bdrv_unapply_subtree_drain(child, bs);
In this case I think this would actually work, because removing/adding
the child from the ->children list beforehand just prevents an
additional recursion call (I think, and the fact that tests are passing
seems to confirm my theory).
Of course you know this stuff better than me, so let me know if
something here is wrong.
/*
@@ -2940,6 +2942,7 @@ static int
bdrv_attach_child_noperm(BlockDriverState *parent_bs,
return ret;
}
+ assert_bdrv_graph_writable(parent_bs);
QLIST_INSERT_HEAD(&parent_bs->children, *child, next);
/*
* child is removed in bdrv_attach_child_c
Re: [PATCH-for-6.2 v2 1/2] hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196
Hi Philippe,
A small nit below, but otherwise looks good.
On Thursday, 2021-11-18 at 00:24:21 +01, Philippe Mathieu-Daudé wrote:
> Guest might select another drive on the bus by setting the
> DRIVE_SEL bit of the DIGITAL OUTPUT REGISTER (DOR).
> The current controller model doesn't expect a BlockBackend
> to be NULL. A simple way to fix CVE-2021-20196 is to create
> an empty BlockBackend when it is missing. All further
> accesses will be safely handled, and the controller state
> machines keep behaving correctly.
>
> Fixes: CVE-2021-20196
> Reported-by: Gaoning Pan (Ant Security Light-Year Lab)
> BugLink: https://bugs.launchpad.net/qemu/+bug/1912780
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/338
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/block/fdc.c | 14 +-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index fa933cd3263..eab17e946d6 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1161,7 +1161,19 @@ static FDrive *get_drv(FDCtrl *fdctrl, int unit)
>
> static FDrive *get_cur_drv(FDCtrl *fdctrl)
> {
> -return get_drv(fdctrl, fdctrl->cur_drv);
> +FDrive *cur_drv = get_drv(fdctrl, fdctrl->cur_drv);
> +
> +if (!cur_drv->blk) {
> +/*
> + * Kludge: empty drive line selected. Create an anonymous
> + * BlockBackend to avoid NULL deref with various BlockBackend
> + * API calls within this model (CVE-2021-20196).
> + * Due to the controller QOM model limitations, we don't
> + * attach the created to the controller.
>
The created to the controller
Something is missing here, maybe 'device'?
Thanks,
Darren.
> + */
> +cur_drv->blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL);
> +}
> +return cur_drv;
> }
>
> /* Status A register : 0x00 (read-only) */
> --
> 2.31.1
Re: [PATCH-for-6.2 v2 0/2] hw/block/fdc: Fix CVE-2021-20196
Hi Philippe, Apart from a nit on patch 1, all looks good, so: Reviewed-by: Darren Kenny Thanks, Darren. On Thursday, 2021-11-18 at 00:24:20 +01, Philippe Mathieu-Daudé wrote: > I'm not sure what happened to v1 from Prasad, so since we are > at rc2 I took a simpler approach to fix this CVE: create an > empty drive to satisfy the BlockBackend API calls. > > Added Alexander's reproducer along. > > v1: > https://lore.kernel.org/qemu-devel/20210123100345.642933-1-ppan...@redhat.com/ > > Alexander Bulekov (1): > tests/qtest/fdc-test: Add a regression test for CVE-2021-20196 > > Philippe Mathieu-Daudé (1): > hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196 > > hw/block/fdc.c | 14 +- > tests/qtest/fdc-test.c | 21 + > 2 files changed, 34 insertions(+), 1 deletion(-) > > -- > 2.31.1
Re: [PATCH-for-6.2 v2 1/2] hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196
On 11/18/21 11:44, Darren Kenny wrote:
> Hi Philippe,
>
> A small nit below, but otherwise looks good.
>
> On Thursday, 2021-11-18 at 00:24:21 +01, Philippe Mathieu-Daudé wrote:
>> Guest might select another drive on the bus by setting the
>> DRIVE_SEL bit of the DIGITAL OUTPUT REGISTER (DOR).
>> The current controller model doesn't expect a BlockBackend
>> to be NULL. A simple way to fix CVE-2021-20196 is to create
>> an empty BlockBackend when it is missing. All further
>> accesses will be safely handled, and the controller state
>> machines keep behaving correctly.
>>
>> Fixes: CVE-2021-20196
>> Reported-by: Gaoning Pan (Ant Security Light-Year Lab)
>> BugLink: https://bugs.launchpad.net/qemu/+bug/1912780
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/338
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> hw/block/fdc.c | 14 +-
>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index fa933cd3263..eab17e946d6 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -1161,7 +1161,19 @@ static FDrive *get_drv(FDCtrl *fdctrl, int unit)
>>
>> static FDrive *get_cur_drv(FDCtrl *fdctrl)
>> {
>> -return get_drv(fdctrl, fdctrl->cur_drv);
>> +FDrive *cur_drv = get_drv(fdctrl, fdctrl->cur_drv);
>> +
>> +if (!cur_drv->blk) {
>> +/*
>> + * Kludge: empty drive line selected. Create an anonymous
>> + * BlockBackend to avoid NULL deref with various BlockBackend
>> + * API calls within this model (CVE-2021-20196).
>> + * Due to the controller QOM model limitations, we don't
>> + * attach the created to the controller.
>>
> The created to the controller
>
> Something is missing here, maybe 'device'?
OK. I forgot to add Cc: qemu-sta...@nongnu.org so will respin.
Thanks,
Phil.
[PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507
Trivial fix for CVE-2021-3507. Philippe Mathieu-Daudé (2): hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507) tests/qtest/fdc-test: Add a regression test for CVE-2021-3507 hw/block/fdc.c | 8 tests/qtest/fdc-test.c | 20 2 files changed, 28 insertions(+) -- 2.31.1
[PATCH-for-6.2 1/2] hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507)
Per the 82078 datasheet, if the end-of-track (EOT byte in
the FIFO) is more than the number of sectors per side, the
command is terminated unsuccessfully:
* 5.2.5 DATA TRANSFER TERMINATION
The 82078 supports terminal count explicitly through
the TC pin and implicitly through the underrun/over-
run and end-of-track (EOT) functions. For full sector
transfers, the EOT parameter can define the last
sector to be transferred in a single or multisector
transfer. If the last sector to be transferred is a par-
tial sector, the host can stop transferring the data in
mid-sector, and the 82078 will continue to complete
the sector as if a hardware TC was received. The
only difference between these implicit functions and
TC is that they return "abnormal termination" result
status. Such status indications can be ignored if they
were expected.
* 6.1.3 READ TRACK
This command terminates when the EOT specified
number of sectors have been read. If the 82078
does not find an I D Address Mark on the diskette
after the second· occurrence of a pulse on the
INDX# pin, then it sets the IC code in Status Regis-
ter 0 to "01" (Abnormal termination), sets the MA bit
in Status Register 1 to "1", and terminates the com-
mand.
* 6.1.6 VERIFY
Refer to Table 6-6 and Table 6-7 for information
concerning the values of MT and EC versus SC and
EOT value.
* Table 6·6. Result Phase Table
* Table 6-7. Verify Command Result Phase Table
Fix by aborting the transfer when EOT > # Sectors Per Side.
Cc: qemu-sta...@nongnu.org
Cc: Hervé Poussineau
Fixes: baca51faff0 ("floppy driver: disk geometry auto detect")
Reported-by: Alexander Bulekov
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/339
Signed-off-by: Philippe Mathieu-Daudé
---
hw/block/fdc.c | 8
1 file changed, 8 insertions(+)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index fa933cd3263..d21b717b7d6 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1512,6 +1512,14 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int
direction)
int tmp;
fdctrl->data_len = 128 << (fdctrl->fifo[5] > 7 ? 7 : fdctrl->fifo[5]);
tmp = (fdctrl->fifo[6] - ks + 1);
+if (tmp < 0) {
+FLOPPY_DPRINTF("invalid EOT: %d\n", tmp);
+fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
+fdctrl->fifo[3] = kt;
+fdctrl->fifo[4] = kh;
+fdctrl->fifo[5] = ks;
+return;
+}
if (fdctrl->fifo[0] & 0x80)
tmp += fdctrl->fifo[6];
fdctrl->data_len *= tmp;
--
2.31.1
[PATCH-for-6.2 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-3507
Add the reproducer from https://gitlab.com/qemu-project/qemu/-/issues/339
Without the previous commit, when running 'make check-qtest-i386'
with QEMU configured with '--enable-sanitizers' we get:
==4028352==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x61962a00 at pc 0x5626d03c491a bp 0x7ffdb4199410 sp 0x7ffdb4198bc0
READ of size 786432 at 0x61962a00 thread T0
#0 0x5626d03c4919 in __asan_memcpy (qemu-system-i386+0x1e65919)
#1 0x5626d1c023cc in flatview_write_continue softmmu/physmem.c:2787:13
#2 0x5626d1bf0c0f in flatview_write softmmu/physmem.c:2822:14
#3 0x5626d1bf0798 in address_space_write softmmu/physmem.c:2914:18
#4 0x5626d1bf0f37 in address_space_rw softmmu/physmem.c:2924:16
#5 0x5626d1bf14c8 in cpu_physical_memory_rw softmmu/physmem.c:2933:5
#6 0x5626d0bd5649 in cpu_physical_memory_write
include/exec/cpu-common.h:82:5
#7 0x5626d0bd0a07 in i8257_dma_write_memory hw/dma/i8257.c:452:9
#8 0x5626d09f825d in fdctrl_transfer_handler hw/block/fdc.c:1616:13
#9 0x5626d0a048b4 in fdctrl_start_transfer hw/block/fdc.c:1539:13
#10 0x5626d09f4c3e in fdctrl_write_data hw/block/fdc.c:2266:13
#11 0x5626d09f22f7 in fdctrl_write hw/block/fdc.c:829:9
#12 0x5626d1c20bc5 in portio_write softmmu/ioport.c:207:17
0x61962a00 is located 0 bytes to the right of 512-byte region
[0x61962800,0x61962a00)
allocated by thread T0 here:
#0 0x5626d03c66ec in posix_memalign (qemu-system-i386+0x1e676ec)
#1 0x5626d2b988d4 in qemu_try_memalign util/oslib-posix.c:210:11
#2 0x5626d2b98b0c in qemu_memalign util/oslib-posix.c:226:27
#3 0x5626d09fbaf0 in fdctrl_realize_common hw/block/fdc.c:2341:20
#4 0x5626d0a150ed in isabus_fdc_realize hw/block/fdc-isa.c:113:5
#5 0x5626d2367935 in device_set_realized hw/core/qdev.c:531:13
SUMMARY: AddressSanitizer: heap-buffer-overflow (qemu-system-i386+0x1e65919)
in __asan_memcpy
Shadow bytes around the buggy address:
0x0c32800044f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c3280004500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c3280004510: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c3280004520: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c3280004530: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c3280004540:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c3280004550: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c3280004560: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c3280004570: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c3280004580: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c3280004590: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Heap left redzone: fa
Freed heap region: fd
==4028352==ABORTING
Reported-by: Alexander Bulekov
Signed-off-by: Philippe Mathieu-Daudé
---
tests/qtest/fdc-test.c | 20
1 file changed, 20 insertions(+)
diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
index 26b69f7c5cd..f164d972d10 100644
--- a/tests/qtest/fdc-test.c
+++ b/tests/qtest/fdc-test.c
@@ -546,6 +546,25 @@ static void fuzz_registers(void)
}
}
+static void test_cve_2021_3507(void)
+{
+QTestState *s;
+
+s = qtest_initf("-nographic -m 32M -nodefaults "
+"-drive file=%s,format=raw,if=floppy", test_image);
+qtest_outl(s, 0x9, 0x0a0206);
+qtest_outw(s, 0x3f4, 0x1600);
+qtest_outw(s, 0x3f4, 0x);
+qtest_outw(s, 0x3f4, 0x);
+qtest_outw(s, 0x3f4, 0x);
+qtest_outw(s, 0x3f4, 0x0200);
+qtest_outw(s, 0x3f4, 0x0200);
+qtest_outw(s, 0x3f4, 0x);
+qtest_outw(s, 0x3f4, 0x);
+qtest_outw(s, 0x3f4, 0x);
+qtest_quit(s);
+}
+
int main(int argc, char **argv)
{
int fd;
@@ -576,6 +595,7 @@ int main(int argc, char **argv)
qtest_add_func("/fdc/read_no_dma_18", test_read_no_dma_18);
qtest_add_func("/fdc/read_no_dma_19", test_read_no_dma_19);
qtest_add_func("/fdc/fuzz-registers", fuzz_registers);
+qtest_add_func("/fdc/fuzz/cve_2021_3507", test_cve_2021_3507);
ret = g_test_run();
--
2.31.1
[PATCH-for-6.2 v3 1/2] hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196
Guest might select another drive on the bus by setting the
DRIVE_SEL bit of the DIGITAL OUTPUT REGISTER (DOR).
The current controller model doesn't expect a BlockBackend
to be NULL. A simple way to fix CVE-2021-20196 is to create
an empty BlockBackend when it is missing. All further
accesses will be safely handled, and the controller state
machines keep behaving correctly.
Cc: qemu-sta...@nongnu.org
Fixes: CVE-2021-20196
Reported-by: Gaoning Pan (Ant Security Light-Year Lab)
BugLink: https://bugs.launchpad.net/qemu/+bug/1912780
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/338
Reviewed-by: Darren Kenny
Signed-off-by: Philippe Mathieu-Daudé
---
hw/block/fdc.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index d21b717b7d6..6f94b6a6daa 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1161,7 +1161,19 @@ static FDrive *get_drv(FDCtrl *fdctrl, int unit)
static FDrive *get_cur_drv(FDCtrl *fdctrl)
{
-return get_drv(fdctrl, fdctrl->cur_drv);
+FDrive *cur_drv = get_drv(fdctrl, fdctrl->cur_drv);
+
+if (!cur_drv->blk) {
+/*
+ * Kludge: empty drive line selected. Create an anonymous
+ * BlockBackend to avoid NULL deref with various BlockBackend
+ * API calls within this model (CVE-2021-20196).
+ * Due to the controller QOM model limitations, we don't
+ * attach the created to the controller device.
+ */
+cur_drv->blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL);
+}
+return cur_drv;
}
/* Status A register : 0x00 (read-only) */
--
2.31.1
[PATCH-for-6.2 v3 0/2] hw/block/fdc: Fix CVE-2021-20196
I'm not sure what happened to v1 from Prasad, so since we are at rc2 I took a simpler approach to fix this CVE: create an empty drive to satisfy the BlockBackend API calls. Added Alexander's reproducer along. Since v2: - Reword comment (Darren) - Add Darren R-b tag v2: https://lore.kernel.org/qemu-devel/2027232422.1026411-1-phi...@redhat.com/ v1: https://lore.kernel.org/qemu-devel/20210123100345.642933-1-ppan...@redhat.com/ Based-on: <2028115733.4038610-1-phi...@redhat.com> Alexander Bulekov (1): tests/qtest/fdc-test: Add a regression test for CVE-2021-20196 Philippe Mathieu-Daudé (1): hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196 hw/block/fdc.c | 14 +- tests/qtest/fdc-test.c | 21 + 2 files changed, 34 insertions(+), 1 deletion(-) -- 2.31.1
[PATCH-for-6.2 v3 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196
From: Alexander Bulekov
Without the previous commit, when running 'make check-qtest-i386'
with QEMU configured with '--enable-sanitizers' we get:
AddressSanitizer:DEADLYSIGNAL
=
==287878==ERROR: AddressSanitizer: SEGV on unknown address 0x0344
==287878==The signal is caused by a WRITE memory access.
==287878==Hint: address points to the zero page.
#0 0x564b2e5bac27 in blk_inc_in_flight block/block-backend.c:1346:5
#1 0x564b2e5bb228 in blk_pwritev_part block/block-backend.c:1317:5
#2 0x564b2e5bcd57 in blk_pwrite block/block-backend.c:1498:11
#3 0x564b2ca1cdd3 in fdctrl_write_data hw/block/fdc.c:2221:17
#4 0x564b2ca1b2f7 in fdctrl_write hw/block/fdc.c:829:9
#5 0x564b2dc49503 in portio_write softmmu/ioport.c:201:9
Add the reproducer for CVE-2021-20196.
Signed-off-by: Alexander Bulekov
Message-Id: <20210319050906.14875-2-alx...@bu.edu>
[PMD: Rebased, use global test_image]
Reviewed-by: Darren Kenny
Signed-off-by: Philippe Mathieu-Daudé
---
tests/qtest/fdc-test.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
index f164d972d10..0f8b9b753f4 100644
--- a/tests/qtest/fdc-test.c
+++ b/tests/qtest/fdc-test.c
@@ -565,6 +565,26 @@ static void test_cve_2021_3507(void)
qtest_quit(s);
}
+static void test_cve_2021_20196(void)
+{
+QTestState *s;
+
+s = qtest_initf("-nographic -m 32M -nodefaults "
+"-drive file=%s,format=raw,if=floppy", test_image);
+qtest_outw(s, 0x3f2, 0x0004);
+qtest_outw(s, 0x3f4, 0x0200);
+qtest_outw(s, 0x3f4, 0x);
+qtest_outw(s, 0x3f4, 0x);
+qtest_outw(s, 0x3f4, 0x);
+qtest_outw(s, 0x3f4, 0x);
+qtest_outw(s, 0x3f4, 0x);
+qtest_outw(s, 0x3f4, 0x);
+qtest_outw(s, 0x3f4, 0x);
+qtest_outw(s, 0x3f4, 0x);
+qtest_outw(s, 0x3f2, 0x0001);
+qtest_quit(s);
+}
+
int main(int argc, char **argv)
{
int fd;
@@ -596,6 +616,7 @@ int main(int argc, char **argv)
qtest_add_func("/fdc/read_no_dma_19", test_read_no_dma_19);
qtest_add_func("/fdc/fuzz-registers", fuzz_registers);
qtest_add_func("/fdc/fuzz/cve_2021_3507", test_cve_2021_3507);
+qtest_add_func("/fdc/fuzz/cve_2021_20196", test_cve_2021_20196);
ret = g_test_run();
--
2.31.1
Re: [PATCH v4 19/25] block_int-common.h: split function pointers in BlockDriver
On 15/11/2021 13:00, Hanna Reitz wrote:
+
+ /*
+ * I/O API functions. These functions are thread-safe.
+ *
+ * See include/block/block-io.h for more information about
+ * the I/O API.
+ */
+
+ int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
+ Error **errp);
+ int coroutine_fn (*bdrv_co_create_opts)(BlockDriver *drv,
+ const char *filename,
+ QemuOpts *opts,
+ Error **errp);
Now this is really interesting. Technically I suppose these should work
in any thread, but trying to do so results in:
$ touch /tmp/iothread-create-test.qcow2
$ ./qemu-system-x86_64 -object iothread,id=iothr0 -qmp stdio <{"execute":"blockdev-add","arguments":{"node-name":"proto","driver":"file","filename":"/tmp/iothread-create-test.qcow2"}}
{"execute":"x-blockdev-set-iothread","arguments":{"node-name":"proto","iothread":"iothr0"}}
{"execute":"blockdev-create","arguments":{"job-id":"create","options":{"driver":"qcow2","file":"proto","size":0}}}
EOF
{"QMP": {"version": {"qemu": {"micro": 90, "minor": 1, "major": 6},
"package": "v6.2.0-rc0-40-gd02d5fe5fb-dirty"}, "capabilities": ["oob"]}}
{"return": {}}
{"return": {}}
{"return": {}}
{"timestamp": {"seconds": 1636973542, "microseconds": 338117}, "event":
"JOB_STATUS_CHANGE", "data": {"status": "created", "id": "create"}}
{"timestamp": {"seconds": 1636973542, "microseconds": 338197}, "event":
"JOB_STATUS_CHANGE", "data": {"status": "running", "id": "create"}}
{"return": {}}
qemu: qemu_mutex_unlock_impl: Operation not permitted
[1] 86154 IOT instruction (core dumped) ./qemu-system-x86_64 -object
iothread,id=iothr0 -qmp stdio <<<''
So something’s fishy and perhaps we should investigate this... I mean,
I can’t really imagine a case where someone would need to run a
blockdev-create job in an I/O thread, but right now the interface allows
for it.
And then bdrv_create() is classified as global state, and also
bdrv_co_create_opts_simple(), which is supposed to be a drop-in function
for this .bdrv_co_create_opts function. So that can’t work.
Also, I believe there might have been some functions you classified as
GS that are called from .create* implementations. I accepted that,
given the abort I sketched above. However, if we classify image
creation as I/O, then those would need to be re-evaluated. For example,
qcow2_co_create_opts() calls bdrv_create_file(), which is a GS function.
Some of this issues could be addressed by making .bdrv_co_create_opts a
GS function and .bdrv_co_create an I/O function. I believe that would
be the ideal split, even though as shown above .bdrv_co_create doesn’t
work in an I/O thread, and then you have the issue of probably all
format drivers’ .bdrv_co_create implementations calling
bdrv_open_blockdev_ref(), which is a GS function.
(VMDK even calls blk_new_open(), blk_new_with_bs(), and blk_unref(),
none of which can ever be I/O functions, I think.)
I believe in practice the best is to for now classify all create-related
functions as GS functions. This is supported by the fact that
qmp_blockdev_create() specifically creates the create job in the main
context (with a TODO comment) and requires block drivers to error out
when they encounter a node in a different AioContext.
Ok after better reviewing this I agree with you:
- .bdrv_co_create_opts is for sure a GS function. It is called by
bdrv_create and it is asserted to be under BQL.
- .bdrv_co_create should also be a GS, and the easiest thing to do would
be to follow the existing TODO and make sure we cannot run it outside
the main loop. I think that I will put it as GS, and add the BQL
assertion to blockdev_create_run, so that if for some reasons someone
tries to do what you did above, will crash because of the assertion, and
not because of the aiocontext lock missing.
Emanuele
Re: does drive_get_next(IF_NONE) make sense?
On Tue, Nov 16, 2021 at 2:10 AM Thomas Huth wrote: > > On 15/11/2021 08.12, Alistair Francis wrote: > > On Mon, Nov 15, 2021 at 3:32 PM Markus Armbruster wrote: > >> > >> Peter Maydell writes: > >> > >>> On Fri, 12 Nov 2021 at 13:34, Markus Armbruster wrote: > > Thomas Huth writes: > > > On 03/11/2021 09.41, Markus Armbruster wrote: > >> Peter Maydell writes: > >> > >>> Does it make sense for a device/board to do drive_get_next(IF_NONE) ? > >> Short answer: hell, no! ;) > > > > Would it make sense to add an "assert(type != IF_NONE)" to drive_get() > > to avoid such mistakes in the future? > > Worth a try. > >>> > >>> You need to fix the sifive_u_otp device first :-) > >> > >> And for that, we may want Hao Wu's "[PATCH v4 5/7] blockdev: Add a new > >> IF type IF_OTHER" first. > > > > I can fixup sifive_u_otp, just let me know what the prefered solution is > > What kind of device is that OTP exactly? If it is some kind of non-serial > flash device, maybe you could simply use IF_PFLASH instead? It just says "one time programmable memory". I'm guessing it's sometype of eFuse. Alistair > > Thomas >
Re: does drive_get_next(IF_NONE) make sense?
On Thu, 18 Nov 2021 at 13:04, Alistair Francis wrote: > > On Tue, Nov 16, 2021 at 2:10 AM Thomas Huth wrote: > > What kind of device is that OTP exactly? If it is some kind of non-serial > > flash device, maybe you could simply use IF_PFLASH instead? > > It just says "one time programmable memory". I'm guessing it's > sometype of eFuse. We used IF_PFLASH for the Xilinx efuse devices we recently added. So we should probably use that for consistency, unless we want to instead say that efuses should be something other than IF_PFLASH. Either way it's a compatibility break for command lines, so we should probably try to have only one rather than two :-) -- PMM
Re: [PATCH v4 00/25] block layer: split block APIs in global state and I/O
On 11/15/21 17:03, Hanna Reitz wrote: I only really see four solutions for this: (1) We somehow make the amend job run in the main context under the BQL and have it prevent all concurrent I/O access (seems bad) (2) We can make the permission functions part of the I/O path (seems wrong and probably impossible?) (3) We can drop the permissions update and permanently require the permissions that we need when updating keys (I think this might break existing use cases) (4) We can acquire the BQL around the permission update call and perhaps that works? I don’t know how (4) would work but it’s basically the only reasonable solution I can come up with. Would this be a way to call a BQL function from an I/O function? I think that would deadlock: mainI/O thread - start bdrv_co_amend take BQL bdrv_drain ... hangs ... (2) is definitely wrong. (3) I have no idea. Would it be possible or meaningful to do the bdrv_child_refresh_perms in qmp_x_blockdev_amend? It seems that all users need it, and in general it seems weird to amend a qcow2 or luks header (and thus the meaning of parts of the file) while others can write to the same file. Paolo
Re: [PATCH v4 00/25] block layer: split block APIs in global state and I/O
On 11/15/21 17:03, Hanna Reitz wrote: and second fuse_do_truncate(), which calls blk_set_perm(). Here it seems that a non-growable export is still growable as long as nobody is watching. :) Is this the desired behavior? Paolo
[PATCH-for-6.2?] docs: Spell QEMU all caps
Replace Qemu -> QEMU. Signed-off-by: Philippe Mathieu-Daudé --- docs/devel/modules.rst| 2 +- docs/devel/multi-thread-tcg.rst | 2 +- docs/devel/style.rst | 2 +- docs/devel/ui.rst | 4 ++-- docs/interop/nbd.txt | 6 +++--- docs/interop/qcow2.txt| 8 docs/multiseat.txt| 2 +- docs/system/device-url-syntax.rst.inc | 2 +- docs/system/i386/sgx.rst | 26 +- docs/u2f.txt | 2 +- 10 files changed, 28 insertions(+), 28 deletions(-) diff --git a/docs/devel/modules.rst b/docs/devel/modules.rst index 066f347b89b..8e999c4fa48 100644 --- a/docs/devel/modules.rst +++ b/docs/devel/modules.rst @@ -1,5 +1,5 @@ -Qemu modules +QEMU modules .. kernel-doc:: include/qemu/module.h diff --git a/docs/devel/multi-thread-tcg.rst b/docs/devel/multi-thread-tcg.rst index 5b446ee08b6..c9541a7b20a 100644 --- a/docs/devel/multi-thread-tcg.rst +++ b/docs/devel/multi-thread-tcg.rst @@ -228,7 +228,7 @@ Emulated hardware state Currently thanks to KVM work any access to IO memory is automatically protected by the global iothread mutex, also known as the BQL (Big -Qemu Lock). Any IO region that doesn't use global mutex is expected to +QEMU Lock). Any IO region that doesn't use global mutex is expected to do its own locking. However IO memory isn't the only way emulated hardware state can be diff --git a/docs/devel/style.rst b/docs/devel/style.rst index 260e3263fa0..e00af62e763 100644 --- a/docs/devel/style.rst +++ b/docs/devel/style.rst @@ -686,7 +686,7 @@ Rationale: hex numbers are hard to read in logs when there is no 0x prefix, especially when (occasionally) the representation doesn't contain any letters and especially in one line with other decimal numbers. Number groups are allowed to not use '0x' because for some things notations like %x.%x.%x are used not -only in Qemu. Also dumping raw data bytes with '0x' is less readable. +only in QEMU. Also dumping raw data bytes with '0x' is less readable. '#' printf flag --- diff --git a/docs/devel/ui.rst b/docs/devel/ui.rst index 06c7d622ce7..17fb667dec4 100644 --- a/docs/devel/ui.rst +++ b/docs/devel/ui.rst @@ -1,8 +1,8 @@ = -Qemu UI subsystem +QEMU UI subsystem = -Qemu Clipboard +QEMU Clipboard -- .. kernel-doc:: include/ui/clipboard.h diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt index 10ce098a29b..bdb0f2a41ac 100644 --- a/docs/interop/nbd.txt +++ b/docs/interop/nbd.txt @@ -1,4 +1,4 @@ -Qemu supports the NBD protocol, and has an internal NBD client (see +QEMU supports the NBD protocol, and has an internal NBD client (see block/nbd.c), an internal NBD server (see blockdev-nbd.c), and an external NBD server tool (see qemu-nbd.c). The common code is placed in nbd/*. @@ -7,11 +7,11 @@ The NBD protocol is specified here: https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md The following paragraphs describe some specific properties of NBD -protocol realization in Qemu. +protocol realization in QEMU. = Metadata namespaces = -Qemu supports the "base:allocation" metadata context as defined in the +QEMU supports the "base:allocation" metadata context as defined in the NBD protocol specification, and also defines an additional metadata namespace "qemu". diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt index 0463f761efb..f7dc304ff69 100644 --- a/docs/interop/qcow2.txt +++ b/docs/interop/qcow2.txt @@ -313,7 +313,7 @@ The fields of the bitmaps extension are: The number of bitmaps contained in the image. Must be greater than or equal to 1. - Note: Qemu currently only supports up to 65535 bitmaps per + Note: QEMU currently only supports up to 65535 bitmaps per image. 4 - 7: Reserved, must be zero. @@ -775,7 +775,7 @@ Structure of a bitmap directory entry: 2: extra_data_compatible This flags is meaningful when the extra data is unknown to the software (currently any extra data is - unknown to Qemu). + unknown to QEMU). If it is set, the bitmap may be used as expected, extra data must be left as is. If it is not set, the bitmap must not be used, but @@ -793,7 +793,7 @@ Structure of a bitmap directory entry: 17:granularity_bits Granularity bits. Valid values: 0 - 63. -Note: Qemu currently supports only values 9 - 31. +Note: QEMU currently supports only values 9 - 31. Granularity is calculated as granularity = 1 << granula
[PATCH 1/3] block: better document SSH host key fingerprint checking
The docs still illustrate host key fingerprint checking using the old
md5 hashes which are considered insecure and obsolete. Change it to
illustrate using a sha256 hash. Also show how to extract the hash
value from the known_hosts file.
Signed-off-by: Daniel P. Berrangé
---
docs/system/qemu-block-drivers.rst.inc | 30 ++
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/docs/system/qemu-block-drivers.rst.inc
b/docs/system/qemu-block-drivers.rst.inc
index 16225710eb..2aeeaf6361 100644
--- a/docs/system/qemu-block-drivers.rst.inc
+++ b/docs/system/qemu-block-drivers.rst.inc
@@ -778,10 +778,32 @@ The optional *HOST_KEY_CHECK* parameter controls how the
remote
host's key is checked. The default is ``yes`` which means to use
the local ``.ssh/known_hosts`` file. Setting this to ``no``
turns off known-hosts checking. Or you can check that the host key
-matches a specific fingerprint:
-``host_key_check=md5:78:45:8e:14:57:4f:d5:45:83:0a:0e:f3:49:82:c9:c8``
-(``sha1:`` can also be used as a prefix, but note that OpenSSH
-tools only use MD5 to print fingerprints).
+matches a specific fingerprint. The fingerprint can be provided in
+``md5``, ``sha1``, or ``sha256`` format, however, it is strongly
+recommended to only use ``sha256``, since the other options are
+considered insecure by modern standards. The fingerprint value
+must be given as a hex encoded string::
+
+
host_key_check=sha256:04ce2ae89ff4295a6b9c4111640bdcb3297858ee55cb434d9dd88796e93aa795``
+
+The key string may optionally contain ":" separators between
+each pair of hex digits.
+
+The ``$HOME/.ssh/known_hosts`` file contains the base64 encoded
+host keys. These can be converted into the format needed for
+QEMU using a command such as::
+
+ $ for key in `grep 10.33.8.112 known_hosts | awk '{print $3}'`
+ do
+ echo $key | base64 -d | sha256sum
+ done
+ 6c3aa525beda9dc83eadfbd7e5ba7d976ecb59575d1633c87cd06ed2ed6e366f -
+ 12214fd9ea5b408086f98ecccd9958609bd9ac7c0ea316734006bc7818b45dc8 -
+ d36420137bcbd101209ef70c3b15dc07362fbe0fa53c5b135eba6e6afa82f0ce -
+
+Note that there can be multiple keys present per host, each with
+different key ciphers. Care is needed to pick the key fingerprint
+that matches the cipher QEMU will negotiate with the remote server.
Currently authentication must be done using ssh-agent. Other
authentication methods may be supported in future.
--
2.31.1
[PATCH 2/3] block: support sha256 fingerprint with pre-blockdev options
When support for sha256 fingerprint checking was aded in
commit bf783261f0aee6e81af3916bff7606d71ccdc153
Author: Daniel P. Berrangé
Date: Tue Jun 22 12:51:56 2021 +0100
block/ssh: add support for sha256 host key fingerprints
it was only made to work with -blockdev. Getting it working with
-drive requires some extra custom parsing.
Signed-off-by: Daniel P. Berrangé
---
block/ssh.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/block/ssh.c b/block/ssh.c
index e0fbb4934b..fcc0ab765a 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -556,6 +556,11 @@ static bool ssh_process_legacy_options(QDict *output_opts,
qdict_put_str(output_opts, "host-key-check.type", "sha1");
qdict_put_str(output_opts, "host-key-check.hash",
&host_key_check[5]);
+} else if (strncmp(host_key_check, "sha256:", 7) == 0) {
+qdict_put_str(output_opts, "host-key-check.mode", "hash");
+qdict_put_str(output_opts, "host-key-check.type", "sha256");
+qdict_put_str(output_opts, "host-key-check.hash",
+ &host_key_check[7]);
} else if (strcmp(host_key_check, "yes") == 0) {
qdict_put_str(output_opts, "host-key-check.mode", "known_hosts");
} else {
--
2.31.1
[PATCH 0/3] block: misc fixes & improvements for SSH block driver key fingerprints
* The docs were pointing people towards the obsolete and insecure MD5 fingerprint config instead of preferred sha256 * The sha256 fingerprint handling wasn't wired up into the legacy CLI parsing code * Finger print check failures were hard to diagnose due to limited info reported on error. Daniel P. Berrangé (3): block: better document SSH host key fingerprint checking block: support sha256 fingerprint with pre-blockdev options block: print the server key type and fingerprint on failure block/ssh.c| 42 +- docs/system/qemu-block-drivers.rst.inc | 30 +++--- 2 files changed, 61 insertions(+), 11 deletions(-) -- 2.31.1
[PATCH 3/3] block: print the server key type and fingerprint on failure
When validating the server key fingerprint fails, it is difficult for
the user to know what they got wrong. The fingerprint accepted by QEMU
is received in a different format than openssh displays. There can also
be keys for multiple different ciphers in known_hosts. It may not be
obvious which cipher QEMU will use and whether it will be the same
as openssh. Address this by printing the server key type and its
corresponding fingerprint in the format QEMU accepts.
Signed-off-by: Daniel P. Berrangé
---
block/ssh.c | 37 ++---
1 file changed, 30 insertions(+), 7 deletions(-)
diff --git a/block/ssh.c b/block/ssh.c
index fcc0ab765a..967a2b971e 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -386,14 +386,28 @@ static int compare_fingerprint(const unsigned char
*fingerprint, size_t len,
return *host_key_check - '\0';
}
+static char *format_fingerprint(const unsigned char *fingerprint, size_t len)
+{
+static const char *hex = "0123456789abcdef";
+char *ret = g_new0(char, (len * 2) + 1);
+for (size_t i = 0; i < len; i++) {
+ret[i * 2] = hex[((fingerprint[i] >> 4) & 0xf)];
+ret[(i * 2) + 1] = hex[(fingerprint[i] & 0xf)];
+}
+ret[len * 2] = '\0';
+return ret;
+}
+
static int
check_host_key_hash(BDRVSSHState *s, const char *hash,
-enum ssh_publickey_hash_type type, Error **errp)
+enum ssh_publickey_hash_type type, const char *typestr,
+Error **errp)
{
int r;
ssh_key pubkey;
unsigned char *server_hash;
size_t server_hash_len;
+const char *keytype;
r = ssh_get_server_publickey(s->session, &pubkey);
if (r != SSH_OK) {
@@ -401,6 +415,8 @@ check_host_key_hash(BDRVSSHState *s, const char *hash,
return -EINVAL;
}
+keytype = ssh_key_type_to_char(ssh_key_type(pubkey));
+
r = ssh_get_publickey_hash(pubkey, type, &server_hash, &server_hash_len);
ssh_key_free(pubkey);
if (r != 0) {
@@ -410,12 +426,16 @@ check_host_key_hash(BDRVSSHState *s, const char *hash,
}
r = compare_fingerprint(server_hash, server_hash_len, hash);
-ssh_clean_pubkey_hash(&server_hash);
if (r != 0) {
-error_setg(errp, "remote host key does not match host_key_check '%s'",
- hash);
+g_autofree char *server_fp = format_fingerprint(server_hash,
+server_hash_len);
+error_setg(errp, "remote host %s key fingerprint '%s:%s' "
+ "does not match host_key_check '%s:%s'",
+ keytype, typestr, server_fp, typestr, hash);
+ssh_clean_pubkey_hash(&server_hash);
return -EPERM;
}
+ssh_clean_pubkey_hash(&server_hash);
return 0;
}
@@ -436,13 +456,16 @@ static int check_host_key(BDRVSSHState *s,
SshHostKeyCheck *hkc, Error **errp)
case SSH_HOST_KEY_CHECK_MODE_HASH:
if (hkc->u.hash.type == SSH_HOST_KEY_CHECK_HASH_TYPE_MD5) {
return check_host_key_hash(s, hkc->u.hash.hash,
- SSH_PUBLICKEY_HASH_MD5, errp);
+ SSH_PUBLICKEY_HASH_MD5, "md5",
+ errp);
} else if (hkc->u.hash.type == SSH_HOST_KEY_CHECK_HASH_TYPE_SHA1) {
return check_host_key_hash(s, hkc->u.hash.hash,
- SSH_PUBLICKEY_HASH_SHA1, errp);
+ SSH_PUBLICKEY_HASH_SHA1, "sha1",
+ errp);
} else if (hkc->u.hash.type == SSH_HOST_KEY_CHECK_HASH_TYPE_SHA256) {
return check_host_key_hash(s, hkc->u.hash.hash,
- SSH_PUBLICKEY_HASH_SHA256, errp);
+ SSH_PUBLICKEY_HASH_SHA256, "sha256",
+ errp);
}
g_assert_not_reached();
break;
--
2.31.1
Re: [PATCH v2 12/13] hw/arm/aspeed: Replace drive_get_next() by drive_get()
On 11/17/21 17:34, Markus Armbruster wrote:
drive_get_next() is basically a bad idea. It returns the "next" block
backend of a certain interface type. "Next" means bus=0,unit=N, where
subsequent calls count N up from zero, per interface type.
This lets you define unit numbers implicitly by execution order. If the
order changes, or new calls appear "in the middle", unit numbers change.
ABI break. Hard to spot in review.
The aspeed machines connects backends with drive_get_next() in several
counting loops, one of them in a helper function, and a conditional.
Change it to use drive_get() directly. This makes the unit numbers
explicit in the code.
I hope we can introduce some bus id. At least for the SPI controllers.
Cc: "Cédric Le Goater"
Cc: Peter Maydell
Cc: Andrew Jeffery
Cc: Joel Stanley
Cc: qemu-...@nongnu.org
Signed-off-by: Markus Armbruster
---
Reviewed-by: Cédric Le Goater
Thanks,
C.
hw/arm/aspeed.c | 21 +
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index a77f46b3ad..cf20ae0db5 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -284,12 +284,13 @@ static void write_boot_rom(DriveInfo *dinfo, hwaddr addr,
size_t rom_size,
}
static void aspeed_board_init_flashes(AspeedSMCState *s,
- const char *flashtype)
+ const char *flashtype,
+ int unit0)
{
int i ;
for (i = 0; i < s->num_cs; ++i) {
-DriveInfo *dinfo = drive_get_next(IF_MTD);
+DriveInfo *dinfo = drive_get(IF_MTD, 0, unit0 + i);
qemu_irq cs_line;
DeviceState *dev;
@@ -382,10 +383,12 @@ static void aspeed_machine_init(MachineState *machine)
"max_ram", max_ram_size - machine->ram_size);
memory_region_add_subregion(&bmc->ram_container, machine->ram_size,
&bmc->max_ram);
-aspeed_board_init_flashes(&bmc->soc.fmc, bmc->fmc_model ?
- bmc->fmc_model : amc->fmc_model);
-aspeed_board_init_flashes(&bmc->soc.spi[0], bmc->spi_model ?
- bmc->spi_model : amc->spi_model);
+aspeed_board_init_flashes(&bmc->soc.fmc,
+ bmc->fmc_model ? bmc->fmc_model : amc->fmc_model,
+ 0);
+aspeed_board_init_flashes(&bmc->soc.spi[0],
+ bmc->spi_model ? bmc->spi_model : amc->spi_model,
+ bmc->soc.fmc.num_cs);
/* Install first FMC flash content as a boot rom. */
if (drive0) {
@@ -435,11 +438,13 @@ static void aspeed_machine_init(MachineState *machine)
}
for (i = 0; i < bmc->soc.sdhci.num_slots; i++) {
-sdhci_attach_drive(&bmc->soc.sdhci.slots[i], drive_get_next(IF_SD));
+sdhci_attach_drive(&bmc->soc.sdhci.slots[i],
+ drive_get(IF_SD, 0, i));
}
if (bmc->soc.emmc.num_slots) {
-sdhci_attach_drive(&bmc->soc.emmc.slots[0], drive_get_next(IF_SD));
+sdhci_attach_drive(&bmc->soc.emmc.slots[0],
+ drive_get(IF_SD, 0, bmc->soc.sdhci.num_slots));
}
arm_load_kernel(ARM_CPU(first_cpu), machine, &aspeed_board_binfo);
[PATCH-for-6.2?] docs: Render binary names as monospaced text
Signed-off-by: Philippe Mathieu-Daudé --- docs/about/removed-features.rst| 8 docs/devel/build-system.rst| 6 +++--- docs/devel/multi-process.rst | 6 +++--- docs/devel/testing.rst | 8 docs/image-fuzzer.txt | 6 +++--- docs/system/arm/orangepi.rst | 2 +- docs/system/images.rst | 2 +- docs/system/qemu-block-drivers.rst.inc | 6 +++--- docs/system/tls.rst| 2 +- docs/tools/qemu-img.rst| 18 +- docs/tools/qemu-nbd.rst| 4 ++-- docs/tools/qemu-storage-daemon.rst | 7 --- docs/tools/virtiofsd.rst | 4 ++-- 13 files changed, 40 insertions(+), 39 deletions(-) diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst index 9d0d90c90d9..c02d1f6d494 100644 --- a/docs/about/removed-features.rst +++ b/docs/about/removed-features.rst @@ -658,8 +658,8 @@ enforce that any failure to open the backing image (including if the backing file is missing or an incorrect format was specified) is an error when ``-u`` is not used. -qemu-img amend to adjust backing file (removed in 6.1) -'' +``qemu-img`` amend to adjust backing file (removed in 6.1) +'' The use of ``qemu-img amend`` to modify the name or format of a qcow2 backing image was never fully documented or tested, and interferes @@ -670,8 +670,8 @@ backing chain should be performed with ``qemu-img rebase -u`` either before or after the remaining changes being performed by amend, as appropriate. -qemu-img backing file without format (removed in 6.1) -' +``qemu-img`` backing file without format (removed in 6.1) +' The use of ``qemu-img create``, ``qemu-img rebase``, or ``qemu-img convert`` to create or modify an image that depends on a backing file diff --git a/docs/devel/build-system.rst b/docs/devel/build-system.rst index 7a83f5fc0db..431caba7aa0 100644 --- a/docs/devel/build-system.rst +++ b/docs/devel/build-system.rst @@ -121,11 +121,11 @@ process for: 1) executables, which include: - - Tools - qemu-img, qemu-nbd, qga (guest agent), etc + - Tools - ``qemu-img``, ``qemu-nbd``, ``qga`` (guest agent), etc - - System emulators - qemu-system-$ARCH + - System emulators - ``qemu-system-$ARCH`` - - Userspace emulators - qemu-$ARCH + - Userspace emulators - ``qemu-$ARCH`` - Unit tests diff --git a/docs/devel/multi-process.rst b/docs/devel/multi-process.rst index e5758a79aba..2c5ec977df8 100644 --- a/docs/devel/multi-process.rst +++ b/docs/devel/multi-process.rst @@ -187,9 +187,9 @@ desired, in which the emulation application should only be allowed to access the files or devices the VM it's running on behalf of can access. qemu-io model -Qemu-io is a test harness used to test changes to the QEMU block backend -object code. (e.g., the code that implements disk images for disk driver -emulation) Qemu-io is not a device emulation application per se, but it +``qemu-io`` is a test harness used to test changes to the QEMU block backend +object code (e.g., the code that implements disk images for disk driver +emulation). ``qemu-io`` is not a device emulation application per se, but it does compile the QEMU block objects into a separate binary from the main QEMU one. This could be useful for disk device emulation, since its emulation applications will need to include the QEMU block objects. diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index 60c59023e58..755343c7dd0 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -564,11 +564,11 @@ exploiting a QEMU security bug to compromise the host. QEMU binaries ~ -By default, qemu-system-x86_64 is searched in $PATH to run the guest. If there -isn't one, or if it is older than 2.10, the test won't work. In this case, +By default, ``qemu-system-x86_64`` is searched in $PATH to run the guest. If +there isn't one, or if it is older than 2.10, the test won't work. In this case, provide the QEMU binary in env var: ``QEMU=/path/to/qemu-2.10+``. -Likewise the path to qemu-img can be set in QEMU_IMG environment variable. +Likewise the path to ``qemu-img`` can be set in QEMU_IMG environment variable. Make jobs ~ @@ -650,7 +650,7 @@ supported. To start the fuzzer, run tests/image-fuzzer/runner.py -c '[["qemu-img", "info", "$test_img"]]' /tmp/test qcow2 -Alternatively, some command different from "qemu-img info" can be tested, by +Alternatively, some command different from ``qemu-img info`` can be tested, by changing the ``-c`` option. Integration tests using the Avocado Framework diff --git a/docs/image-fuzzer.txt b/docs/image-fuzzer.txt index 3e23ebec331..279cc8c807f 100644 --- a/d
Re: [PATCH v2 10/13] hw/arm/xlnx-zcu102: Replace drive_get_next() by drive_get()
On Wed, Nov 17, 2021 at 05:34:06PM +0100, Markus Armbruster wrote:
> drive_get_next() is basically a bad idea. It returns the "next" block
> backend of a certain interface type. "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
>
> This lets you define unit numbers implicitly by execution order. If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break. Hard to spot in review.
>
> Machine "xlnx-zcu102" connects backends with drive_get_next() in
> several counting loops. Change it to use drive_get() directly. This
> makes the unit numbers explicit in the code.
Acked-by: Edgar E. Iglesias
>
> Cc: Alistair Francis
> Cc: "Edgar E. Iglesias"
> Cc: Peter Maydell
> Cc: qemu-...@nongnu.org
> Signed-off-by: Markus Armbruster
> ---
> hw/arm/xlnx-zcu102.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> index 3dc2b5e8ca..45eb19ab3b 100644
> --- a/hw/arm/xlnx-zcu102.c
> +++ b/hw/arm/xlnx-zcu102.c
> @@ -169,7 +169,7 @@ static void xlnx_zcu102_init(MachineState *machine)
> /* Create and plug in the SD cards */
> for (i = 0; i < XLNX_ZYNQMP_NUM_SDHCI; i++) {
> BusState *bus;
> -DriveInfo *di = drive_get_next(IF_SD);
> +DriveInfo *di = drive_get(IF_SD, 0, i);
> BlockBackend *blk = di ? blk_by_legacy_dinfo(di) : NULL;
> DeviceState *carddev;
> char *bus_name;
> @@ -190,7 +190,7 @@ static void xlnx_zcu102_init(MachineState *machine)
> BusState *spi_bus;
> DeviceState *flash_dev;
> qemu_irq cs_line;
> -DriveInfo *dinfo = drive_get_next(IF_MTD);
> +DriveInfo *dinfo = drive_get(IF_MTD, 0, i);
> gchar *bus_name = g_strdup_printf("spi%d", i);
>
> spi_bus = qdev_get_child_bus(DEVICE(&s->soc), bus_name);
> @@ -212,7 +212,7 @@ static void xlnx_zcu102_init(MachineState *machine)
> BusState *spi_bus;
> DeviceState *flash_dev;
> qemu_irq cs_line;
> -DriveInfo *dinfo = drive_get_next(IF_MTD);
> +DriveInfo *dinfo = drive_get(IF_MTD, 0, XLNX_ZYNQMP_NUM_SPIS + i);
> int bus = i / XLNX_ZYNQMP_NUM_QSPI_BUS_CS;
> gchar *bus_name = g_strdup_printf("qspi%d", bus);
>
> --
> 2.31.1
>
Re: [PATCH v2 11/13] hw/arm/xilinx_zynq: Replace drive_get_next() by drive_get()
On Wed, Nov 17, 2021 at 05:34:07PM +0100, Markus Armbruster wrote:
> drive_get_next() is basically a bad idea. It returns the "next" block
> backend of a certain interface type. "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
>
> This lets you define unit numbers implicitly by execution order. If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break. Hard to spot in review.
>
> Machine "xlnx-zcu102" connects backends with drive_get_next() in two
> counting loops, one of them in a helper function. Change it to use
> drive_get() directly. This makes the unit numbers explicit in the
> code.
Acked-by: Edgar E. Iglesias
>
> Cc: "Edgar E. Iglesias"
> Cc: Alistair Francis
> Cc: Peter Maydell
> Cc: qemu-...@nongnu.org
> Signed-off-by: Markus Armbruster
> ---
> hw/arm/xilinx_zynq.c | 16 +---
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 69c333e91b..50e7268396 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -125,9 +125,10 @@ static void gem_init(NICInfo *nd, uint32_t base,
> qemu_irq irq)
> sysbus_connect_irq(s, 0, irq);
> }
>
> -static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
> - bool is_qspi)
> +static inline int zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
> +bool is_qspi, int unit0)
> {
> +int unit = unit0;
> DeviceState *dev;
> SysBusDevice *busdev;
> SSIBus *spi;
> @@ -156,7 +157,7 @@ static inline void zynq_init_spi_flashes(uint32_t
> base_addr, qemu_irq irq,
> spi = (SSIBus *)qdev_get_child_bus(dev, bus_name);
>
> for (j = 0; j < num_ss; ++j) {
> -DriveInfo *dinfo = drive_get_next(IF_MTD);
> +DriveInfo *dinfo = drive_get(IF_MTD, 0, unit++);
> flash_dev = qdev_new("n25q128");
> if (dinfo) {
> qdev_prop_set_drive_err(flash_dev, "drive",
> @@ -170,6 +171,7 @@ static inline void zynq_init_spi_flashes(uint32_t
> base_addr, qemu_irq irq,
> }
> }
>
> +return unit;
> }
>
> static void zynq_init(MachineState *machine)
> @@ -247,9 +249,9 @@ static void zynq_init(MachineState *machine)
> pic[n] = qdev_get_gpio_in(dev, n);
> }
>
> -zynq_init_spi_flashes(0xE0006000, pic[58-IRQ_OFFSET], false);
> -zynq_init_spi_flashes(0xE0007000, pic[81-IRQ_OFFSET], false);
> -zynq_init_spi_flashes(0xE000D000, pic[51-IRQ_OFFSET], true);
> +n = zynq_init_spi_flashes(0xE0006000, pic[58 - IRQ_OFFSET], false, 0);
> +n = zynq_init_spi_flashes(0xE0007000, pic[81 - IRQ_OFFSET], false, n);
> +n = zynq_init_spi_flashes(0xE000D000, pic[51 - IRQ_OFFSET], true, n);
>
> sysbus_create_simple(TYPE_CHIPIDEA, 0xE0002000, pic[53 - IRQ_OFFSET]);
> sysbus_create_simple(TYPE_CHIPIDEA, 0xE0003000, pic[76 - IRQ_OFFSET]);
> @@ -298,7 +300,7 @@ static void zynq_init(MachineState *machine)
> sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, hci_addr);
> sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[hci_irq -
> IRQ_OFFSET]);
>
> -di = drive_get_next(IF_SD);
> +di = drive_get(IF_SD, 0, n);
> blk = di ? blk_by_legacy_dinfo(di) : NULL;
> carddev = qdev_new(TYPE_SD_CARD);
> qdev_prop_set_drive_err(carddev, "drive", blk, &error_fatal);
> --
> 2.31.1
>
Re: [PATCH v2 09/13] hw/microblaze: Replace drive_get_next() by drive_get()
On Wed, Nov 17, 2021 at 05:34:05PM +0100, Markus Armbruster wrote:
> drive_get_next() is basically a bad idea. It returns the "next" block
> backend of a certain interface type. "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
>
> This lets you define unit numbers implicitly by execution order. If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break. Hard to spot in review.
>
> Machine "petalogix-ml605" connects backends with drive_get_next() in a
> counting loop. Change it to use drive_get() directly. This makes the
> unit numbers explicit in the code.
Acked-by: Edgar E. Iglesias
>
> Cc: "Edgar E. Iglesias"
> Signed-off-by: Markus Armbruster
> ---
> hw/microblaze/petalogix_ml605_mmu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/microblaze/petalogix_ml605_mmu.c
> b/hw/microblaze/petalogix_ml605_mmu.c
> index 159db6cbe2..a24fadddca 100644
> --- a/hw/microblaze/petalogix_ml605_mmu.c
> +++ b/hw/microblaze/petalogix_ml605_mmu.c
> @@ -183,7 +183,7 @@ petalogix_ml605_init(MachineState *machine)
> spi = (SSIBus *)qdev_get_child_bus(dev, "spi");
>
> for (i = 0; i < NUM_SPI_FLASHES; i++) {
> -DriveInfo *dinfo = drive_get_next(IF_MTD);
> +DriveInfo *dinfo = drive_get(IF_MTD, 0, i);
> qemu_irq cs_line;
>
> dev = qdev_new("n25q128");
> --
> 2.31.1
>
Re: [PATCH v2 08/13] hw/arm/xlnx-versal-virt: Replace drive_get_next() by drive_get()
On Wed, Nov 17, 2021 at 05:34:04PM +0100, Markus Armbruster wrote:
> drive_get_next() is basically a bad idea. It returns the "next" block
> backend of a certain interface type. "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
>
> This lets you define unit numbers implicitly by execution order. If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break. Hard to spot in review.
>
> Machine "xlnx-versal-virt" connects backends with drive_get_next() in
> a counting loop. Change it to use drive_get() directly. This makes
> the unit numbers explicit in the code.
Acked-by: Edgar E. Iglesias
>
> Cc: Alistair Francis
> Cc: "Edgar E. Iglesias"
> Cc: Peter Maydell
> Cc: qemu-...@nongnu.org
> Signed-off-by: Markus Armbruster
> ---
> hw/arm/xlnx-versal-virt.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
> index d2f55e29b6..0c5edc898e 100644
> --- a/hw/arm/xlnx-versal-virt.c
> +++ b/hw/arm/xlnx-versal-virt.c
> @@ -669,7 +669,8 @@ static void versal_virt_init(MachineState *machine)
>
> /* Plugin SD cards. */
> for (i = 0; i < ARRAY_SIZE(s->soc.pmc.iou.sd); i++) {
> -sd_plugin_card(&s->soc.pmc.iou.sd[i], drive_get_next(IF_SD));
> +sd_plugin_card(&s->soc.pmc.iou.sd[i],
> + drive_get(IF_SD, 0, i));
> }
>
> s->binfo.ram_size = machine->ram_size;
> --
> 2.31.1
>
Re: [PATCH] block/vvfat.c fix leak when failure occurs
On 18.11.21 10:33, Daniella Lee wrote: Thanks for your reply and your suggestion is useful. This is my first submission, and I will pay attention to these issues in the future. There are many other places you mentioned need to be modified, do I need to resubmit the patch, or you want to modify them with other codes? Hi, Yes, you should send a v2 that addresses the issues. As for the suggestions that concern places outside of `vvfat_open()`: Most of them need not be your concern if you don’t want them to be, but we definitely need to have `s->used_clusters` be allocated with `g_malloc0()`, or we can’t free it with `g_free()`. (We could free it with `free()`, but that would be a suboptimal solution...) So that allocation line in `enable_write_target()` should be fixed in v2. The best way to do that would be to do it in an own patch (i.e. patch 1 changes that allocation, and patch 2 is this patch), but I wouldn’t mind it too much if you do both changes in a single patch. Regarding the other suggestions for other places: It would be nice to drop the clean-up path in `enable_write_target()`, but isn’t necessary. If you want to do it, you can do it as part of this patch, or on top in another patch, but you can also choose just to ignore that bit. (And maybe then I’ll send a patch later.) The fact that we’re leaking two of these buffers in `vvfat_close()` is definitely unrelated to this patch, so that’s also nothing you have to care about if you don’t want to. I hope that made it clear...? Don’t hesitate to ask more if it didn’t (or for any other questions you might have). Hanna On Thu, Nov 18, 2021 at 4:34 PM Hanna Reitz wrote: On 16.11.21 13:57, Daniella Lee wrote: > Function vvfat_open called function enable_write_target and init_directories, > and these functions malloc new memory for BDRVVVFATState::qcow_filename, > BDRVVVFATState::used_clusters, and BDRVVVFATState::cluster_buff. > > When the specified folder does not exist ,it may contains memory leak. > After init_directories function is executed, the vvfat_open return -EIO, > and bdrv_open_driver goto label open_failed, > the program use g_free(bs->opaque) to release BDRVVVFATState struct > without members mentioned. > > command line: > qemu-system-x86_64 -hdb -usb -device usb-storage,drive=fat16 > -drive file=fat:rw:fat-type=16:"", > id=fat16,format=raw,if=none > > enable_write_target called: > (gdb) bt > #0 enable_write_target (bs=0x56f9f000, errp=0x7fffd780) > at ../block/vvfat.c:3114 > #1 vvfat_open (bs=0x56f9f000, options=0x56fa45d0, > flags=155650, errp=0x7fffd780) at ../block/vvfat.c:1236 > #2 bdrv_open_driver (bs=0x56f9f000, drv=0x56c47920 , > node_name=0x0, options=0x56fa45d0, open_flags=155650, > errp=0x7fffd890) at ../block.c:1558 > #3 bdrv_open_common (bs=0x56f9f000, file=0x0, options=0x56fa45d0, > errp=0x7fffd890) at ../block.c:1852 > #4 bdrv_open_inherit (filename=0x56f73310 "fat:rw:", > reference=0x0, options=0x56fa45d0, flags=40962, parent=0x56f98cd0, > child_class=0x56b1d6a0 , child_role=19, > errp=0x7fffda90) at ../block.c:3779 > #5 bdrv_open_child_bs (filename=0x56f73310 "fat:rw:", > options=0x56f9cfc0, bdref_key=0x56239bb8 "file", > parent=0x56f98cd0, child_class=0x56b1d6a0 , > child_role=19, allow_none=true, errp=0x7fffda90) at ../block.c:3419 > #6 bdrv_open_inherit (filename=0x56f73310 "fat:rw:", > reference=0x0, options=0x56f9cfc0, flags=8194, parent=0x0, > child_class=0x0, child_role=0, errp=0x56c98c40 ) > at ../block.c:3726 > #7 bdrv_open (filename=0x56f73310 "fat:rw:", reference=0x0, > options=0x56f757b0, flags=0, errp=0x56c98c40 ) > at ../block.c:3872 > #8 blk_new_open (filename=0x56f73310 "fat:rw:", reference=0x0, > options=0x56f757b0, flags=0, errp=0x56c98c40 ) > at ../block/block-backend.c:436 > #9 blockdev_init (file=0x56f73310 "fat:rw:", > bs_opts=0x56f757b0, errp=0x56c98c40 ) > at ../blockdev.c:608 > #10 drive_new (all_opts=0x56d2b700, block_default_type=IF_IDE, > errp=0x56c98c40 ) at ../blockdev.c:992 > .. > > Signed-off-by: Daniella Lee > --- > block/vvfat.c | 15 +++ > 1 file changed, 15 insertions(+) Hi, Thanks for your patch! Yes, that makes sense. I believe there are some issues that should be addressed, though: > diff --git a/block/vvfat.c b/block/vvfat.c > index 05e78e3c27..454a74c5d5 100644 > --- a/block/vvfat.c > +++ b/block/vvfat.c > @@ -1280,7 +1280,22 @@ static int vvfat_op
Re: [PATCH v4 08/25] block: introduce assert_bdrv_graph_writable
On 18.11.21 10:55, Emanuele Giuseppe Esposito wrote:
On 12/11/2021 15:40, Hanna Reitz wrote:
On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:
We want to be sure that the functions that write the child and
parent list of a bs are under BQL and drain.
BQL prevents from concurrent writings from the GS API, while
drains protect from I/O.
TODO: drains are missing in some functions using this assert.
Therefore a proper assertion will fail. Because adding drains
requires additional discussions, they will be added in future
series.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Paolo Bonzini
Reviewed-by: Stefan Hajnoczi
---
block.c | 5 +
block/io.c | 11 +++
include/block/block_int-global-state.h | 10 +-
3 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 41c5883c5c..94bff5c757 100644
--- a/block.c
+++ b/block.c
@@ -2734,12 +2734,14 @@ static void
bdrv_replace_child_noperm(BdrvChild *child,
if (child->klass->detach) {
child->klass->detach(child);
}
+ assert_bdrv_graph_writable(old_bs);
QLIST_REMOVE(child, next_parent);
I think this belongs above the .detach() call (and the QLIST_REMOVE()
belongs into the .detach() implementation, as done in
https://lists.nongnu.org/archive/html/qemu-block/2021-11/msg00240.html,
which has been merged to Kevin’s block branch).
Yes, I rebased on kwolf/block branch. Thank you for pointing that out.
}
child->bs = new_bs;
if (new_bs) {
+ assert_bdrv_graph_writable(new_bs);
QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
In both these places it’s a bit strange that the assertion is done on
the child nodes. The subgraph starting from them isn’t modified
after all, so their subgraph technically doesn’t need to be
writable. I think a single assertion on the parent node would be
preferable.
I presume the problem with that is that we don’t have the parent node
here? Do we need a new BdrvChildClass method that performs this
assertion on the parent node?
Uhm I am not sure what you mean here.
Just to recap on how I see this: the assertion
assert_bdrv_graph_writable(bs) is basically used to make sure we are
protecting the write on some fields (childrens and parents lists in
this patch) of a given @bs.
Oh, OK. I understood it to mean that the subgraph starting at `bs` is
mutable, i.e. including all of its recursive children.
And yes, you’re right, the child BDSs are indeed modified, too, so we
should check them, too.
It should work like a rwlock: reading is allowed to be concurrent, but
a write should stop all readers to prevent concurrency issues. This is
achieved by draining.
Draining works on a subgraph, so I suppose it does mean that the whole
subgraph will be mutable. But no matter, at least new_bs is not in the
parent’s subgraph, so it wouldn’t be included in the check if we only
checked the parent.
Let's use the first case that you point out, old_bs (it's specular for
new_bs):
>> + assert_bdrv_graph_writable(old_bs);
>> QLIST_REMOVE(child, next_parent);
So old_bs should be the child "son" (child->bs), meaning
old_bs->parents contains the child. Therefore when a child is removed
by old_bs, we need to be sure we are doing it safely.
So we should check that if old_bs exists, old_bs should be drained, to
prevent any other iothread from reading the ->parents list that is
being updated.
The only thing to keep in mind in this case is that just wrapping a
drain around that won't be enough, because then the child won't be
included in the drain_end(old_bs). Therefore the right way to cover
this drain-wise once the assertion also checks for drains is:
drain_begin(old_bs)
assert_bdrv_graph_writable(old_bs)
QLIST_REMOVE(child, next_parent)
/* old_bs will be under drain_end, but not the child */
bdrv_parent_drained_end_single(child);
bdrv_drained_end(old_bs);
I think you agree on this so far.
Now I think your concern is related to the child "parent", namely
child->opaque. The problem is that in the .detach and .attach
callbacks we are firstly adding/removing the child from the list, and
then calling drain on the subtree.
It was my impression that you’d want bdrv_replace_child_noperm() to
always be called in a section where the subgraph starting from the
parent BDS is drained, not just the child BDSs that are swapped (old_bs
and new_bs).
My abstract concern is that bdrv_replace_child_noperm() does not modify
only old_bs and new_bs, but actually modifies the whole subgraph
starting at the parent node. A concrete example for this is that we
modify not only the children’s parent lists, but also the parent’s
children list.
That’s why I’m asking why we only check that the graph is writable for
the children, when actually I feel like we’re modifying the parent, too.
We
Re: [PATCH v4 00/25] block layer: split block APIs in global state and I/O
On 18.11.21 15:04, Paolo Bonzini wrote: On 11/15/21 17:03, Hanna Reitz wrote: and second fuse_do_truncate(), which calls blk_set_perm(). Here it seems that a non-growable export is still growable as long as nobody is watching. :) Is this the desired behavior? Yes, absolutely. “Growable” is documented to mean that writes after the end of the exported file will grow it to fit. Explicit truncating is something else, and I believe we should allow it on all writable exports. (Of course only when other potential users of the block node in question allow it to be resized, but that’s what the permission is for.) Hanna
Re: [PATCH v4 00/25] block layer: split block APIs in global state and I/O
On 18.11.21 14:50, Paolo Bonzini wrote: On 11/15/21 17:03, Hanna Reitz wrote: I only really see four solutions for this: (1) We somehow make the amend job run in the main context under the BQL and have it prevent all concurrent I/O access (seems bad) (2) We can make the permission functions part of the I/O path (seems wrong and probably impossible?) (3) We can drop the permissions update and permanently require the permissions that we need when updating keys (I think this might break existing use cases) (4) We can acquire the BQL around the permission update call and perhaps that works? I don’t know how (4) would work but it’s basically the only reasonable solution I can come up with. Would this be a way to call a BQL function from an I/O function? I think that would deadlock: main I/O thread - start bdrv_co_amend take BQL bdrv_drain ... hangs ... :/ Is there really nothing we can do? Forgive me if I’m talking complete nonsense here (because frankly I don’t even really know what a bottom half is exactly), but can’t we schedule some coroutine in the main thread to do the perm notifications and wait for them in the I/O thread? (2) is definitely wrong. (3) I have no idea. Would it be possible or meaningful to do the bdrv_child_refresh_perms in qmp_x_blockdev_amend? It seems that all users need it, and in general it seems weird to amend a qcow2 or luks header (and thus the meaning of parts of the file) while others can write to the same file. Hmm... Perhaps. We would need to undo the permission change when the job finishes, though, i.e. in JobDriver.prepare() or JobDriver.clean(). Doing the change in qmp_x_blockdev_amend() would be asymmetric then, so we’d probably want a new JobDriver method that runs in the main thread before .run() is invoked. (Unfortunately, “.prepare()” is now taken already...) Doesn’t solve the FUSE problem, but there we could try to just take the RESIZE permission permanently and if that fails, we just don’t allow truncates for that export. Not nice, but should work for common cases. Hanna
Re: [PATCH-for-6.2?] docs: Render binary names as monospaced text
On Thursday, 2021-11-18 at 15:43:17 +01, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Darren Kenny > --- > docs/about/removed-features.rst| 8 > docs/devel/build-system.rst| 6 +++--- > docs/devel/multi-process.rst | 6 +++--- > docs/devel/testing.rst | 8 > docs/image-fuzzer.txt | 6 +++--- > docs/system/arm/orangepi.rst | 2 +- > docs/system/images.rst | 2 +- > docs/system/qemu-block-drivers.rst.inc | 6 +++--- > docs/system/tls.rst| 2 +- > docs/tools/qemu-img.rst| 18 +- > docs/tools/qemu-nbd.rst| 4 ++-- > docs/tools/qemu-storage-daemon.rst | 7 --- > docs/tools/virtiofsd.rst | 4 ++-- > 13 files changed, 40 insertions(+), 39 deletions(-) > > diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst > index 9d0d90c90d9..c02d1f6d494 100644 > --- a/docs/about/removed-features.rst > +++ b/docs/about/removed-features.rst > @@ -658,8 +658,8 @@ enforce that any failure to open the backing image > (including if the > backing file is missing or an incorrect format was specified) is an > error when ``-u`` is not used. > > -qemu-img amend to adjust backing file (removed in 6.1) > -'' > +``qemu-img`` amend to adjust backing file (removed in 6.1) > +'' > > The use of ``qemu-img amend`` to modify the name or format of a qcow2 > backing image was never fully documented or tested, and interferes > @@ -670,8 +670,8 @@ backing chain should be performed with ``qemu-img rebase > -u`` either > before or after the remaining changes being performed by amend, as > appropriate. > > -qemu-img backing file without format (removed in 6.1) > -' > +``qemu-img`` backing file without format (removed in 6.1) > +' > > The use of ``qemu-img create``, ``qemu-img rebase``, or ``qemu-img > convert`` to create or modify an image that depends on a backing file > diff --git a/docs/devel/build-system.rst b/docs/devel/build-system.rst > index 7a83f5fc0db..431caba7aa0 100644 > --- a/docs/devel/build-system.rst > +++ b/docs/devel/build-system.rst > @@ -121,11 +121,11 @@ process for: > > 1) executables, which include: > > - - Tools - qemu-img, qemu-nbd, qga (guest agent), etc > + - Tools - ``qemu-img``, ``qemu-nbd``, ``qga`` (guest agent), etc > > - - System emulators - qemu-system-$ARCH > + - System emulators - ``qemu-system-$ARCH`` > > - - Userspace emulators - qemu-$ARCH > + - Userspace emulators - ``qemu-$ARCH`` > > - Unit tests > > diff --git a/docs/devel/multi-process.rst b/docs/devel/multi-process.rst > index e5758a79aba..2c5ec977df8 100644 > --- a/docs/devel/multi-process.rst > +++ b/docs/devel/multi-process.rst > @@ -187,9 +187,9 @@ desired, in which the emulation application should only > be allowed to > access the files or devices the VM it's running on behalf of can access. > qemu-io model > > -Qemu-io is a test harness used to test changes to the QEMU block backend > -object code. (e.g., the code that implements disk images for disk driver > -emulation) Qemu-io is not a device emulation application per se, but it > +``qemu-io`` is a test harness used to test changes to the QEMU block backend > +object code (e.g., the code that implements disk images for disk driver > +emulation). ``qemu-io`` is not a device emulation application per se, but it > does compile the QEMU block objects into a separate binary from the main > QEMU one. This could be useful for disk device emulation, since its > emulation applications will need to include the QEMU block objects. > diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst > index 60c59023e58..755343c7dd0 100644 > --- a/docs/devel/testing.rst > +++ b/docs/devel/testing.rst > @@ -564,11 +564,11 @@ exploiting a QEMU security bug to compromise the host. > QEMU binaries > ~ > > -By default, qemu-system-x86_64 is searched in $PATH to run the guest. If > there > -isn't one, or if it is older than 2.10, the test won't work. In this case, > +By default, ``qemu-system-x86_64`` is searched in $PATH to run the guest. If > +there isn't one, or if it is older than 2.10, the test won't work. In this > case, > provide the QEMU binary in env var: ``QEMU=/path/to/qemu-2.10+``. > > -Likewise the path to qemu-img can be set in QEMU_IMG environment variable. > +Likewise the path to ``qemu-img`` can be set in QEMU_IMG environment > variable. > > Make jobs > ~ > @@ -650,7 +650,7 @@ supported. To start the fuzzer, run > >tests/image-fuzzer/runner.py -c '[["qemu-img", "info", "$test_img"]]' > /tmp/test qcow2 > > -Alternatively, som
Re: [PATCH-for-6.2?] docs: Spell QEMU all caps
On Thursday, 2021-11-18 at 15:34:01 +01, Philippe Mathieu-Daudé wrote: > Replace Qemu -> QEMU. > > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Darren Kenny > --- > docs/devel/modules.rst| 2 +- > docs/devel/multi-thread-tcg.rst | 2 +- > docs/devel/style.rst | 2 +- > docs/devel/ui.rst | 4 ++-- > docs/interop/nbd.txt | 6 +++--- > docs/interop/qcow2.txt| 8 > docs/multiseat.txt| 2 +- > docs/system/device-url-syntax.rst.inc | 2 +- > docs/system/i386/sgx.rst | 26 +- > docs/u2f.txt | 2 +- > 10 files changed, 28 insertions(+), 28 deletions(-) > > diff --git a/docs/devel/modules.rst b/docs/devel/modules.rst > index 066f347b89b..8e999c4fa48 100644 > --- a/docs/devel/modules.rst > +++ b/docs/devel/modules.rst > @@ -1,5 +1,5 @@ > > -Qemu modules > +QEMU modules > > > .. kernel-doc:: include/qemu/module.h > diff --git a/docs/devel/multi-thread-tcg.rst b/docs/devel/multi-thread-tcg.rst > index 5b446ee08b6..c9541a7b20a 100644 > --- a/docs/devel/multi-thread-tcg.rst > +++ b/docs/devel/multi-thread-tcg.rst > @@ -228,7 +228,7 @@ Emulated hardware state > > Currently thanks to KVM work any access to IO memory is automatically > protected by the global iothread mutex, also known as the BQL (Big > -Qemu Lock). Any IO region that doesn't use global mutex is expected to > +QEMU Lock). Any IO region that doesn't use global mutex is expected to > do its own locking. > > However IO memory isn't the only way emulated hardware state can be > diff --git a/docs/devel/style.rst b/docs/devel/style.rst > index 260e3263fa0..e00af62e763 100644 > --- a/docs/devel/style.rst > +++ b/docs/devel/style.rst > @@ -686,7 +686,7 @@ Rationale: hex numbers are hard to read in logs when > there is no 0x prefix, > especially when (occasionally) the representation doesn't contain any letters > and especially in one line with other decimal numbers. Number groups are > allowed > to not use '0x' because for some things notations like %x.%x.%x are used not > -only in Qemu. Also dumping raw data bytes with '0x' is less readable. > +only in QEMU. Also dumping raw data bytes with '0x' is less readable. > > '#' printf flag > --- > diff --git a/docs/devel/ui.rst b/docs/devel/ui.rst > index 06c7d622ce7..17fb667dec4 100644 > --- a/docs/devel/ui.rst > +++ b/docs/devel/ui.rst > @@ -1,8 +1,8 @@ > = > -Qemu UI subsystem > +QEMU UI subsystem > = > > -Qemu Clipboard > +QEMU Clipboard > -- > > .. kernel-doc:: include/ui/clipboard.h > diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt > index 10ce098a29b..bdb0f2a41ac 100644 > --- a/docs/interop/nbd.txt > +++ b/docs/interop/nbd.txt > @@ -1,4 +1,4 @@ > -Qemu supports the NBD protocol, and has an internal NBD client (see > +QEMU supports the NBD protocol, and has an internal NBD client (see > block/nbd.c), an internal NBD server (see blockdev-nbd.c), and an > external NBD server tool (see qemu-nbd.c). The common code is placed > in nbd/*. > @@ -7,11 +7,11 @@ The NBD protocol is specified here: > https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md > > The following paragraphs describe some specific properties of NBD > -protocol realization in Qemu. > +protocol realization in QEMU. > > = Metadata namespaces = > > -Qemu supports the "base:allocation" metadata context as defined in the > +QEMU supports the "base:allocation" metadata context as defined in the > NBD protocol specification, and also defines an additional metadata > namespace "qemu". > > diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt > index 0463f761efb..f7dc304ff69 100644 > --- a/docs/interop/qcow2.txt > +++ b/docs/interop/qcow2.txt > @@ -313,7 +313,7 @@ The fields of the bitmaps extension are: > The number of bitmaps contained in the image. Must be > greater than or equal to 1. > > - Note: Qemu currently only supports up to 65535 bitmaps per > + Note: QEMU currently only supports up to 65535 bitmaps per > image. > >4 - 7: Reserved, must be zero. > @@ -775,7 +775,7 @@ Structure of a bitmap directory entry: >2: extra_data_compatible > This flags is meaningful when the extra data is > unknown to the software (currently any extra data is > - unknown to Qemu). > + unknown to QEMU). > If it is set, the bitmap may be used as expected, > extra > data must be left as is. > If it is not set, the bitmap must not be used, but > @@ -793,7 +793,7 @@ Structure of a bitmap directory entry: >
Re: [PATCH-for-6.2?] docs: Spell QEMU all caps
Philippe Mathieu-Daudé writes: > Replace Qemu -> QEMU. > > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Markus Armbruster
Re: [PATCH-for-6.2?] docs: Render binary names as monospaced text
On Thu, Nov 18, 2021 at 03:43:17PM +0100, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé > --- > +++ b/docs/about/removed-features.rst > @@ -658,8 +658,8 @@ enforce that any failure to open the backing image > (including if the > backing file is missing or an incorrect format was specified) is an > error when ``-u`` is not used. > > -qemu-img amend to adjust backing file (removed in 6.1) > -'' > +``qemu-img`` amend to adjust backing file (removed in 6.1) > +'' Why quote just qemu-img here, > > The use of ``qemu-img amend`` to modify the name or format of a qcow2 when the context is obvious that it is the 'qemu-img amend' subcommand? > backing image was never fully documented or tested, and interferes > @@ -670,8 +670,8 @@ backing chain should be performed with ``qemu-img rebase > -u`` either > before or after the remaining changes being performed by amend, as > appropriate. > > -qemu-img backing file without format (removed in 6.1) > -' > +``qemu-img`` backing file without format (removed in 6.1) > +' This one makes sense, though, as "backing" is not a qemu-img subcommand. > > The use of ``qemu-img create``, ``qemu-img rebase``, or ``qemu-img > convert`` to create or modify an image that depends on a backing file > diff --git a/docs/devel/build-system.rst b/docs/devel/build-system.rst > index 7a83f5fc0db..431caba7aa0 100644 > --- a/docs/devel/build-system.rst > +++ b/docs/tools/qemu-nbd.rst > @@ -38,7 +38,7 @@ driver options if ``--image-opts`` is specified. >supported. The common object types that it makes sense to define are the >``secret`` object, which is used to supply passwords and/or encryption >keys, and the ``tls-creds`` object, which is used to supply TLS > - credentials for the qemu-nbd server or client. > + credentials for the ``qemu-nbd`` server or client. > > .. option:: -p, --port=PORT > > @@ -238,7 +238,7 @@ daemon: > Expose the guest-visible contents of a qcow2 file via a block device > /dev/nbd0 (and possibly creating /dev/nbd0p1 and friends for > partitions found within), then disconnect the device when done. > -Access to bind qemu-nbd to an /dev/nbd device generally requires root > +Access to bind ``qemu-nbd`` to an /dev/nbd device generally requires root As long as you're touching this line, s/an/a/. > privileges, and may also require the execution of ``modprobe nbd`` > to enable the kernel NBD client module. *CAUTION*: Do not use > this method to mount filesystems from an untrusted guest image - a Overall looks like a nice changeset. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PULL 0/5] Python patches
On Thu, Nov 18, 2021 at 1:46 AM Gerd Hoffmann wrote: > Hi, > > > - Split python/qemu/qmp out into its own repository and begin uploading > it > > to PyPI, as a test. (Do not delete python/qemu/qmp yet at this phase.) > > I think you can do that as two separate steps. > > pip can install from vcs too, i.e. when splitted to a separate repo but > not yet uploaded to pypi you can simply drop something like ... > > git+https://gitlab.com/qemu/qemu-python.git@master > > ... into pip-requirements.txt. That way you can easily test things > before actually uploading to pypi. > > Indeed - a limitation here however is that pip will not install from this source unless explicitly asked to, so you couldn't use this package as a requirement for another one, for example -- but it works as a testing step. but that's the rough outline of where I am headed and what I think needs to be done to get there. It's just taking me a while to get everything put in order exactly the right way to be able to flip the switch. Hopefully soon, though. I realized when re-reading my mails last night that I said I wouldn't be able to do it until "next release" but what I really meant was "until the next development window". --js
Re: [PATCH v2 2/2] iotests/149: Skip on unsupported ciphers
On 17.11.21 16:46, Daniel P. Berrangé wrote: On Wed, Nov 17, 2021 at 04:17:07PM +0100, Hanna Reitz wrote: Whenever qemu-img or qemu-io report that some cipher is unsupported, skip the whole test, because that is probably because qemu has been configured with the gnutls crypto backend. We could taylor the algorithm list to what gnutls supports, but this is a test that is run rather rarely anyway (because it requires password-less sudo), and so it seems better and easier to skip it. When this test is intentionally run to check LUKS compatibility, it seems better not to limit the algorithms but keep the list extensive. I'd really like to figure out a way to be able to partially run this test. When I have hit problems in the past, I needed to run specific tests, but then the expected output always contains everything. I've thought of a few options - Split it into many stanadlone tests - eg tests/qemu-iotests/tests/luks-host-$ALG I wouldn’t hate it, though we should have some common file where common code can be sourced from. - Split only the expected output eg 149-$SUBTEST and have a way to indicate which of expected output files we need to concatenate for the set of subtests that we run. I’d prefer it if the test could verify its own output so that the reference output is basically just the usual unittest output of dots, “Ran XX tests” and “OK”. (Two reasons: You can then easily disable some tests with the reference output changing only slightly; and it makes reviewing a test much easier because then I don’t need to verify the reference output...) - Introduce some template syntax in expected output tha can be used to munge the output. - Stop comparing expected output entirely and just then this into a normal python unit test. That’s something that might indeed be useful for unittest-style iotests. Then again, we already allow them to skip any test case and it will be counted as success, is that not sufficient? - Insert your idea here ? I personally most prefer unittest-style tests, because with them you can just %s/def test_/def xtest_/, then reverse this change for all the cases you want to run, and then adjust the reference output to match the number of tests run. So I suppose the best idea I have is to convert this test into unittest style, and then it should be more modular when it comes to what subtests it wants to run. I mean, it doesn’t have to truly be an iotests.QMPTestCase. It would be sufficient if the test itself verified the output of every command it invokes (instead of leaving that to a separate reference output file) and then printed something like “OK” afterwards. Then we could trivially skip some cases just by printing “OK” even if they weren’t run. Hanna
Re: [PATCH-for-6.2?] docs: Render binary names as monospaced text
On 11/18/21 16:46, Eric Blake wrote: > On Thu, Nov 18, 2021 at 03:43:17PM +0100, Philippe Mathieu-Daudé wrote: >> Signed-off-by: Philippe Mathieu-Daudé >> --- > >> +++ b/docs/about/removed-features.rst >> @@ -658,8 +658,8 @@ enforce that any failure to open the backing image >> (including if the >> backing file is missing or an incorrect format was specified) is an >> error when ``-u`` is not used. >> >> -qemu-img amend to adjust backing file (removed in 6.1) >> -'' >> +``qemu-img`` amend to adjust backing file (removed in 6.1) >> +'' > > Why quote just qemu-img here, > >> >> The use of ``qemu-img amend`` to modify the name or format of a qcow2 > > when the context is obvious that it is the 'qemu-img amend' subcommand? Because I was not careful enough :) >> backing image was never fully documented or tested, and interferes >> @@ -670,8 +670,8 @@ backing chain should be performed with ``qemu-img rebase >> -u`` either >> before or after the remaining changes being performed by amend, as >> appropriate. >> >> -qemu-img backing file without format (removed in 6.1) >> -' >> +``qemu-img`` backing file without format (removed in 6.1) >> +' > > This one makes sense, though, as "backing" is not a qemu-img subcommand. > >> >> The use of ``qemu-img create``, ``qemu-img rebase``, or ``qemu-img >> convert`` to create or modify an image that depends on a backing file >> diff --git a/docs/devel/build-system.rst b/docs/devel/build-system.rst >> index 7a83f5fc0db..431caba7aa0 100644 >> --- a/docs/devel/build-system.rst >> +++ b/docs/tools/qemu-nbd.rst >> @@ -38,7 +38,7 @@ driver options if ``--image-opts`` is specified. >>supported. The common object types that it makes sense to define are the >>``secret`` object, which is used to supply passwords and/or encryption >>keys, and the ``tls-creds`` object, which is used to supply TLS >> - credentials for the qemu-nbd server or client. >> + credentials for the ``qemu-nbd`` server or client. >> >> .. option:: -p, --port=PORT >> >> @@ -238,7 +238,7 @@ daemon: >> Expose the guest-visible contents of a qcow2 file via a block device >> /dev/nbd0 (and possibly creating /dev/nbd0p1 and friends for >> partitions found within), then disconnect the device when done. >> -Access to bind qemu-nbd to an /dev/nbd device generally requires root >> +Access to bind ``qemu-nbd`` to an /dev/nbd device generally requires root > > As long as you're touching this line, s/an/a/. OK. >> privileges, and may also require the execution of ``modprobe nbd`` >> to enable the kernel NBD client module. *CAUTION*: Do not use >> this method to mount filesystems from an untrusted guest image - a > > Overall looks like a nice changeset. >
[PATCH-for-6.2? v2] docs: Render binary names as monospaced text
Reviewed-by: Darren Kenny Signed-off-by: Philippe Mathieu-Daudé --- v2: Addressed Eric comments --- docs/about/removed-features.rst| 8 docs/devel/build-system.rst| 6 +++--- docs/devel/multi-process.rst | 6 +++--- docs/devel/testing.rst | 8 docs/image-fuzzer.txt | 6 +++--- docs/system/arm/orangepi.rst | 2 +- docs/system/images.rst | 2 +- docs/system/qemu-block-drivers.rst.inc | 6 +++--- docs/system/tls.rst| 2 +- docs/tools/qemu-img.rst| 18 +- docs/tools/qemu-nbd.rst| 4 ++-- docs/tools/qemu-storage-daemon.rst | 7 --- docs/tools/virtiofsd.rst | 4 ++-- 13 files changed, 40 insertions(+), 39 deletions(-) diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst index 9d0d90c90d9..d42c3341dee 100644 --- a/docs/about/removed-features.rst +++ b/docs/about/removed-features.rst @@ -658,8 +658,8 @@ enforce that any failure to open the backing image (including if the backing file is missing or an incorrect format was specified) is an error when ``-u`` is not used. -qemu-img amend to adjust backing file (removed in 6.1) -'' +``qemu-img amend`` to adjust backing file (removed in 6.1) +'' The use of ``qemu-img amend`` to modify the name or format of a qcow2 backing image was never fully documented or tested, and interferes @@ -670,8 +670,8 @@ backing chain should be performed with ``qemu-img rebase -u`` either before or after the remaining changes being performed by amend, as appropriate. -qemu-img backing file without format (removed in 6.1) -' +``qemu-img`` backing file without format (removed in 6.1) +' The use of ``qemu-img create``, ``qemu-img rebase``, or ``qemu-img convert`` to create or modify an image that depends on a backing file diff --git a/docs/devel/build-system.rst b/docs/devel/build-system.rst index 7a83f5fc0db..431caba7aa0 100644 --- a/docs/devel/build-system.rst +++ b/docs/devel/build-system.rst @@ -121,11 +121,11 @@ process for: 1) executables, which include: - - Tools - qemu-img, qemu-nbd, qga (guest agent), etc + - Tools - ``qemu-img``, ``qemu-nbd``, ``qga`` (guest agent), etc - - System emulators - qemu-system-$ARCH + - System emulators - ``qemu-system-$ARCH`` - - Userspace emulators - qemu-$ARCH + - Userspace emulators - ``qemu-$ARCH`` - Unit tests diff --git a/docs/devel/multi-process.rst b/docs/devel/multi-process.rst index e5758a79aba..2c5ec977df8 100644 --- a/docs/devel/multi-process.rst +++ b/docs/devel/multi-process.rst @@ -187,9 +187,9 @@ desired, in which the emulation application should only be allowed to access the files or devices the VM it's running on behalf of can access. qemu-io model -Qemu-io is a test harness used to test changes to the QEMU block backend -object code. (e.g., the code that implements disk images for disk driver -emulation) Qemu-io is not a device emulation application per se, but it +``qemu-io`` is a test harness used to test changes to the QEMU block backend +object code (e.g., the code that implements disk images for disk driver +emulation). ``qemu-io`` is not a device emulation application per se, but it does compile the QEMU block objects into a separate binary from the main QEMU one. This could be useful for disk device emulation, since its emulation applications will need to include the QEMU block objects. diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index 60c59023e58..755343c7dd0 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -564,11 +564,11 @@ exploiting a QEMU security bug to compromise the host. QEMU binaries ~ -By default, qemu-system-x86_64 is searched in $PATH to run the guest. If there -isn't one, or if it is older than 2.10, the test won't work. In this case, +By default, ``qemu-system-x86_64`` is searched in $PATH to run the guest. If +there isn't one, or if it is older than 2.10, the test won't work. In this case, provide the QEMU binary in env var: ``QEMU=/path/to/qemu-2.10+``. -Likewise the path to qemu-img can be set in QEMU_IMG environment variable. +Likewise the path to ``qemu-img`` can be set in QEMU_IMG environment variable. Make jobs ~ @@ -650,7 +650,7 @@ supported. To start the fuzzer, run tests/image-fuzzer/runner.py -c '[["qemu-img", "info", "$test_img"]]' /tmp/test qcow2 -Alternatively, some command different from "qemu-img info" can be tested, by +Alternatively, some command different from ``qemu-img info`` can be tested, by changing the ``-c`` option. Integration tests using the Avocado Framework diff --git a/docs/image-fuzzer.txt b/docs/ima
Re: [PATCH v4 00/25] block layer: split block APIs in global state and I/O
El jue., 18 nov. 2021 16:31, Hanna Reitz escribió: > On 18.11.21 14:50, Paolo Bonzini wrote: > > On 11/15/21 17:03, Hanna Reitz wrote: > >> > >> I only really see four solutions for this: > >> (1) We somehow make the amend job run in the main context under the > >> BQL and have it prevent all concurrent I/O access (seems bad) > >> (2) We can make the permission functions part of the I/O path (seems > >> wrong and probably impossible?) > >> (3) We can drop the permissions update and permanently require the > >> permissions that we need when updating keys (I think this might break > >> existing use cases) > >> (4) We can acquire the BQL around the permission update call and > >> perhaps that works? > >> > >> I don’t know how (4) would work but it’s basically the only > >> reasonable solution I can come up with. Would this be a way to call > >> a BQL function from an I/O function? > > > > I think that would deadlock: > > > > mainI/O thread > > - > > start bdrv_co_amend > > take BQL > > bdrv_drain > > ... hangs ... > > :/ > > Is there really nothing we can do? Forgive me if I’m talking complete > nonsense here (because frankly I don’t even really know what a bottom > half is exactly), but can’t we schedule some coroutine in the main > thread to do the perm notifications and wait for them in the I/O thread? > I think you still get a deadlock, just one with a longer chain. You still have a cycle of things depending on each other, but one of them is now the I/O thread waiting for the bottom half. Hmm... Perhaps. We would need to undo the permission change when the > job finishes, though, i.e. in JobDriver.prepare() or JobDriver.clean(). > Doing the change in qmp_x_blockdev_amend() would be asymmetric then, so > we’d probably want a new JobDriver method that runs in the main thread > before .run() is invoked. (Unfortunately, “.prepare()” is now taken > already...) > Ok at least it's feasible. Doesn’t solve the FUSE problem, but there we could try to just take the > RESIZE permission permanently and if that fails, we just don’t allow > truncates for that export. Not nice, but should work for common cases. > Yeah definitely not nice. Probably permissions could be protected by their own mutex, even a global one like the one we have for jobs. For now I suggest just ignoring the problem and adding a comment, since it's not really something that didn't exist. Paolo
