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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Peter Xu (peterx@redhat.com) wrote:
> 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<---

Ah OK, yes I guess that's needed.

> 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..

It could at least do with a comment on test_migrate_start?

Dave

> 
> -- 
> Peter Xu
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK