[ovs-dev] [PATCH v2 3/5] tnl-neigh-cache: Unwildcard flow members before inspecting them.

[Daniele Di Proietto]

Thanks,

I applied this and the previous patch to master, branch-2.6 and branch-2.5

I'll rebase and send the rest of the series (which is mostly for master) in the next few days.

Thanks,

Daniele




On 19/09/2016 17:14, "Jarno Rajahalme" <jarno at ovn.org> wrote:

>Acked-by: Jarno Rajahalme <jarno at ovn.org>
>
>IMO this and the earlier (2/5) one are bug fixes that should be cherry-picked to branches 2.6 and 2.5, if applicable.
>
>  Jarno
>
>> On Aug 30, 2016, at 6:47 PM, Daniele Di Proietto <diproiettod at vmware.com> wrote:
>> 
>> tnl_neigh_snoop() is part of the translation.  During translation we
>> have to unwildcard all the fields we examine to make a decision.
>> 
>> tnl_arp_snoop() and tnl_nd_snoop() failed to unwildcard fileds in case
>> of failure.  The solution is to do unwildcarding before the field is
>> inspected.
>> 
>> Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
>> ---
>> lib/flow.h            |  7 +++++++
>> lib/tnl-neigh-cache.c | 22 ++++++++--------------
>> 2 files changed, 15 insertions(+), 14 deletions(-)
>> 
>> diff --git a/lib/flow.h b/lib/flow.h
>> index 5b83695..ea24e28 100644
>> --- a/lib/flow.h
>> +++ b/lib/flow.h
>> @@ -854,6 +854,13 @@ pkt_metadata_from_flow(struct pkt_metadata *md, const struct flow *flow)
>>     md->ct_label = flow->ct_label;
>> }
>> 
>> +/* Often, during translation we need to read a value from a flow('FLOW') and
>> + * unwildcard the corresponding bits in the wildcards('WC').  This macro makes
>> + * it easier to do that. */
>> +
>> +#define FLOW_WC_GET_AND_MASK_WC(FLOW, WC, FIELD) \
>> +    (((WC) ? WC_MASK_FIELD(WC, FIELD) : NULL), ((FLOW)->FIELD))
>> +
>> static inline bool is_vlan(const struct flow *flow,
>>                            struct flow_wildcards *wc)
>> {
>> diff --git a/lib/tnl-neigh-cache.c b/lib/tnl-neigh-cache.c
>> index f7d29f6..499efff 100644
>> --- a/lib/tnl-neigh-cache.c
>> +++ b/lib/tnl-neigh-cache.c
>> @@ -115,7 +115,7 @@ tnl_neigh_delete(struct tnl_neigh_entry *neigh)
>> 
>> static void
>> tnl_neigh_set__(const char name[IFNAMSIZ], const struct in6_addr *dst,
>> -              const struct eth_addr mac)
>> +                const struct eth_addr mac)
>> {
>>     ovs_mutex_lock(&mutex);
>>     struct tnl_neigh_entry *neigh = tnl_neigh_lookup__(name, dst);
>> @@ -151,26 +151,21 @@ static int
>> tnl_arp_snoop(const struct flow *flow, struct flow_wildcards *wc,
>>               const char name[IFNAMSIZ])
>> {
>> -    if (flow->dl_type != htons(ETH_TYPE_ARP) ||
>> -        flow->nw_proto != ARP_OP_REPLY ||
>> -        eth_addr_is_zero(flow->arp_sha)) {
>> +    if (flow->dl_type != htons(ETH_TYPE_ARP)
>> +        || FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_proto) != ARP_OP_REPLY
>> +        || eth_addr_is_zero(FLOW_WC_GET_AND_MASK_WC(flow, wc, arp_sha))) {
>>         return EINVAL;
>>     }
>> 
>> -    /* Exact Match on all ARP flows. */
>> -    memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
>> -    memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src);
>> -    memset(&wc->masks.arp_sha, 0xff, sizeof wc->masks.arp_sha);
>> -
>> -    tnl_arp_set(name, flow->nw_src, flow->arp_sha);
>> +    tnl_arp_set(name, FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src), flow->arp_sha);
>>     return 0;
>> }
>> 
>> static int
>> tnl_nd_snoop(const struct flow *flow, struct flow_wildcards *wc,
>> -              const char name[IFNAMSIZ])
>> +             const char name[IFNAMSIZ])
>> {
>> -    if (!is_nd(flow, NULL) || flow->tp_src != htons(ND_NEIGHBOR_ADVERT)) {
>> +    if (!is_nd(flow, wc) || flow->tp_src != htons(ND_NEIGHBOR_ADVERT)) {
>>         return EINVAL;
>>     }
>>     /* - RFC4861 says Neighbor Advertisements sent in response to unicast Neighbor
>> @@ -179,14 +174,13 @@ tnl_nd_snoop(const struct flow *flow, struct flow_wildcards *wc,
>>      *   TLL address and other Advertisements not including it can be ignored.
>>      * - OVS flow extract can set this field to zero in case of packet parsing errors.
>>      *   For details refer miniflow_extract()*/
>> -    if (eth_addr_is_zero(flow->arp_tha)) {
>> +    if (eth_addr_is_zero(FLOW_WC_GET_AND_MASK_WC(flow, wc, arp_tha))) {
>>         return EINVAL;
>>     }
>> 
>>     memset(&wc->masks.ipv6_src, 0xff, sizeof wc->masks.ipv6_src);
>>     memset(&wc->masks.ipv6_dst, 0xff, sizeof wc->masks.ipv6_dst);
>>     memset(&wc->masks.nd_target, 0xff, sizeof wc->masks.nd_target);
>> -    memset(&wc->masks.arp_tha, 0xff, sizeof wc->masks.arp_tha);
>> 
>>     tnl_neigh_set__(name, &flow->nd_target, flow->arp_tha);
>>     return 0;
>> -- 
>> 2.9.3
>> 
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>