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?
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
