Re: [PATCH 20/20] tests: Add postcopy preempt test

[Peter Xu]

On Wed, Feb 23, 2022 at 03:50:24PM +0800, Peter Xu wrote:
> On Tue, Feb 22, 2022 at 12:51:59PM +0000, Dr. David Alan Gilbert wrote:
> > * Peter Xu (peterx@redhat.com) wrote:
> > > Two tests are added: a normal postcopy preempt test, and a recovery test.
> > 
> > Yes, this is difficult; without hugepages the tests are limited; did you
> > see if this test actually causes pages to go down the fast path?
> 
> I didn't observe the test case explicitly, but I did observe in my own test
> that I ran that it goes with the fast path, or I can't get a huge speed up.
> 
> Meanwhile my own test is only using 2M huge pages, and I can observe
> interruptions of huge page sendings frequently.
> 
> But yeah let me try to capture something in this test too, at least to make
> sure the standalone socket is being used.  Covering of huge pages might be
> doable but obviously requires host privileges, so I'll leave that for later.

When I tried to observe the test case today, I found that the preempt new
tests are all running with the new channels, however funnily I found the
original vanilla test is using it too!

Looked into it, that's because the MigrateStart* pointer is freed in
test_migrate_start() but the test referenced it after that... so it's a
use-after-free bug in the test code.  I need to squash this:

---8<---
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 5053b40589..09a9ce4401 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -664,6 +664,8 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
                                     MigrateStart *args)
 {
     g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
+    /* NOTE: args will be freed in test_migrate_start(), cache it */
+    bool postcopy_preempt = args->postcopy_preempt;
     QTestState *from, *to;
 
     if (test_migrate_start(&from, &to, uri, args)) {
@@ -674,7 +676,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
     migrate_set_capability(to, "postcopy-ram", true);
     migrate_set_capability(to, "postcopy-blocktime", true);
 
-    if (args->postcopy_preempt) {
+    if (postcopy_preempt) {
         migrate_set_capability(from, "postcopy-preempt", true);
         migrate_set_capability(to, "postcopy-preempt", true);
     }
---8<---

That's tricky, and we could have done something better.. E.g., we could
pass in the MigrateStart** into test_migrate_start() so it can clear it
when free, that's not silent use-after-free but crashing, which is better
in this case.

I feel lucky I tried..

-- 
Peter Xu