[ovs-dev] bug in sflow_choose_agent_address()

[Neil McKee]

Yes.  Exactly.

Neil

------
Neil McKee
InMon Corp.
http://www.inmon.com


On Wed, Sep 19, 2018 at 12:10 PM, Justin Pettit <jpettit at ovn.org> wrote:
> 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
>