On 5/13/20 8:44 AM, Dumitru Ceara wrote:
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
I pushed this to master.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
