[libvirt] [PATCH] virnwfilterbindingobj: Introduce and use virNWFilterBindingObjStealDef

[John Ferlan]

On 3/20/19 9:11 AM, Michal Privoznik wrote:
> On 3/20/19 12:17 PM, John Ferlan wrote:
>>
>>
>> On 3/20/19 6:31 AM, Michal Privoznik wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1686927
>>>
>>> When trying to create a nwfilter binding via
>>> nwfilterBindingCreateXML() we may encounter a crash. The sequence
>>> of functions called is as follows:
>>>
>>> 1) nwfilterBindingCreateXML() parses the XML and calls
>>> virNWFilterBindingObjListAdd() which calls
>>> virNWFilterBindingObjListAddLocked()
>>>
>>> 2) Here, @binding is not found because binding->remove is set.
>>>
>>> 3) Therefore, controls continue with creating new @binding,
>>> setting its def to the one from 1) and adding it to the hash
>>> table.
>>>
>>> 4) This fails, because the binding is still in the hash table
>>> (duplicate key is detected).
>>>
>>> 5) The control jumps to 'error' label where
>>> virNWFilterBindingObjEndAPI() is called which frees the binding
>>> definition passed.
>>>
>>> 6) Error is propagated to the caller, which calls
>>> virNWFilterBindingDefFree() over the definition again.
>>>
>>> The solution is to unset binding->def in case of failure so it's
>>> not freed in step 5).
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>> ---
>>>
>>> Technically, this is a v2 of:
>>>
>>> https://www.redhat.com/archives/libvir-list/2019-March/msg01209.html
>>>
>>> But since this one implements different approach than v1 I'm not marking
>>> it as such.
>>>
>>>   src/conf/virnwfilterbindingobj.c     | 10 ++++++++++
>>>   src/conf/virnwfilterbindingobj.h     |  3 +++
>>>   src/conf/virnwfilterbindingobjlist.c |  4 ++++
>>>   src/libvirt_private.syms             |  1 +
>>>   4 files changed, 18 insertions(+)
>>>
>>
>> Why wouldn't your other series take care of this in a more efficient
>> way?, e.g. patch 3/3:
>>
>> https://www.redhat.com/archives/libvir-list/2019-March/msg01321.html
>>
> 
> These are separate issues really. virHashAddEntry() can fail in more
> cases than just 'Duplicate key'. For instance, on OOM. The patches you
> reference help us prevent tickling this bug in case of 'Duplicate key'
> but not really for OOM.
> 

Ah... right... you could update the commit message above to indicate key
error or oom error (just for posterity)

>> Isn't the problem that we can get to the virNWFilterBindingObjSetDef and
>> virNWFilterBindingObjListAddObjLocked because of the same portdevname
>> key name which causes the virHashAddEntry to fail.
>>
>> If we fail after virNWFilterBindingObjListFindByPortDevLocked because
>> the other patch recognized that the @binding was in the process of being
>> removed, then we'd never create a new @obj with our new @def that had
>> the duplicated portdevname key.
> 
> Again, patch you refer to will only prevent from tickling this bug.
> 

Right - just trying to complete my thought.

>>
>>
>>> diff --git a/src/conf/virnwfilterbindingobj.c
>>> b/src/conf/virnwfilterbindingobj.c
>>> index 23978d4207..68afb9c434 100644
>>> --- a/src/conf/virnwfilterbindingobj.c
>>> +++ b/src/conf/virnwfilterbindingobj.c
>>> @@ -88,6 +88,16 @@
>>> virNWFilterBindingObjSetDef(virNWFilterBindingObjPtr obj,
>>>   }
>>>     +virNWFilterBindingDefPtr
>>> +virNWFilterBindingObjStealDef(virNWFilterBindingObjPtr obj)
>>> +{
>>> +    virNWFilterBindingDefPtr def;
>>> +
>>> +    VIR_STEAL_PTR(def, obj->def);
>>> +    return def;
>>> +}
>>> +
>>> +
>>>   bool
>>>   virNWFilterBindingObjGetRemoving(virNWFilterBindingObjPtr obj)
>>>   {
>>> diff --git a/src/conf/virnwfilterbindingobj.h
>>> b/src/conf/virnwfilterbindingobj.h
>>> index 8e5fbee35f..b26bb3c8ec 100644
>>> --- a/src/conf/virnwfilterbindingobj.h
>>> +++ b/src/conf/virnwfilterbindingobj.h
>>> @@ -39,6 +39,9 @@ void
>>>   virNWFilterBindingObjSetDef(virNWFilterBindingObjPtr obj,
>>>                               virNWFilterBindingDefPtr def);
>>>   +virNWFilterBindingDefPtr
>>> +virNWFilterBindingObjStealDef(virNWFilterBindingObjPtr obj);
>>> +
>>>   bool
>>>   virNWFilterBindingObjGetRemoving(virNWFilterBindingObjPtr obj);
>>>   diff --git a/src/conf/virnwfilterbindingobjlist.c
>>> b/src/conf/virnwfilterbindingobjlist.c
>>> index 06ccbf53af..4ee2c1b194 100644
>>> --- a/src/conf/virnwfilterbindingobjlist.c
>>> +++ b/src/conf/virnwfilterbindingobjlist.c
>>> @@ -167,6 +167,7 @@
>>> virNWFilterBindingObjListAddLocked(virNWFilterBindingObjListPtr
>>> bindings,
>>>                                      virNWFilterBindingDefPtr def)
>>>   {
>>>       virNWFilterBindingObjPtr binding;
>>> +    bool stealDef = false;
>>>         /* See if a binding with matching portdev already exists */
>>>       if ((binding = virNWFilterBindingObjListFindByPortDevLocked(
>>> @@ -181,6 +182,7 @@
>>> virNWFilterBindingObjListAddLocked(virNWFilterBindingObjListPtr
>>> bindings,
>>>           goto error;
>>>         virNWFilterBindingObjSetDef(binding, def);
>>> +    stealDef = true;
>>>         if (virNWFilterBindingObjListAddObjLocked(bindings, binding)
>>> < 0)
>>>           goto error;
>>> @@ -188,6 +190,8 @@
>>> virNWFilterBindingObjListAddLocked(virNWFilterBindingObjListPtr
>>> bindings,
>>>       return binding;
>>>      error:
>>> +    if (stealDef)
>>> +        virNWFilterBindingObjStealDef(binding);
>>
>> I think this is no different than modifying the failure path for
>> virNWFilterBindingObjListAddObjLocked to clear out @def acknowledging
>> our caller would free @def upon failure return.
>>
>>     if !AddObj() {
>>        ObjSet(binding, NULL)
>>        goto error
>>     }
> 
> As said in review for v1, virNWFilterBindingObjSetDef() calls
> virNWFilterBindingDefFree(binding->def) and only after that it sets
> binding->def to desired value. This approach won't help.
> 

Ah right - mind block. Trying to recall other places and the order of
when setting ->def occurs - for some reason I recall it always occurred
after the Hash insertion was successful, hence the subsequent comment of
changing order. That's more of a consistency type thing.

I'm fine with this approach and see Jano already R-By'd it. Didn't want
to leave you hanging with any unresolved questions/concerns.

John

>>
>> Alternatively, reorder the code to do the ObjSetDef after successful
>> return from virNWFilterBindingObjListAddLocked which would take
>> def->portdevname as a "const char *key" type parameter; however, there
>> could be some affect with virNWFilterBindingObjListLoadStatus
>> processing. In particular how virNWFilterBindingObjParseXML sets
>> obj->def...
> 
> That's an alternative approach. There are two (or more) ways to fix this
> bug. I've chosen one of them. What advantages does the other have that
> it should be done that way?
> 
>>
>> BTW: When I read the create a StealDef I was thinking more in the terms
>> of "if binding is in the process of being removed", then use the
>> StealDef to return the current @binding->def so it could be Free'd and
>> clear the removing bit. There may need to be some obj ref counting magic
>> to make sure some other thread in the process of deleting the object
>> doesn't do something bad or logic in that thread to recognize the @obj
>> is no longer being deleted. I didn't dig into the removing code and
>> there's not enough coffee in me yet to think more clearly about that.
> 
> In general, I don't think such logic is desired. Just look at my
> reproducer (1/3 from the linked series). One thread is doing 'destroy'
> the other is doing 'create'. There can't be any logic that would make
> both APIs happy, can it? One of the pair has to fail.
> 
> Michal