@@ -662,6 +662,11 @@ struct BdrvChild {
*/
uint64_t shared_perm;
+ /* backup of permissions during permission update procedure */
+ bool has_backup_perm;
+ uint64_t backup_perm;
+ uint64_t backup_shared_perm;
+
QLIST_ENTRY(BdrvChild) next;
QLIST_ENTRY(BdrvChild) next_parent;
};
@@ -1954,13 +1954,32 @@ static int bdrv_child_check_perm(BdrvChild *c, BlockReopenQueue *q,
ret = bdrv_check_update_perm(c->bs, q, perm, shared, ignore_children, errp);
g_slist_free(ignore_children);
- return ret;
+ if (ret < 0) {
+ return ret;
+ }
+
+ if (!c->has_backup_perm) {
+ c->has_backup_perm = true;
+ c->backup_perm = c->perm;
+ c->backup_shared_perm = c->shared_perm;
+ }
+ /*
+ * Note: it's OK if c->has_backup_perm was already set, as we can find the
+ * same child twice during check_perm procedure
+ */
+
+ c->perm = perm;
+ c->shared_perm = shared;
+
+ return 0;
}
static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared)
{
uint64_t cumulative_perms, cumulative_shared_perms;
+ c->has_backup_perm = false;
+
c->perm = perm;
c->shared_perm = shared;
@@ -1971,6 +1990,12 @@ static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared)
static void bdrv_child_abort_perm_update(BdrvChild *c)
{
+ if (c->has_backup_perm) {
+ c->perm = c->backup_perm;
+ c->shared_perm = c->backup_shared_perm;
+ c->has_backup_perm = false;
+ }
+
bdrv_abort_perm_update(c->bs);
}
bdrv_check_perm in it's recursion checks each node in context of new permissions for one parent, because of nature of DFS. It works well, while children subgraph of top-most updated node is a tree, i.e. it doesn't have any kind of loops. But if we have a loop (not oriented, of course), i.e. we have two different ways from top-node to some child-node, then bdrv_check_perm will do wrong thing: top | \ | | v v A B | | v v node It will once check new permissions of node in context of new A permissions and old B permissions and once visa-versa. It's a wrong way and may lead to corruption of permission system. We may start with no-permissions and all-shared for both A->node and B->node relations and finish up with non shared write permission for both ways. The following commit will add a test, which shows this bug. To fix this situation, let's really set BdrvChild permissions during bdrv_check_perm procedure. And we are happy here, as check-perm is already written in transaction manner, so we just need to restore backed-up permissions in _abort. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- Thing to discuss: in bdrv_child_abort_perm_update(), should we include "bdrv_abort_perm_update(c->bs);" into "if"? Or not? Anyway, if yes, it'd better be additional patch, as it may be other bug. Or, all things are prepared for abort even if check was not called for this particular child? include/block/block_int.h | 5 +++++ block.c | 27 ++++++++++++++++++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-)