block: fix bdrv_check_perm for non-tree subgraph

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>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
This commit is contained in:
Vladimir Sementsov-Ogievskiy 2019-02-23 22:20:40 +03:00 committed by Kevin Wolf
parent 2f30b7c377
commit f962e96150
2 changed files with 31 additions and 1 deletions

25
block.c
View File

@ -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);
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);
}

View File

@ -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;
};