bwlp/qemu.git/block, branch master

block-backend: avoid bdrv_unregister_buf() NULL pointer deref

2022-11-29T23:15:26+00:00

bdrv_*() APIs expect a valid BlockDriverState. Calling them with bs=NULL leads to undefined behavior. Jonathan Cameron reported this following NULL pointer dereference when a VM with a virtio-blk device and a memory-backend-file object is terminated: 1. qemu_cleanup() closes all drives, setting blk->root to NULL 2. qemu_cleanup() calls user_creatable_cleanup(), which results in a RAM block notifier callback because the memory-backend-file is destroyed. 3. blk_unregister_buf() is called by virtio-blk's BlockRamRegistrar notifier callback and undefined behavior occurs. Fixes: baf422684d73 ("virtio-blk: use BDRV_REQ_REGISTERED_BUF optimization hint") Co-authored-by: Jonathan Cameron Reviewed-by: Kevin Wolf Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Stefan Hajnoczi Message-Id: <20221121211923.1993171-1-stefanha@redhat.com>

block: Start/end drain on correct AioContext

2022-11-10T13:58:43+00:00

bdrv_parent_drained_{begin,end}_single() are supposed to operate on the parent, not on the child, so they should not attempt to get the context to poll from the child but the parent instead. BDRV_POLL_WHILE(c->bs) does get the context from the child, so we should replace it with AIO_WAIT_WHILE() on the parent's context instead. This problem becomes apparent when bdrv_replace_child_noperm() invokes bdrv_parent_drained_end_single() after removing a child from a subgraph that is in an I/O thread. By the time bdrv_parent_drained_end_single() is called, child->bs is NULL, and so BDRV_POLL_WHILE(c->bs, ...) will poll the main loop instead of the I/O thread; but anything that bdrv_parent_drained_end_single_no_poll() may have scheduled is going to want to run in the I/O thread, but because we poll the main loop, the I/O thread is never unpaused, and nothing is run, resulting in a deadlock. Closes: https://gitlab.com/qemu-project/qemu/-/issues/1215 Reviewed-by: Kevin Wolf Signed-off-by: Hanna Reitz Message-Id: <20221107151321.211175-4-hreitz@redhat.com> Signed-off-by: Kevin Wolf

block-backend: Update ctx immediately after root

2022-11-10T13:58:38+00:00

blk_get_aio_context() asserts that blk->ctx is always equal to the root BDS's context (if there is a root BDS). Therefore, blk_do_set_aio_context() must update blk->ctx immediately after the root BDS's context has changed. Without this patch, the next patch would break iotest 238, because bdrv_drained_begin() (called by blk_do_set_aio_context()) may then invoke bdrv_child_get_parent_aio_context() on the root child, i.e. blk_get_aio_context(). However, by this point, blk->ctx would not have been updated and thus differ from the root node's context. This patch fixes that. Reviewed-by: Kevin Wolf Signed-off-by: Hanna Reitz Message-Id: <20221107151321.211175-3-hreitz@redhat.com> Signed-off-by: Kevin Wolf

block: Make bdrv_child_get_parent_aio_context I/O

2022-11-10T13:58:34+00:00

We want to use bdrv_child_get_parent_aio_context() from bdrv_parent_drained_{begin,end}_single(), both of which are "I/O or GS" functions. Prior to 3ed4f708fe1, all the implementations were I/O code anyway. 3ed4f708fe1 has put block jobs' AioContext field under the job mutex, so to make child_job_get_parent_aio_context() work in an I/O context, we need to take that lock there. Furthermore, blk_root_get_parent_aio_context() is not marked as anything, but is safe to run in an I/O context, so mark it that way now. (blk_get_aio_context() is an I/O code function.) With that done, all implementations explicitly are I/O code, so we can mark bdrv_child_get_parent_aio_context() as I/O code, too, so callers know it is safe to run from both GS and I/O contexts. Signed-off-by: Hanna Reitz Message-Id: <20221107151321.211175-2-hreitz@redhat.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf

block/blkio: Set BlockDriver::has_variable_length to false

2022-11-10T13:52:36+00:00

Setting it to true can cause the device size to be queried from libblkio in otherwise fast paths, degrading performance. Set it to false and require users to refresh the device size explicitly instead. Fixes: 4c8f4fda0504 ("block/blkio: Tolerate device size changes") Suggested-by: Kevin Wolf Signed-off-by: Alberto Faria Message-Id: <20221108144433.1334074-1-afaria@redhat.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf

block/mirror: Fix NULL s->job in active writes

2022-11-10T12:33:55+00:00

There is a small gap in mirror_start_job() before putting the mirror filter node into the block graph (bdrv_append() call) and the actual job being created. Before the job is created, MirrorBDSOpaque.job is NULL. It is possible that requests come in when bdrv_drained_end() is called, and those requests would see MirrorBDSOpaque.job == NULL. Have our filter node handle that case gracefully. Signed-off-by: Hanna Reitz Message-Id: <20221109165452.67927-4-hreitz@redhat.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf

block/mirror: Drop mirror_wait_for_any_operation()

2022-11-10T12:33:47+00:00

mirror_wait_for_free_in_flight_slot() is the only remaining user of mirror_wait_for_any_operation(), so inline the latter into the former. Signed-off-by: Hanna Reitz Message-Id: <20221109165452.67927-3-hreitz@redhat.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf

block/mirror: Do not wait for active writes

2022-11-10T12:33:41+00:00

Waiting for all active writes to settle before daring to create a background copying operation means that we will never do background operations while the guest does anything (in write-blocking mode), and therefore cannot converge. Yes, we also will not diverge, but actually converging would be even nicer. It is unclear why we did decide to wait for all active writes to settle before creating a background operation, but it just does not seem necessary. Active writes will put themselves into the in_flight bitmap and thus properly block actually conflicting background requests. It is important for active requests to wait on overlapping background requests, which we do in active_write_prepare(). However, so far it was not documented why it is important. Add such documentation now, and also to the other call of mirror_wait_on_conflicts(), so that it becomes more clear why and when requests need to actively wait for other requests to settle. Another thing to note is that of course we need to ensure that there are no active requests when the job completes, but that is done by virtue of the BDS being drained anyway, so there cannot be any active requests at that point. With this change, we will need to explicitly keep track of how many bytes are in flight in active requests so that job_progress_set_remaining() in mirror_run() can set the correct number of remaining bytes. Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2123297 Signed-off-by: Hanna Reitz Message-Id: <20221109165452.67927-2-hreitz@redhat.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf

dmg: warn when opening dmg images containing blocks of unknown type

2022-11-06T08:48:50+00:00

Signed-off-by: Kevin Wolf Signed-off-by: Claudio Fontana Reviewed-by: Richard Henderson Message-Id: <20220929093035.4231-5-cfontana@suse.de> Signed-off-by: Paolo Bonzini

module: add Error arguments to module_load and module_load_qom

2022-11-06T08:48:50+00:00

improve error handling during module load, by changing: bool module_load(const char *prefix, const char *lib_name); void module_load_qom(const char *type); to: int module_load(const char *prefix, const char *name, Error **errp); int module_load_qom(const char *type, Error **errp); where the return value is: -1 on module load error, and errp is set with the error 0 on module or one of its dependencies are not installed 1 on module load success 2 on module load success (module already loaded or built-in) module_load_qom_one has been introduced in: commit 28457744c345 ("module: qom module support"), which built on top of module_load_one, but discarded the bool return value. Restore it. Adapt all callers to emit errors, or ignore them, or fail hard, as appropriate in each context. Replace the previous emission of errors via fprintf in _some_ error conditions with Error and error_report, so as to emit to the appropriate target. A memory leak is also fixed as part of the module_load changes. audio: when attempting to load an audio module, report module load errors. Note that still for some callers, a single issue may generate multiple error reports, and this could be improved further. Regarding the audio code itself, audio_add() seems to ignore errors, and this should probably be improved. block: when attempting to load a block module, report module load errors. For the code paths that already use the Error API, take advantage of those to report module load errors into the Error parameter. For the other code paths, we currently emit the error, but this could be improved further by adding Error parameters to all possible code paths. console: when attempting to load a display module, report module load errors. qdev: when creating a new qdev Device object (DeviceState), report load errors. If a module cannot be loaded to create that device, now abort execution (if no CONFIG_MODULE) or exit (if CONFIG_MODULE). qom/object.c: when initializing a QOM object, or looking up class_by_name, report module load errors. qtest: when processing the "module_load" qtest command, report errors in the load of the module. Signed-off-by: Claudio Fontana Reviewed-by: Richard Henderson Message-Id: <20220929093035.4231-4-cfontana@suse.de> Signed-off-by: Paolo Bonzini