The following changes since commit 42f6c9179be4401974dd3a75ee72defd16b5092d:

Merge tag 'pull-ppc-20211112' of https://github.com/legoater/qemu into staging (2021-11-12 12:28:25 +0100)

https://gitlab.com/hreitz/qemu.git tags/pull-block-2021-11-16

for you to fetch changes up to 5dbd0ce115c7720268e653fe928bb6a0c1314a80:

file-posix: Fix alignment after reopen changing O_DIRECT (2021-11-16 11:30:29 +0100)

Block patches for 6.2.0-rc1:

- Fixes to image streaming job and block layer reconfiguration to make

iotest 030 pass again

- docs: Deprecate incorrectly typed device_add arguments

- file-posix: Fix alignment after reopen changing O_DIRECT

softmmu/qdev-monitor: fix use-after-free in qdev_set_id()

docs/about/deprecated.rst | 14 +++

include/qemu/transactions.h | 3 +

block.c | 233 +++++++++++++++++++++++++++---------

block/file-posix.c | 20 +++-

block/stream.c | 7 +-

softmmu/qdev-monitor.c | 2 +-

util/transactions.c | 8 +-

tests/qemu-iotests/030 | 11 +-

tests/qemu-iotests/142 | 29 +++++

tests/qemu-iotests/142.out | 18 +++

10 files changed, 279 insertions(+), 66 deletions(-)

2.33.1

As of a future patch, bdrv_replace_child_tran() will take a BdrvChild **

pointer. Prepare for that by getting such a pointer and using it where

applicable, and (dereferenced) as a parameter for

bdrv_replace_child_tran().

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Message-Id: <20211111120829.81329-7-hreitz@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Message-Id: <20211115145409.176785-7-kwolf@redhat.com>

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

---

block.c | 21 ++++++++++++---------

1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,

BdrvChild *child,

Transaction *tran)

{

+ BdrvChild **childp;

BdrvRemoveFilterOrCowChild *s;

- assert(child == bs->backing || child == bs->file);

-

if (!child) {

return;

+ if (child == bs->backing) {

+ childp = &bs->backing;

+ } else if (child == bs->file) {

+ childp = &bs->file;

+ } else {

+ g_assert_not_reached();

if (child->bs) {

- bdrv_replace_child_tran(child, NULL, tran);

+ bdrv_replace_child_tran(*childp, NULL, tran);

}

s = g_new(BdrvRemoveFilterOrCowChild, 1);

*s = (BdrvRemoveFilterOrCowChild) {

.child = child,

- .is_backing = (child == bs->backing),

+ .is_backing = (childp == &bs->backing),

};

tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);

- if (s->is_backing) {

- bs->backing = NULL;

- } else {

- bs->file = NULL;

+ *childp = NULL;

2.33.1

As of a future commit, bdrv_replace_child_noperm() will clear the

indirect BdrvChild pointer passed to it if the new child BDS is NULL.

bdrv_replace_child_tran() will want to let it do that, but revert this

change in its abort handler. For that, we need to have it receive a

BdrvChild ** pointer, too, and keep it stored in the

BdrvReplaceChildState object that we attach to the transaction.

Note that we do not need to store it in the BdrvReplaceChildState when

new_bs is not NULL, because then there is nothing to revert. This is

important so that bdrv_replace_node_noperm() can pass a pointer to a

loop-local variable to bdrv_replace_child_tran() without worrying that

this pointer will outlive one loop iteration.

(Of course, for that to work, bdrv_replace_node_noperm() and in turn

bdrv_replace_node() and its relatives may not be called with a NULL @to

node. Luckily, they already are not, but now we should assert this.)

bdrv_remove_file_or_backing_child() on the other hand needs to ensure

that the indirect pointer it passes will stay valid for the duration of

the transaction. Ensure this by keeping a strong reference to the BDS

whose &bs->backing or &bs->file it passes to bdrv_replace_child_tran(),

and giving up that reference only in the transaction .clean() handler.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Message-Id: <20211111120829.81329-9-hreitz@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Message-Id: <20211115145409.176785-9-kwolf@redhat.com>

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

---

block.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++-------

1 file changed, 73 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c

index XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ static int bdrv_drv_set_perm(BlockDriverState *bs, uint64_t perm,

typedef struct BdrvReplaceChildState {

BdrvChild *child;

+ BdrvChild **childp;

BlockDriverState *old_bs;

} BdrvReplaceChildState;

@@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_abort(void *opaque)

BdrvReplaceChildState *s = opaque;

BlockDriverState *new_bs = s->child->bs;

- /* old_bs reference is transparently moved from @s to @s->child */

+ /*

+ * old_bs reference is transparently moved from @s to s->child.

+ *

+ * Pass &s->child here instead of s->childp, because:

+ * (1) s->old_bs must be non-NULL, so bdrv_replace_child_noperm() will not

+ * modify the BdrvChild * pointer we indirectly pass to it, i.e. it

+ * will not modify s->child. From that perspective, it does not matter

+ * whether we pass s->childp or &s->child.

+ * (TODO: Right now, bdrv_replace_child_noperm() never modifies that

+ * pointer anyway (though it will in the future), so at this point it

+ * absolutely does not matter whether we pass s->childp or &s->child.)

+ * (2) If new_bs is not NULL, s->childp will be NULL. We then cannot use

+ * it here.

+ * (3) If new_bs is NULL, *s->childp will have been NULLed by

+ * bdrv_replace_child_tran()'s bdrv_replace_child_noperm() call, and we

+ * must not pass a NULL *s->childp here.

+ * (TODO: In its current state, bdrv_replace_child_noperm() will not

+ * have NULLed *s->childp, so this does not apply yet. It will in the

+ * future.)

+ *

+ * So whether new_bs was NULL or not, we cannot pass s->childp here; and in

+ * any case, there is no reason to pass it anyway.

+ */

bdrv_replace_child_noperm(&s->child, s->old_bs);

bdrv_unref(new_bs);

}

@@ -XXX,XX +XXX,XX @@ static TransactionActionDrv bdrv_replace_child_drv = {

* Note: real unref of old_bs is done only on commit.

*

* The function doesn't update permissions, caller is responsible for this.

+ *

+ * Note that if new_bs == NULL, @childp is stored in a state object attached

+ * to @tran, so that the old child can be reinstated in the abort handler.

+ * Therefore, if @new_bs can be NULL, @childp must stay valid until the

+ * transaction is committed or aborted.

+ *

+ * (TODO: The reinstating does not happen yet, but it will once

+ * bdrv_replace_child_noperm() NULLs *childp when new_bs is NULL.)

*/

-static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs,

+static void bdrv_replace_child_tran(BdrvChild **childp,

+ BlockDriverState *new_bs,

Transaction *tran)

{

BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1);

*s = (BdrvReplaceChildState) {

- .child = child,

- .old_bs = child->bs,

+ .child = *childp,

+ .childp = new_bs == NULL ? childp : NULL,

+ .old_bs = (*childp)->bs,

};

tran_add(tran, &bdrv_replace_child_drv, s);

if (new_bs) {

bdrv_ref(new_bs);

}

- bdrv_replace_child_noperm(&child, new_bs);

- /* old_bs reference is transparently moved from @child to @s */

+ bdrv_replace_child_noperm(childp, new_bs);

+ /* old_bs reference is transparently moved from *childp to @s */

}

/*

@@ -XXX,XX +XXX,XX @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)

typedef struct BdrvRemoveFilterOrCowChild {

BdrvChild *child;

+ BlockDriverState *bs;

bool is_backing;

} BdrvRemoveFilterOrCowChild;

@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_filter_or_cow_child_commit(void *opaque)

bdrv_child_free(s->child);

}

+static void bdrv_remove_filter_or_cow_child_clean(void *opaque)

+{

+ BdrvRemoveFilterOrCowChild *s = opaque;

+

+ /* Drop the bs reference after the transaction is done */

+ bdrv_unref(s->bs);

+ g_free(s);

+}

+

static TransactionActionDrv bdrv_remove_filter_or_cow_child_drv = {

.abort = bdrv_remove_filter_or_cow_child_abort,

.commit = bdrv_remove_filter_or_cow_child_commit,

- .clean = g_free,

+ .clean = bdrv_remove_filter_or_cow_child_clean,

};

/*

@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,

return;

}

+ /*

+ * Keep a reference to @bs so @childp will stay valid throughout the

+ * transaction (required by bdrv_replace_child_tran())

+ */

+ bdrv_ref(bs);

if (child == bs->backing) {

childp = &bs->backing;

} else if (child == bs->file) {

@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,

}

if (child->bs) {

- bdrv_replace_child_tran(*childp, NULL, tran);

+ bdrv_replace_child_tran(childp, NULL, tran);

}

s = g_new(BdrvRemoveFilterOrCowChild, 1);

*s = (BdrvRemoveFilterOrCowChild) {

.child = child,

+ .bs = bs,

.is_backing = (childp == &bs->backing),

};

tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);

@@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_noperm(BlockDriverState *from,

{

BdrvChild *c, *next;

+ assert(to != NULL);

+

QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {

assert(c->bs == from);

if (!should_update_child(c, to)) {

@@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_noperm(BlockDriverState *from,

c->name, from->node_name);

return -EPERM;

}

- bdrv_replace_child_tran(c, to, tran);

+

+ /*

+ * Passing a pointer to the local variable @c is fine here, because

+ * @to is not NULL, and so &c will not be attached to the transaction.

+ */

+ bdrv_replace_child_tran(&c, to, tran);

}

return 0;

@@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_noperm(BlockDriverState *from,

*

* With @detach_subchain=true @to must be in a backing chain of @from. In this

* case backing link of the cow-parent of @to is removed.

+ *

+ * @to must not be NULL.

*/

static int bdrv_replace_node_common(BlockDriverState *from,

BlockDriverState *to,

@@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_common(BlockDriverState *from,

BlockDriverState *to_cow_parent = NULL;

int ret;

+ assert(to != NULL);

+

if (detach_subchain) {

assert(bdrv_chain_contains(from, to));

assert(from != to);

@@ -XXX,XX +XXX,XX @@ out:

return ret;

}

+/**

+ * Replace node @from by @to (where neither may be NULL).

+ */

int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,

Error **errp)

{

@@ -XXX,XX +XXX,XX @@ int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,

bdrv_drained_begin(old_bs);

bdrv_drained_begin(new_bs);

- bdrv_replace_child_tran(child, new_bs, tran);

+ bdrv_replace_child_tran(&child, new_bs, tran);

found = g_hash_table_new(NULL, NULL);

refresh_list = bdrv_topological_dfs(refresh_list, found, old_bs);

--

2.33.1

In most of the block layer, especially when traversing down from other

BlockDriverStates, we assume that BdrvChild.bs can never be NULL. When

it becomes NULL, it is expected that the corresponding BdrvChild pointer

also becomes NULL and the BdrvChild object is freed.

Therefore, once bdrv_replace_child_noperm() sets the BdrvChild.bs

pointer to NULL, it should also immediately set the corresponding

BdrvChild pointer (like bs->file or bs->backing) to NULL.

In that context, it also makes sense for this function to free the

child. Sometimes we cannot do so, though, because it is called in a

transactional context where the caller might still want to reinstate the

child in the abort branch (and free it only on commit), so this behavior

has to remain optional.

In bdrv_replace_child_tran()'s abort handler, we now rely on the fact

that the BdrvChild passed to bdrv_replace_child_tran() must have had a

non-NULL .bs pointer initially. Make a note of that and assert it.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Message-Id: <20211111120829.81329-10-hreitz@redhat.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Message-Id: <20211115145409.176785-10-kwolf@redhat.com>

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

---

block.c | 102 +++++++++++++++++++++++++++++++++++++++++++-------------

1 file changed, 79 insertions(+), 23 deletions(-)

diff --git a/block.c b/block.c

index XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ static BlockDriverState *bdrv_open_inherit(const char *filename,

static bool bdrv_recurse_has_child(BlockDriverState *bs,

BlockDriverState *child);

+static void bdrv_child_free(BdrvChild *child);

static void bdrv_replace_child_noperm(BdrvChild **child,

- BlockDriverState *new_bs);

+ BlockDriverState *new_bs,

+ bool free_empty_child);

static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,

BdrvChild *child,

Transaction *tran);

@@ -XXX,XX +XXX,XX @@ typedef struct BdrvReplaceChildState {

BdrvChild *child;

BdrvChild **childp;

BlockDriverState *old_bs;

+ bool free_empty_child;

} BdrvReplaceChildState;

static void bdrv_replace_child_commit(void *opaque)

{

BdrvReplaceChildState *s = opaque;

+ if (s->free_empty_child && !s->child->bs) {

+ bdrv_child_free(s->child);

+ }

bdrv_unref(s->old_bs);

}

@@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_abort(void *opaque)

* modify the BdrvChild * pointer we indirectly pass to it, i.e. it

* will not modify s->child. From that perspective, it does not matter

* whether we pass s->childp or &s->child.

- * (TODO: Right now, bdrv_replace_child_noperm() never modifies that

- * pointer anyway (though it will in the future), so at this point it

- * absolutely does not matter whether we pass s->childp or &s->child.)

* (2) If new_bs is not NULL, s->childp will be NULL. We then cannot use

* it here.

* (3) If new_bs is NULL, *s->childp will have been NULLed by

* bdrv_replace_child_tran()'s bdrv_replace_child_noperm() call, and we

* must not pass a NULL *s->childp here.

- * (TODO: In its current state, bdrv_replace_child_noperm() will not

- * have NULLed *s->childp, so this does not apply yet. It will in the

- * future.)

*

* So whether new_bs was NULL or not, we cannot pass s->childp here; and in

* any case, there is no reason to pass it anyway.

*/

- bdrv_replace_child_noperm(&s->child, s->old_bs);

+ bdrv_replace_child_noperm(&s->child, s->old_bs, true);

+ /*

+ * The child was pre-existing, so s->old_bs must be non-NULL, and

+ * s->child thus must not have been freed

+ */

+ assert(s->child != NULL);

+ if (!new_bs) {

+ /* As described above, *s->childp was cleared, so restore it */

+ assert(s->childp != NULL);

+ *s->childp = s->child;

+ }

bdrv_unref(new_bs);

}

@@ -XXX,XX +XXX,XX @@ static TransactionActionDrv bdrv_replace_child_drv = {

*

* The function doesn't update permissions, caller is responsible for this.

*

+ * (*childp)->bs must not be NULL.

+ *

* Note that if new_bs == NULL, @childp is stored in a state object attached

* to @tran, so that the old child can be reinstated in the abort handler.

* Therefore, if @new_bs can be NULL, @childp must stay valid until the

* transaction is committed or aborted.

*

- * (TODO: The reinstating does not happen yet, but it will once

- * bdrv_replace_child_noperm() NULLs *childp when new_bs is NULL.)

+ * If @free_empty_child is true and @new_bs is NULL, the BdrvChild is

+ * freed (on commit). @free_empty_child should only be false if the

+ * caller will free the BDrvChild themselves (which may be important

+ * if this is in turn called in another transactional context).

*/

static void bdrv_replace_child_tran(BdrvChild **childp,

BlockDriverState *new_bs,

- Transaction *tran)

+ Transaction *tran,

+ bool free_empty_child)

{

BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1);

*s = (BdrvReplaceChildState) {

.child = *childp,

.childp = new_bs == NULL ? childp : NULL,

.old_bs = (*childp)->bs,

+ .free_empty_child = free_empty_child,

};

tran_add(tran, &bdrv_replace_child_drv, s);

+ /* The abort handler relies on this */

+ assert(s->old_bs != NULL);

+

if (new_bs) {

bdrv_ref(new_bs);

}

- bdrv_replace_child_noperm(childp, new_bs);

+ /*

+ * Pass free_empty_child=false, we will free the child (if

+ * necessary) in bdrv_replace_child_commit() (if our

+ * @free_empty_child parameter was true).

+ */

+ bdrv_replace_child_noperm(childp, new_bs, false);

/* old_bs reference is transparently moved from *childp to @s */

}

@@ -XXX,XX +XXX,XX @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm)

return permissions[qapi_perm];

}

+/**

+ * Replace (*childp)->bs by @new_bs.

+ *

+ * If @new_bs is NULL, *childp will be set to NULL, too: BDS parents

+ * generally cannot handle a BdrvChild with .bs == NULL, so clearing

+ * BdrvChild.bs should generally immediately be followed by the

+ * BdrvChild pointer being cleared as well.

+ *

+ * If @free_empty_child is true and @new_bs is NULL, the BdrvChild is

+ * freed. @free_empty_child should only be false if the caller will

+ * free the BdrvChild themselves (this may be important in a

+ * transactional context, where it may only be freed on commit).

+ */

static void bdrv_replace_child_noperm(BdrvChild **childp,

- BlockDriverState *new_bs)

+ BlockDriverState *new_bs,

+ bool free_empty_child)

{

BdrvChild *child = *childp;

BlockDriverState *old_bs = child->bs;

@@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_noperm(BdrvChild **childp,

}

child->bs = new_bs;

+ if (!new_bs) {

+ *childp = NULL;

+ }

if (new_bs) {

QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);

@@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_noperm(BdrvChild **childp,

bdrv_parent_drained_end_single(child);

drain_saldo++;

}

+

+ if (free_empty_child && !child->bs) {

+ bdrv_child_free(child);

+ }

}

/**

@@ -XXX,XX +XXX,XX @@ static void bdrv_attach_child_common_abort(void *opaque)

BdrvChild *child = *s->child;

BlockDriverState *bs = child->bs;

- bdrv_replace_child_noperm(s->child, NULL);

+ /*

+ * Pass free_empty_child=false, because we still need the child

+ * for the AioContext operations on the parent below; those

+ * BdrvChildClass methods all work on a BdrvChild object, so we

+ * need to keep it as an empty shell (after this function, it will

+ * not be attached to any parent, and it will not have a .bs).

+ */

+ bdrv_replace_child_noperm(s->child, NULL, false);

if (bdrv_get_aio_context(bs) != s->old_child_ctx) {

bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort);

@@ -XXX,XX +XXX,XX @@ static void bdrv_attach_child_common_abort(void *opaque)

bdrv_unref(bs);

bdrv_child_free(child);

- *s->child = NULL;

}

static TransactionActionDrv bdrv_attach_child_common_drv = {

@@ -XXX,XX +XXX,XX @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,

}

bdrv_ref(child_bs);

- bdrv_replace_child_noperm(&new_child, child_bs);

+ bdrv_replace_child_noperm(&new_child, child_bs, true);

+ /* child_bs was non-NULL, so new_child must not have been freed */

+ assert(new_child != NULL);

*child = new_child;

@@ -XXX,XX +XXX,XX @@ static void bdrv_detach_child(BdrvChild **childp)

{

BlockDriverState *old_bs = (*childp)->bs;

- bdrv_replace_child_noperm(childp, NULL);

- bdrv_child_free(*childp);

+ bdrv_replace_child_noperm(childp, NULL, true);

if (old_bs) {

/*

@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,

}

if (child->bs) {

- bdrv_replace_child_tran(childp, NULL, tran);

+ /*

+ * Pass free_empty_child=false, we will free the child in

+ * bdrv_remove_filter_or_cow_child_commit()

+ */

+ bdrv_replace_child_tran(childp, NULL, tran, false);

}

s = g_new(BdrvRemoveFilterOrCowChild, 1);

@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,

.is_backing = (childp == &bs->backing),

};

tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);

-

- *childp = NULL;

}

/*

@@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_noperm(BlockDriverState *from,

* Passing a pointer to the local variable @c is fine here, because

* @to is not NULL, and so &c will not be attached to the transaction.

*/

- bdrv_replace_child_tran(&c, to, tran);

+ bdrv_replace_child_tran(&c, to, tran, true);

}

return 0;

@@ -XXX,XX +XXX,XX @@ int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,

bdrv_drained_begin(old_bs);

bdrv_drained_begin(new_bs);

- bdrv_replace_child_tran(&child, new_bs, tran);

+ bdrv_replace_child_tran(&child, new_bs, tran, true);

+ /* @new_bs must have been non-NULL, so @child must not have been freed */

+ assert(child != NULL);

found = g_hash_table_new(NULL, NULL);

refresh_list = bdrv_topological_dfs(refresh_list, found, old_bs);

--

2.33.1