On 5/13/20 2:09 PM, Ilya Maximets wrote:
> On 5/13/20 1:48 PM, Dumitru Ceara wrote:
>> On 5/13/20 1:17 PM, Ilya Maximets wrote:
>>> ovn-controller has uninitialized flow cookie because ofctrl_dup_flow()
>>> doesn't copy it from the original flow while duplicating. This could
>>> cause redundant flow updates or other issues.
>>>
>>> Valgrind reports:
>>>
>>> Conditional jump or move depends on uninitialised value(s)
>>> at 0x414B1C: ofctrl_put (ofctrl.c:1192)
>>> by 0x409ACB: main (ovn-controller.c:2086)
>>> Uninitialised value was created by a heap allocation
>>> at 0x483980B: malloc (vg_replace_malloc.c:309)
>>> by 0x4C8474: xmalloc (util.c:138)
>>> by 0x414553: ofctrl_dup_flow (ofctrl.c:793)
>>> by 0x4152CD: ofctrl_put (ofctrl.c:1231)
>>> by 0x409ACB: main (ovn-controller.c:2086)
>>>
>>> Fixes: d25e286ddb5c ("ovn-controller: Tie OpenFlow and logical flows using
>>> OpenFlow cookie.")
>>> Signed-off-by: Ilya Maximets <[email protected]>
>>> ---
>>> controller/ofctrl.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
>>> index 4b51cd86e..e5e78d9b6 100644
>>> --- a/controller/ofctrl.c
>>> +++ b/controller/ofctrl.c
>>> @@ -799,6 +799,7 @@ ofctrl_dup_flow(struct ovn_flow *src)
>>> dst->sb_uuid = src->sb_uuid;
>>> dst->match_hmap_node.hash = src->match_hmap_node.hash;
>>> dst->uuid_hindex_node.hash = uuid_hash(&src->sb_uuid);
>>> + dst->cookie = src->cookie;
>>> return dst;
>>> }
>>>
>>>
>>
>> Hi Ilya,
>>
>> This looks ok to me. However, I was thinking if it's worth making a more
>> "intrusive" change: instead of reinitializing each field explicitly in
>> ofctrl_dup_flow() we could call ovn_flow_alloc().
>>
>> This would need a bit of rework as we'd have to pass the flow_match_hash
>> as argument to ovn_flow_alloc() but has the advantage that if fields are
>> added to struct ovn_flow we only need to care about one place where we
>> initialize them.
>>
>> What do you think?
>
> Yeah, that could be good, but I don't see a clean way to do that right now.
> There are 2 issues:
>
> 1. alloc gets match as argument and converts to minimatch while dup copies
> existing minimatch. That is not easy to unify.
>
> 2. ovn_flow_match_hash() needs already allocated ovn_flow, so we'll have to
> modify it to get all the fields instead of ovn_flow to make it work.
>
> I'd suggest to have this fix as is and work on code refactoring as a separate
> change. This will also be much easier to backport without refactoring.
>
Sounds good to me. Also, I'm not sure how often the ovn_flow structure
will change in the future so this might be overkill.
Acked-by: Dumitru Ceara <[email protected]>
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
