[ovs-dev,v4,4/4] datapath-windows: Compute ct hash based on 5-tuple and zone

Conntrack 5-tuple consists of src address, dst address, src port,
dst port and protocol which will be unique to a ct session.
Use this information along with zone to compute hash.

Also re-factor conntrack code related to parsing netlink attributes.

Signed-off-by: Anand Kumar <kumaranand@vmware.com>
---
 datapath-windows/ovsext/Conntrack.c | 228 ++++++++++++++++++------------------
 datapath-windows/ovsext/Conntrack.h |   2 -
 2 files changed, 116 insertions(+), 114 deletions(-)

On 06/17/2018 10:37 PM, Anand Kumar wrote:
> Conntrack 5-tuple consists of src address, dst address, src port,
> dst port and protocol which will be unique to a ct session.
> Use this information along with zone to compute hash.
>
> Also re-factor conntrack code related to parsing netlink attributes.

Mind adding a "Testing done:" section?

> Signed-off-by: Anand Kumar <kumaranand@vmware.com>
> ---
>   datapath-windows/ovsext/Conntrack.c | 228 ++++++++++++++++++------------------
>   datapath-windows/ovsext/Conntrack.h |   2 -
>   2 files changed, 116 insertions(+), 114 deletions(-)
>
> diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
> index 2a85e57..91bd638 100644
> --- a/datapath-windows/ovsext/Conntrack.c
> +++ b/datapath-windows/ovsext/Conntrack.c
> @@ -151,6 +151,24 @@ OvsCleanupConntrack(VOID)
>       OvsNatCleanup();
>   }
>   
> +/*
> + *----------------------------------------------------------------------------
> + * OvsCtHashKey
> + *     Compute hash using 5-tuple and zone.
> + *----------------------------------------------------------------------------
> + */
> +UINT32
> +OvsCtHashKey(const OVS_CT_KEY *key)
> +{
> +    UINT32 hsrc, hdst, hash;
> +    hsrc = key->src.addr.ipv4 | ntohl(key->src.port);
> +    hdst = key->dst.addr.ipv4 | ntohl(key->dst.port);
> +    hash = hsrc ^ hdst; /* TO identify reverse traffic */

Nit: TO -> To

> +    hash = hash | (key->zone + key->nw_proto);
> +    hash = OvsJhashWords((uint32_t*) &hash, 1, hash);
> +    return hash;
> +}
> +
>   static __inline VOID
>   OvsCtKeyReverse(OVS_CT_KEY *key)
>   {
> @@ -232,7 +250,7 @@ OvsCtAddEntry(POVS_CT_ENTRY entry,
>               if (!OvsNatTranslateCtEntry(entry)) {
>                   return FALSE;
>               }
> -            ctx->hash = OvsHashCtKey(&entry->key);
> +            ctx->hash = OvsCtHashKey(&entry->key);
>           } else {
>               entry->natInfo.natAction = natInfo->natAction;
>           }
> @@ -529,20 +547,6 @@ OvsCtLookup(OvsConntrackKeyLookupCtx *ctx)
>       return found;
>   }
>   
> -UINT32
> -OvsHashCtKey(const OVS_CT_KEY *key)
> -{
> -    UINT32 hsrc, hdst, hash;
> -    hsrc = OvsJhashBytes((UINT32*) &key->src, sizeof(key->src), 0);
> -    hdst = OvsJhashBytes((UINT32*) &key->dst, sizeof(key->dst), 0);
> -    hash = hsrc ^ hdst; /* TO identify reverse traffic */
> -    hash = OvsJhashBytes((uint32_t *) &key->dst + 1,
> -                         ((uint32_t *) (key + 1) -
> -                         (uint32_t *) (&key->dst + 1)),
> -                         hash);
> -    return hash;
> -}
> -
>   static UINT8
>   OvsReverseIcmpType(UINT8 type)
>   {
> @@ -640,7 +644,7 @@ OvsCtSetupLookupCtx(OvsFlowKey *flowKey,
>           OvsCtKeyReverse(&ctx->key);
>       }
>   
> -    ctx->hash = OvsHashCtKey(&ctx->key);
> +    ctx->hash = OvsCtHashKey(&ctx->key);
>       return NDIS_STATUS_SUCCESS;
>   }
>   
> @@ -952,7 +956,6 @@ OvsExecuteConntrackAction(OvsForwardingContext *fwdCtx,
>                             OvsFlowKey *key,
>                             const PNL_ATTR a)
>   {
> -    PNL_ATTR ctAttr;
>       BOOLEAN commit = FALSE;
>       BOOLEAN force = FALSE;
>       BOOLEAN postUpdateEvent = FALSE;
> @@ -972,109 +975,110 @@ OvsExecuteConntrackAction(OvsForwardingContext *fwdCtx,
>           return status;
>       }
>   
> -    /* XXX Convert this to NL_ATTR_FOR_EACH */
> -    ctAttr = NlAttrFindNested(a, OVS_CT_ATTR_ZONE);
> -    if (ctAttr) {
> -        zone = NlAttrGetU16(ctAttr);
> -    }
> -    ctAttr = NlAttrFindNested(a, OVS_CT_ATTR_COMMIT);
> -    if (ctAttr) {
> -        commit = TRUE;
> -    }
> -    ctAttr = NlAttrFindNested(a, OVS_CT_ATTR_MARK);
> -    if (ctAttr) {
> -        mark = NlAttrGet(ctAttr);
> -    }
> -    ctAttr = NlAttrFindNested(a, OVS_CT_ATTR_LABELS);
> -    if (ctAttr) {
> -        labels = NlAttrGet(ctAttr);
> -    }
> -    natActionInfo.natAction = NAT_ACTION_NONE;
> -    ctAttr = NlAttrFindNested(a, OVS_CT_ATTR_NAT);
> -    if (ctAttr) {
> -        /* Pares Nested NAT attributes. */
> -        PNL_ATTR natAttr;
> -        unsigned int left;
> -        BOOLEAN hasMinIp = FALSE;
> -        BOOLEAN hasMinPort = FALSE;
> -        BOOLEAN hasMaxIp = FALSE;
> -        BOOLEAN hasMaxPort = FALSE;
> -        NL_NESTED_FOR_EACH_UNSAFE (natAttr, left, ctAttr) {
> -            enum ovs_nat_attr subtype = NlAttrType(natAttr);
> -            switch(subtype) {
> -            case OVS_NAT_ATTR_SRC:
> -            case OVS_NAT_ATTR_DST:
> -                natActionInfo.natAction |=
> -                    ((subtype == OVS_NAT_ATTR_SRC)
> -                        ? NAT_ACTION_SRC : NAT_ACTION_DST);
> +    PNL_ATTR ctAttr = NULL;
> +    INT left;
> +
> +    NL_NESTED_FOR_EACH (ctAttr, left, a) {
> +        switch(NlAttrType(ctAttr)) {
> +            case OVS_CT_ATTR_ZONE:
> +                zone = NlAttrGetU16(ctAttr);
> +                break;
> +            case OVS_CT_ATTR_COMMIT:
> +                commit = TRUE;
> +                break;
> +            case OVS_CT_ATTR_MARK:
> +                mark = NlAttrGet(ctAttr);
>                   break;
> -            case OVS_NAT_ATTR_IP_MIN:
> -                memcpy(&natActionInfo.minAddr,
> -                       NlAttrData(natAttr), NlAttrGetSize(natAttr));
> -                hasMinIp = TRUE;
> +            case OVS_CT_ATTR_LABELS:
> +                labels = NlAttrGet(ctAttr);
>                   break;
> -            case OVS_NAT_ATTR_IP_MAX:
> -                memcpy(&natActionInfo.maxAddr,
> -                       NlAttrData(natAttr), NlAttrGetSize(natAttr));
> -                hasMaxIp = TRUE;
> +            case OVS_CT_ATTR_HELPER:
> +                helper = NlAttrGetString(ctAttr);
> +                if (helper == NULL) {
> +                    return NDIS_STATUS_INVALID_PARAMETER;
> +                }
> +                if (strcmp("ftp", helper) != 0) {
> +                    /* Only support FTP */
> +                    return NDIS_STATUS_NOT_SUPPORTED;
> +                }
>                   break;
> -            case OVS_NAT_ATTR_PROTO_MIN:
> -                natActionInfo.minPort = NlAttrGetU16(natAttr);
> -                hasMinPort = TRUE;
> +            case OVS_CT_ATTR_FORCE_COMMIT:
> +                force = TRUE;
> +                /* Force implicitly means commit */
> +                commit = TRUE;
>                   break;
> -            case OVS_NAT_ATTR_PROTO_MAX:
> -                natActionInfo.maxPort = NlAttrGetU16(natAttr);
> -                hasMaxPort = TRUE;
> +            case OVS_CT_ATTR_EVENTMASK:
> +                eventmask = NlAttrGetU32(ctAttr);
> +                /* Only mark and label updates are supported. */
> +                if (eventmask & (1 << IPCT_MARK | 1 << IPCT_LABEL))
> +                    postUpdateEvent = TRUE;
>                   break;
> -            case OVS_NAT_ATTR_PERSISTENT:
> -            case OVS_NAT_ATTR_PROTO_HASH:
> -            case OVS_NAT_ATTR_PROTO_RANDOM:
> +            case OVS_CT_ATTR_NAT:
> +                natActionInfo.natAction = NAT_ACTION_NONE;
> +                /* Pares Nested NAT attributes. */
> +                PNL_ATTR natAttr;
> +                unsigned int natLeft;
> +                BOOLEAN hasMinIp = FALSE;
> +                BOOLEAN hasMinPort = FALSE;
> +                BOOLEAN hasMaxIp = FALSE;
> +                BOOLEAN hasMaxPort = FALSE;
> +                NL_NESTED_FOR_EACH_UNSAFE (natAttr, natLeft, ctAttr) {
> +                    enum ovs_nat_attr subtype = NlAttrType(natAttr);
> +                    switch(subtype) {
> +                    case OVS_NAT_ATTR_SRC:
> +                    case OVS_NAT_ATTR_DST:
> +                        natActionInfo.natAction |=
> +                            ((subtype == OVS_NAT_ATTR_SRC)
> +                                ? NAT_ACTION_SRC : NAT_ACTION_DST);
> +                        break;
> +                    case OVS_NAT_ATTR_IP_MIN:
> +                        memcpy(&natActionInfo.minAddr,
> +                                NlAttrData(natAttr), NlAttrGetSize(natAttr));
> +                        hasMinIp = TRUE;
> +                        break;
> +                    case OVS_NAT_ATTR_IP_MAX:
> +                        memcpy(&natActionInfo.maxAddr,
> +                                NlAttrData(natAttr), NlAttrGetSize(natAttr));
> +                        hasMaxIp = TRUE;
> +                        break;
> +                    case OVS_NAT_ATTR_PROTO_MIN:
> +                        natActionInfo.minPort = NlAttrGetU16(natAttr);
> +                        hasMinPort = TRUE;
> +                        break;
> +                    case OVS_NAT_ATTR_PROTO_MAX:
> +                        natActionInfo.maxPort = NlAttrGetU16(natAttr);
> +                        hasMaxPort = TRUE;
> +                        break;
> +                    case OVS_NAT_ATTR_PERSISTENT:
> +                    case OVS_NAT_ATTR_PROTO_HASH:
> +                    case OVS_NAT_ATTR_PROTO_RANDOM:
> +                        break;
> +                    }
> +                }
> +                if (natActionInfo.natAction == NAT_ACTION_NONE) {
> +                    natActionInfo.natAction = NAT_ACTION_REVERSE;
> +                }
> +                if (hasMinIp && !hasMaxIp) {
> +                    memcpy(&natActionInfo.maxAddr,
> +                            &natActionInfo.minAddr,
> +                            sizeof(natActionInfo.maxAddr));
> +                }
> +                if (hasMinPort && !hasMaxPort) {
> +                    natActionInfo.maxPort = natActionInfo.minPort;
> +                }
> +                if (hasMinPort || hasMaxPort) {
> +                    if (natActionInfo.natAction & NAT_ACTION_SRC) {
> +                        natActionInfo.natAction |= NAT_ACTION_SRC_PORT;
> +                    } else if (natActionInfo.natAction & NAT_ACTION_DST) {
> +                        natActionInfo.natAction |= NAT_ACTION_DST_PORT;
> +                    }
> +                }
> +                break;
> +            default:
> +                OVS_LOG_TRACE("Invalid netlink attr type: %u", NlAttrType(ctAttr));
>                   break;
> -            }
> -        }
> -        if (natActionInfo.natAction == NAT_ACTION_NONE) {
> -            natActionInfo.natAction = NAT_ACTION_REVERSE;
> -        }
> -        if (hasMinIp && !hasMaxIp) {
> -            memcpy(&natActionInfo.maxAddr,
> -                   &natActionInfo.minAddr,
> -                   sizeof(natActionInfo.maxAddr));
> -        }
> -        if (hasMinPort && !hasMaxPort) {
> -            natActionInfo.maxPort = natActionInfo.minPort;
> -        }
> -        if (hasMinPort || hasMaxPort) {
> -            if (natActionInfo.natAction & NAT_ACTION_SRC) {
> -                natActionInfo.natAction |= NAT_ACTION_SRC_PORT;
> -            } else if (natActionInfo.natAction & NAT_ACTION_DST) {
> -                natActionInfo.natAction |= NAT_ACTION_DST_PORT;
> -            }
> -        }
> -    }
> -    ctAttr = NlAttrFindNested(a, OVS_CT_ATTR_HELPER);
> -    if (ctAttr) {
> -        helper = NlAttrGetString(ctAttr);
> -        if (helper == NULL) {
> -            return NDIS_STATUS_INVALID_PARAMETER;
> -        }
> -        if (strcmp("ftp", helper) != 0) {
> -            /* Only support FTP */
> -            return NDIS_STATUS_NOT_SUPPORTED;
>           }
>       }
> -    ctAttr = NlAttrFindNested(a, OVS_CT_ATTR_FORCE_COMMIT);
> -    if (ctAttr) {
> -        force = TRUE;
> -        /* Force implicitly means commit */
> -        commit = TRUE;
> -    }
> -    ctAttr = NlAttrFindNested(a, OVS_CT_ATTR_EVENTMASK);
> -    if (ctAttr) {
> -        eventmask = NlAttrGetU32(ctAttr);
> -        /* Only mark and label updates are supported. */
> -        if (eventmask & (1 << IPCT_MARK | 1 << IPCT_LABEL))
> -            postUpdateEvent = TRUE;
> -    }
>       /* If newNbl is not allocated, use the current Nbl*/
>       status = OvsCtExecute_(fwdCtx, key, layers,
>                              commit, force, zone, mark, labels, helper, &natActionInfo,
> diff --git a/datapath-windows/ovsext/Conntrack.h b/datapath-windows/ovsext/Conntrack.h
> index 7a80eea..d4152b3 100644
> --- a/datapath-windows/ovsext/Conntrack.h
> +++ b/datapath-windows/ovsext/Conntrack.h
> @@ -237,6 +237,4 @@ NDIS_STATUS OvsCtHandleFtp(PNET_BUFFER_LIST curNbl,
>                              UINT64 currentTime,
>                              POVS_CT_ENTRY entry,
>                              BOOLEAN request);
> -
> -UINT32 OvsHashCtKey(const OVS_CT_KEY *key);
>   #endif /* __OVS_CONNTRACK_H_ */