1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/6] Complete cleanup of domain object usage
  4. View patch

[libvirt] [PATCH 5/6] libxl: Add refcnt for args->conn during migration

John Ferlan posted 6 patches 7 years, 9 months ago
[libvirt] [PATCH 5/6] libxl: Add refcnt for args->conn during migration
Posted by John Ferlan 7 years, 9 months ago 
Since the @dconn reference via args->conn will be used via a thread
or callback, let's make sure memory associated with it isn't free'd
unexpectedly before we use it. The Unref will be done when the object
is Dispose'd.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/libxl/libxl_migration.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 7fe352306c..d37a4a687a 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -239,6 +239,7 @@ libxlMigrationDstArgsDispose(void *obj)
 
     libxlMigrationCookieFree(args->migcookie);
     VIR_FREE(args->socks);
+    virObjectUnref(args->conn);
     virObjectUnref(args->vm);
 }
 
@@ -608,7 +609,7 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
     if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
         goto error;
 
-    args->conn = dconn;
+    args->conn = virObjectRef(dconn);
     args->vm = virObjectRef(vm);
     args->flags = flags;
     args->migcookie = mig;
@@ -763,7 +764,7 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
     if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
         goto error;
 
-    args->conn = dconn;
+    args->conn = virObjectRef(dconn);
     args->vm = virObjectRef(vm);
     args->flags = flags;
     args->socks = socks;
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] libxl: Add refcnt for args->conn during migration
Posted by Marc Hartmayer 7 years, 9 months ago 
On Tue, Apr 24, 2018 at 02:28 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
> Since the @dconn reference via args->conn will be used via a thread
> or callback, let's make sure memory associated with it isn't free'd
> unexpectedly before we use it. The Unref will be done when the object
> is Dispose'd.
>
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  src/libxl/libxl_migration.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
> index 7fe352306c..d37a4a687a 100644
> --- a/src/libxl/libxl_migration.c
> +++ b/src/libxl/libxl_migration.c
> @@ -239,6 +239,7 @@ libxlMigrationDstArgsDispose(void *obj)
>
>      libxlMigrationCookieFree(args->migcookie);
>      VIR_FREE(args->socks);
> +    virObjectUnref(args->conn);
>      virObjectUnref(args->vm);
>  }
>
> @@ -608,7 +609,7 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
>      if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
>          goto error;
>
> -    args->conn = dconn;
> +    args->conn = virObjectRef(dconn);
>      args->vm = virObjectRef(vm);
>      args->flags = flags;
>      args->migcookie = mig;
> @@ -763,7 +764,7 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
>      if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
>          goto error;
>
> -    args->conn = dconn;
> +    args->conn = virObjectRef(dconn);
>      args->vm = virObjectRef(vm);
>      args->flags = flags;
>      args->socks = socks;
> -- 
> 2.13.6
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>

Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>

-- 
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] libxl: Add refcnt for args->conn during migration
Posted by Erik Skultety 7 years, 9 months ago 
On Tue, Apr 24, 2018 at 08:28:08AM -0400, John Ferlan wrote:
> Since the @dconn reference via args->conn will be used via a thread
> or callback, let's make sure memory associated with it isn't free'd
> unexpectedly before we use it. The Unref will be done when the object
> is Dispose'd.
>
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  src/libxl/libxl_migration.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
> index 7fe352306c..d37a4a687a 100644
> --- a/src/libxl/libxl_migration.c
> +++ b/src/libxl/libxl_migration.c
> @@ -239,6 +239,7 @@ libxlMigrationDstArgsDispose(void *obj)
>
>      libxlMigrationCookieFree(args->migcookie);
>      VIR_FREE(args->socks);
> +    virObjectUnref(args->conn);
>      virObjectUnref(args->vm);
>  }
>
> @@ -608,7 +609,7 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
>      if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
>          goto error;
>
> -    args->conn = dconn;
> +    args->conn = virObjectRef(dconn);
>      args->vm = virObjectRef(vm);
>      args->flags = flags;
>      args->migcookie = mig;
> @@ -763,7 +764,7 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
>      if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
>          goto error;
>
> -    args->conn = dconn;
> +    args->conn = virObjectRef(dconn);
>      args->vm = virObjectRef(vm);
>      args->flags = flags;
>      args->socks = socks;

Do you actually have a use case, where the conn object would be destroyed
before the migration finishes in a way that this could cause a SIGSEGV?

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] libxl: Add refcnt for args->conn during migration
Posted by John Ferlan 7 years, 9 months ago 

On 05/03/2018 08:34 AM, Erik Skultety wrote:
> On Tue, Apr 24, 2018 at 08:28:08AM -0400, John Ferlan wrote:
>> Since the @dconn reference via args->conn will be used via a thread
>> or callback, let's make sure memory associated with it isn't free'd
>> unexpectedly before we use it. The Unref will be done when the object
>> is Dispose'd.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>>  src/libxl/libxl_migration.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
>> index 7fe352306c..d37a4a687a 100644
>> --- a/src/libxl/libxl_migration.c
>> +++ b/src/libxl/libxl_migration.c
>> @@ -239,6 +239,7 @@ libxlMigrationDstArgsDispose(void *obj)
>>
>>      libxlMigrationCookieFree(args->migcookie);
>>      VIR_FREE(args->socks);
>> +    virObjectUnref(args->conn);
>>      virObjectUnref(args->vm);
>>  }
>>
>> @@ -608,7 +609,7 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
>>      if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
>>          goto error;
>>
>> -    args->conn = dconn;
>> +    args->conn = virObjectRef(dconn);
>>      args->vm = virObjectRef(vm);
>>      args->flags = flags;
>>      args->migcookie = mig;
>> @@ -763,7 +764,7 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
>>      if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
>>          goto error;
>>
>> -    args->conn = dconn;
>> +    args->conn = virObjectRef(dconn);
>>      args->vm = virObjectRef(vm);
>>      args->flags = flags;
>>      args->socks = socks;
> 
> Do you actually have a use case, where the conn object would be destroyed
> before the migration finishes in a way that this could cause a SIGSEGV?
> 

Nope - just fear and protection any such possibility.  We're saving a
pointer to some object in an object and possibly dereferencing it at
some point in time in the future during libxlDoMigrateDstReceive.  Other
places where we do similar things w/ conn we also add a Ref to it.

And yes, patch 4 and 5 could be separate, perhaps more so this patch
than patch 4 because of how patch 6 changes @vm refcnt.

It shouldn't be possible to have a failure condition for either Ref, but
those are always "famous last words" and really difficult to diagnose
after the fact.  It doesn't hurt to Ref and then Unref AFAIK.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] libxl: Add refcnt for args->conn during migration
Posted by Erik Skultety 7 years, 9 months ago 
On Thu, May 03, 2018 at 09:02:35AM -0400, John Ferlan wrote:
>
>
> On 05/03/2018 08:34 AM, Erik Skultety wrote:
> > On Tue, Apr 24, 2018 at 08:28:08AM -0400, John Ferlan wrote:
> >> Since the @dconn reference via args->conn will be used via a thread
> >> or callback, let's make sure memory associated with it isn't free'd
> >> unexpectedly before we use it. The Unref will be done when the object
> >> is Dispose'd.
> >>
> >> Signed-off-by: John Ferlan <jferlan@redhat.com>
> >> ---
> >>  src/libxl/libxl_migration.c | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
> >> index 7fe352306c..d37a4a687a 100644
> >> --- a/src/libxl/libxl_migration.c
> >> +++ b/src/libxl/libxl_migration.c
> >> @@ -239,6 +239,7 @@ libxlMigrationDstArgsDispose(void *obj)
> >>
> >>      libxlMigrationCookieFree(args->migcookie);
> >>      VIR_FREE(args->socks);
> >> +    virObjectUnref(args->conn);
> >>      virObjectUnref(args->vm);
> >>  }
> >>
> >> @@ -608,7 +609,7 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
> >>      if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
> >>          goto error;
> >>
> >> -    args->conn = dconn;
> >> +    args->conn = virObjectRef(dconn);
> >>      args->vm = virObjectRef(vm);
> >>      args->flags = flags;
> >>      args->migcookie = mig;
> >> @@ -763,7 +764,7 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
> >>      if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
> >>          goto error;
> >>
> >> -    args->conn = dconn;
> >> +    args->conn = virObjectRef(dconn);
> >>      args->vm = virObjectRef(vm);
> >>      args->flags = flags;
> >>      args->socks = socks;
> >
> > Do you actually have a use case, where the conn object would be destroyed
> > before the migration finishes in a way that this could cause a SIGSEGV?
> >
>
> Nope - just fear and protection any such possibility.  We're saving a
> pointer to some object in an object and possibly dereferencing it at
> some point in time in the future during libxlDoMigrateDstReceive.  Other
> places where we do similar things w/ conn we also add a Ref to it.
>
> And yes, patch 4 and 5 could be separate, perhaps more so this patch
> than patch 4 because of how patch 6 changes @vm refcnt.

Doesn't have any effect on 4, whether you apply 4-5 before 6 or after,
it's an independent fix, but in the end it doesn't really matter, since you've
got my RB for both, so separating them now is irrelevant :).

>
> It shouldn't be possible to have a failure condition for either Ref, but
> those are always "famous last words" and really difficult to diagnose
> after the fact.  It doesn't hurt to Ref and then Unref AFAIK.

It doesn't, I was just curious, because I didn't see anything similar in qemu
migration code, but I wasn't really thorough, so it could have been done prior
to entering the migration code in qemu_driver.c, I'm not going to dig deeper,
it's a simple straightforward Ref/Unref change.

Reviewed-by: Erik Skultety <eskultet@redhat.com>

PS: I still think you could move 4,5 after 6, since to me the causality of
changes is more evident.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] libxl: Add refcnt for args->conn during migration
Posted by Erik Skultety 7 years, 9 months ago 
On Tue, Apr 24, 2018 at 08:28:08AM -0400, John Ferlan wrote:
> Since the @dconn reference via args->conn will be used via a thread
> or callback, let's make sure memory associated with it isn't free'd
> unexpectedly before we use it. The Unref will be done when the object
> is Dispose'd.
>
> Signed-off-by: John Ferlan <jferlan@redhat.com>

Also, yeah, as you figured and noted in the cover letter, these two libxl
patches would probably be better off in a separate series IMO.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.