[ovs-dev] bug in sflow_choose_agent_address()

Justin Pettit jpettit at ovn.org
Wed Sep 19 20:34:12 UTC 2018


Great.  I sent out a formal patch:

	https://mail.openvswitch.org/pipermail/ovs-dev/2018-September/352268.html

If we get it reviewed quickly, we can get it into OVS 2.10.1, which I was planning to prep today.

--Justin


> On Sep 19, 2018, at 12:13 PM, Neil McKee <neil.mckee at inmon.com> wrote:
> 
> 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
>> 



More information about the dev mailing list