[ovs-dev] bug in sflow_choose_agent_address()
Justin Pettit
jpettit at ovn.org
Wed Sep 19 19:10:21 UTC 2018
Thanks for the report. The existing code seems to use "ip" in a way that's inconsistent, which I think makes it confusing. I think the approach is good, but I wonder about calling that variable "target", since don't we want the source address, not the target address?
How about the diff at the bottom of this message?
--Justin
-=-=-=-=-=-=-=-=-=-
diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
index d17d7a8be83c..62a09b5d1b1e 100644
--- a/ofproto/ofproto-dpif-sflow.c
+++ b/ofproto/ofproto-dpif-sflow.c
@@ -472,11 +472,12 @@ sflow_choose_agent_address(const char *agent_device,
/* sFlow only supports target in default routing table with
* packet mark zero.
*/
- ip = ss_get_address(&ss);
+ struct in6_addr target_ip = ss_get_address(&ss);
struct in6_addr gw, src = in6addr_any;
char name[IFNAMSIZ];
- if (ovs_router_lookup(0, &ip, name, &src, &gw)) {
+ if (ovs_router_lookup(0, &target_ip, name, &src, &gw)) {
+ ip = src;
goto success;
}
}
> On Sep 19, 2018, at 10:16 AM, Neil McKee <neil.mckee at inmon.com> wrote:
>
> It looks like the intent here was to get the local source-address
> associated with the route to the sFlow target-address:
>
> https://github.com/openvswitch/ovs/blob/master/ofproto/ofproto-dpif-sflow.c#L475-L481
>
> That's a good idea, but the answer supplied by ovs_router_lookup() via
> &src is ignored, so this code just sets the sFlow agent-address to
> the sFlow target-address... with a high potential for turmoil :)
>
> The suggested fix would be to introduce a new local var "struct
> in6_addr target", offer &target to ovs_router_lookup(), and if the
> lookup works then set ip = target before goto success. That way you
> are never setting "ip" to a toxic value.
>
> Sorry didn't notice this at the time.
>
> The workaround is just to make sure that the agent-address is set in
> the config and not left to automatic selection, which is probably
> what most deployments do anyway. (e.g. running hsflowd with the ovs{}
> module enabled will take care of it).
>
> Neil
>
> ------
> Neil McKee
> InMon Corp.
> http://www.inmon.com
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list