Revert "nwfilter: Move save of config until after successful assign"

[libvirt] [PATCH] Revert "nwfilter: Move save of config until after successful assign"

Posted by John Ferlan 7 years, 10 months ago

This reverts commit b3e71a8830b2683ee88fa10cb048eabb99a446c0.

As it turns out this ends up very badly as the @def could be Free'd
even though it's owned by @obj as a result of the AssignDef.

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

diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
index b5aaa6b..b36eda1 100644
--- a/src/conf/virnwfilterobj.c
+++ b/src/conf/virnwfilterobj.c
@@ -501,14 +501,14 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters,
         goto error;
     }
 
-    if (!(obj = virNWFilterObjListAssignDef(nwfilters, def)))
-        goto error;
-
     /* We generated a UUID, make it permanent by saving the config to disk */
     if (!def->uuid_specified &&
         virNWFilterSaveConfig(configDir, def) < 0)
         goto error;
 
+    if (!(obj = virNWFilterObjListAssignDef(nwfilters, def)))
+        goto error;
+
     VIR_FREE(configFile);
     return obj;
 
-- 
2.9.4

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

Re: [libvirt] [PATCH] Revert "nwfilter: Move save of config until after successful assign"

Posted by Pavel Hrdina 7 years, 10 months ago

On Sat, Jul 15, 2017 at 02:40:06PM -0400, John Ferlan wrote:
> This reverts commit b3e71a8830b2683ee88fa10cb048eabb99a446c0.
> 
> As it turns out this ends up very badly as the @def could be Free'd
> even though it's owned by @obj as a result of the AssignDef.

I don't see a reason to revert it.  What do you mean that the @def can
be freed?  The virNWFilterObjListAssignDef() doesn't free the @def that
is passed to it, it only assign it to nwfilter object and returns it
immediately.

Pavel

> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  src/conf/virnwfilterobj.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
> index b5aaa6b..b36eda1 100644
> --- a/src/conf/virnwfilterobj.c
> +++ b/src/conf/virnwfilterobj.c
> @@ -501,14 +501,14 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters,
>          goto error;
>      }
>  
> -    if (!(obj = virNWFilterObjListAssignDef(nwfilters, def)))
> -        goto error;
> -
>      /* We generated a UUID, make it permanent by saving the config to disk */
>      if (!def->uuid_specified &&
>          virNWFilterSaveConfig(configDir, def) < 0)
>          goto error;
>  
> +    if (!(obj = virNWFilterObjListAssignDef(nwfilters, def)))
> +        goto error;
> +
>      VIR_FREE(configFile);
>      return obj;
>  
> -- 
> 2.9.4
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Revert "nwfilter: Move save of config until after successful assign"

Posted by John Ferlan 7 years, 10 months ago


On 07/17/2017 04:15 AM, Pavel Hrdina wrote:
> On Sat, Jul 15, 2017 at 02:40:06PM -0400, John Ferlan wrote:
>> This reverts commit b3e71a8830b2683ee88fa10cb048eabb99a446c0.
>>
>> As it turns out this ends up very badly as the @def could be Free'd
>> even though it's owned by @obj as a result of the AssignDef.
> 
> I don't see a reason to revert it.  What do you mean that the @def can
> be freed?  The virNWFilterObjListAssignDef() doesn't free the @def that
> is passed to it, it only assign it to nwfilter object and returns it
> immediately.
> 
> Pavel
> 

After ListAssignDef the @def is owned by @obj

If the SaveConfig fails, we jump to error: which will free @def. Now we
have an @obj in the nwfilters->objs list which has obj->def entry using
that address.

At some point in the future this will be undefined, which calls
virNWFilterObjFree from virNWFilterObjListRemove at which time we'll
free something that's already free'd. In between that future free,
something else could use obj->def thinking it hasn't been free'd. I
realized this as part of the review to the nwfilter patch series

John

>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>>  src/conf/virnwfilterobj.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
>> index b5aaa6b..b36eda1 100644
>> --- a/src/conf/virnwfilterobj.c
>> +++ b/src/conf/virnwfilterobj.c
>> @@ -501,14 +501,14 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters,
>>          goto error;
>>      }
>>  
>> -    if (!(obj = virNWFilterObjListAssignDef(nwfilters, def)))
>> -        goto error;
>> -
>>      /* We generated a UUID, make it permanent by saving the config to disk */
>>      if (!def->uuid_specified &&
>>          virNWFilterSaveConfig(configDir, def) < 0)
>>          goto error;
>>  
>> +    if (!(obj = virNWFilterObjListAssignDef(nwfilters, def)))
>> +        goto error;
>> +
>>      VIR_FREE(configFile);
>>      return obj;
>>  
>> -- 
>> 2.9.4
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list

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

Re: [libvirt] [PATCH] Revert "nwfilter: Move save of config until after successful assign"

Posted by Pavel Hrdina 7 years, 10 months ago

On Mon, Jul 17, 2017 at 07:39:28AM -0400, John Ferlan wrote:
> 
> 
> On 07/17/2017 04:15 AM, Pavel Hrdina wrote:
> > On Sat, Jul 15, 2017 at 02:40:06PM -0400, John Ferlan wrote:
> >> This reverts commit b3e71a8830b2683ee88fa10cb048eabb99a446c0.
> >>
> >> As it turns out this ends up very badly as the @def could be Free'd
> >> even though it's owned by @obj as a result of the AssignDef.
> > 
> > I don't see a reason to revert it.  What do you mean that the @def can
> > be freed?  The virNWFilterObjListAssignDef() doesn't free the @def that
> > is passed to it, it only assign it to nwfilter object and returns it
> > immediately.
> > 
> > Pavel
> > 
> 
> After ListAssignDef the @def is owned by @obj
> 
> If the SaveConfig fails, we jump to error: which will free @def. Now we
> have an @obj in the nwfilters->objs list which has obj->def entry using
> that address.

Oh, right, it might happen while reloading libvirtd daemon.

Sigh, I hate the nwfilter code, it's way to complex and ugly and has a
lot of workaround, like this UUID generation and saving it back to the
config file while loading the config file.

Anyway, we should fix it even though reloading of libvirtd probably
don't work.

Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Revert "nwfilter: Move save of config until after successful assign"

Posted by John Ferlan 7 years, 10 months ago

On 07/17/2017 08:44 AM, Pavel Hrdina wrote:
> On Mon, Jul 17, 2017 at 07:39:28AM -0400, John Ferlan wrote:
>>
>>
>> On 07/17/2017 04:15 AM, Pavel Hrdina wrote:
>>> On Sat, Jul 15, 2017 at 02:40:06PM -0400, John Ferlan wrote:
>>>> This reverts commit b3e71a8830b2683ee88fa10cb048eabb99a446c0.
>>>>
>>>> As it turns out this ends up very badly as the @def could be Free'd
>>>> even though it's owned by @obj as a result of the AssignDef.
>>>
>>> I don't see a reason to revert it.  What do you mean that the @def can
>>> be freed?  The virNWFilterObjListAssignDef() doesn't free the @def that
>>> is passed to it, it only assign it to nwfilter object and returns it
>>> immediately.
>>>
>>> Pavel
>>>
>>
>> After ListAssignDef the @def is owned by @obj
>>
>> If the SaveConfig fails, we jump to error: which will free @def. Now we
>> have an @obj in the nwfilters->objs list which has obj->def entry using
>> that address.
> 
> Oh, right, it might happen while reloading libvirtd daemon.
> 

Fortunately to a degree it's an error path on saving a file and only if
needing to generate the UUID - so the impact should be low. It's the
improbable, but possible case. If they were available, the patch should
be backported to 3.3-maint, 3.4-maint, and 3.5-maint trees. I've never
created one of those before though...

> Sigh, I hate the nwfilter code, it's way to complex and ugly and has a
> lot of workaround, like this UUID generation and saving it back to the
> config file while loading the config file.
> 

Yeah - it's really ugly and has quite a few "gotchas" that should be
handled better. The recursive locks are especially nightmarish. Once my
other series is complete, having hash table lookups instead of list
traversal and comparison should make a few things easier. Doubt anyone
wants to profess too much knowledge of the nwfilters in order to take on
the effort to clean things up.

Tks -

John

> Anyway, we should fix it even though reloading of libvirtd probably
> don't work.
> 
> Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
> 

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