Re: [libvirt] [PATCH v3 08/18] snapshot: Avoid latent use-after-free when cleaning snapshots
On 3/7/19 8:02 AM, John Ferlan wrote: > > > On 3/4/19 10:34 PM, Eric Blake wrote: >> Right now, the only callers of qemuDomainSnapshotDiscardAllMetadata() >> are right before freeing the virDomainSnapshotObjList, so it did not >> matter if the list's metaroot (which points to all the defined root >> snapshots) is left inconsistent. But an upcoming patch will want to >> clear all snapshots if a bulk redefine fails partway through, in >> which case things must be reset. Make this work by teaching the >> existing virDomainSnapshotUpdateRelations() to be safe regardless of >> the incoming state of the metaroot (since we don't want to leak that >> internal detail into qemu code), then fixing the qemu code to use >> it after deleting all snapshots. >> >> Signed-off-by: Eric Blake >> --- >> src/conf/snapshot_conf.c | 7 +-- >> src/qemu/qemu_domain.c | 4 >> 2 files changed, 9 insertions(+), 2 deletions(-) >> > > I have some random personal grumbling about the lack of comments for > existing code. What's described in this patch as being done would seem > correct, but my knowledge of snapshot's is 'cursory'. Yeah, and you already tasked me with refactoring virDomainSnapshotObj and virDomainCheckpointObj to share a common, better-commented, parent class. So hopefully there are patches in the next few days to improve the situation (or at least isolate it into one file instead of the current leaky abstraction where src/qemu is poking at .first_child contents in the first place). > >> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c >> index 206b05c172..386ec82d15 100644 >> --- a/src/conf/snapshot_conf.c >> +++ b/src/conf/snapshot_conf.c >> @@ -1209,13 +1209,16 @@ virDomainSnapshotSetRelations(void *payload, >> } >> >> /* Populate parent link and child count of all snapshots, with all >> - * relations starting as 0/NULL. Return 0 on success, -1 if a parent >> - * is missing or if a circular relationship was requested. */ >> + * assigned defs having relations starting as 0/NULL. Return 0 on >> + * success, -1 if a parent is missing or if a circular relationship >> + * was requested. */ > ^^ > nit: there's an extra space here Pre-existing, and habitual, but I'll clean up the ones you or I notice. >> +++ b/src/qemu/qemu_domain.c >> @@ -8625,6 +8625,10 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr >> driver, >> rem.err = 0; >> virDomainSnapshotForEach(vm->snapshots, qemuDomainSnapshotDiscardAll, >> ); >> +if (rem.current) >> +vm->current_snapshot = NULL; > > If rem.err != 0, is this still something that should be done Yes. rem.current is set only if we removed the current snapshot's metadata; even if we later failed halfway through before removing ALL snapshots, keeping the current_snapshot pointer around would be pointing into free'd memory. So the pointer must be cleared whether we succeed or fail to match that we did remove that metadata (and since we don't have any good way to undo a partial failure). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 08/18] snapshot: Avoid latent use-after-free when cleaning snapshots
On 3/4/19 10:34 PM, Eric Blake wrote:
> Right now, the only callers of qemuDomainSnapshotDiscardAllMetadata()
> are right before freeing the virDomainSnapshotObjList, so it did not
> matter if the list's metaroot (which points to all the defined root
> snapshots) is left inconsistent. But an upcoming patch will want to
> clear all snapshots if a bulk redefine fails partway through, in
> which case things must be reset. Make this work by teaching the
> existing virDomainSnapshotUpdateRelations() to be safe regardless of
> the incoming state of the metaroot (since we don't want to leak that
> internal detail into qemu code), then fixing the qemu code to use
> it after deleting all snapshots.
>
> Signed-off-by: Eric Blake
> ---
> src/conf/snapshot_conf.c | 7 +--
> src/qemu/qemu_domain.c | 4
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
I have some random personal grumbling about the lack of comments for
existing code. What's described in this patch as being done would seem
correct, but my knowledge of snapshot's is 'cursory'.
> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index 206b05c172..386ec82d15 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -1209,13 +1209,16 @@ virDomainSnapshotSetRelations(void *payload,
> }
>
> /* Populate parent link and child count of all snapshots, with all
> - * relations starting as 0/NULL. Return 0 on success, -1 if a parent
> - * is missing or if a circular relationship was requested. */
> + * assigned defs having relations starting as 0/NULL. Return 0 on
> + * success, -1 if a parent is missing or if a circular relationship
> + * was requested. */
^^
nit: there's an extra space here
> int
> virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots)
> {
> struct snapshot_set_relation act = { snapshots, 0 };
>
> +snapshots->metaroot.nchildren = 0;
> +snapshots->metaroot.first_child = NULL;
> virHashForEach(snapshots->objs, virDomainSnapshotSetRelations, );
> return act.err;
> }
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index db25e1596c..3ac79fa50b 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8625,6 +8625,10 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr
> driver,
> rem.err = 0;
> virDomainSnapshotForEach(vm->snapshots, qemuDomainSnapshotDiscardAll,
> );
> +if (rem.current)
> +vm->current_snapshot = NULL;
If rem.err != 0, is this still something that should be done
Reviewed-by: John Ferlan
John
> +if (virDomainSnapshotUpdateRelations(vm->snapshots) < 0 && !rem.err)
> +rem.err = -1;
>
> return rem.err;
> }
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 08/18] snapshot: Avoid latent use-after-free when cleaning snapshots
Right now, the only callers of qemuDomainSnapshotDiscardAllMetadata()
are right before freeing the virDomainSnapshotObjList, so it did not
matter if the list's metaroot (which points to all the defined root
snapshots) is left inconsistent. But an upcoming patch will want to
clear all snapshots if a bulk redefine fails partway through, in
which case things must be reset. Make this work by teaching the
existing virDomainSnapshotUpdateRelations() to be safe regardless of
the incoming state of the metaroot (since we don't want to leak that
internal detail into qemu code), then fixing the qemu code to use
it after deleting all snapshots.
Signed-off-by: Eric Blake
---
src/conf/snapshot_conf.c | 7 +--
src/qemu/qemu_domain.c | 4
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 206b05c172..386ec82d15 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -1209,13 +1209,16 @@ virDomainSnapshotSetRelations(void *payload,
}
/* Populate parent link and child count of all snapshots, with all
- * relations starting as 0/NULL. Return 0 on success, -1 if a parent
- * is missing or if a circular relationship was requested. */
+ * assigned defs having relations starting as 0/NULL. Return 0 on
+ * success, -1 if a parent is missing or if a circular relationship
+ * was requested. */
int
virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots)
{
struct snapshot_set_relation act = { snapshots, 0 };
+snapshots->metaroot.nchildren = 0;
+snapshots->metaroot.first_child = NULL;
virHashForEach(snapshots->objs, virDomainSnapshotSetRelations, );
return act.err;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index db25e1596c..3ac79fa50b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8625,6 +8625,10 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr
driver,
rem.err = 0;
virDomainSnapshotForEach(vm->snapshots, qemuDomainSnapshotDiscardAll,
);
+if (rem.current)
+vm->current_snapshot = NULL;
+if (virDomainSnapshotUpdateRelations(vm->snapshots) < 0 && !rem.err)
+rem.err = -1;
return rem.err;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
