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.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
