[ovs-dev] [PATCH v2] datapath-windows: Add support for UDP and ICMP to Conntrack Module

[Alin Serdean]

I am unsure if we actual need another file but that maybe a personal preference.

Other comments inlined.

Alin.
> +
> +static const long long other_timeouts[] = {
> +    [OTHERS_FIRST] = 60 * 10000000,
> +    [OTHERS_MULTIPLE] = 60 * 10000000,
> +    [OTHERS_BIDIR] = 30 * 10000000,
> +};
[Alin Gabriel Serdean: ] Maybe define 10000000 somewhere and add a comment on how many ms/s they will be.
> +
> +static __inline struct conn_other*
> +OvsCastConntrackEntryToOtherEntry(OVS_CT_ENTRY *conn) {
> +    return CONTAINER_OF(conn, struct conn_other, up); }
[Alin Gabriel Serdean: ] Add an assert conn
> +
> +static __inline VOID
> +OvsConntrackUpdateExpiration(struct conn_other *conn, long long now) {
> +    conn->up.expiration = now + other_timeouts[conn->state]; }
[Alin Gabriel Serdean: ] Add an assert to conn
> +
> +enum ct_update_res
> +OvsConntrackUpdateOtherEntry(OVS_CT_ENTRY *conn_,
> +                             BOOLEAN reply,
> +                             UINT64 now) {
> +    struct conn_other *conn = OvsCastConntrackEntryToOtherEntry(conn_);
[Alin Gabriel Serdean: ] Add an assert to conn_
> +
> +OVS_CT_ENTRY *
> +OvsConntrackCreateOtherEntry(UINT64 now) {
> +    struct conn_other *conn;
> +    conn = OvsAllocateMemoryWithTag(sizeof(struct conn_other),
> +                                    OVS_CT_POOL_TAG);
> +    /* XXX Handle memory allocation error*/
[Alin Gabriel Serdean: ] Please handle the memory allocation or at least add an assert to it
> +    conn->up = (OVS_CT_ENTRY) {0};
> +    conn->state = OTHERS_FIRST;
> +    OvsConntrackUpdateExpiration(conn, now);
> +    return &conn->up;
> +}
> \ No newline at end of file
[Alin Gabriel Serdean: ] Nit no new line
> diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-
> windows/ovsext/Conntrack.c
> index 544fd51..e193e9f 100644
> --- a/datapath-windows/ovsext/Conntrack.c
> +++ b/datapath-windows/ovsext/Conntrack.c
> @@ -146,9 +146,20 @@ OvsCtUpdateFlowKey(struct OvsFlowKey *key,
>      }
>  }
> 
> +static __inline VOID
> +OvsCtAddEntry(POVS_CT_ENTRY entry, OvsConntrackKeyLookupCtx *ctx) {
> +    NdisMoveMemory(&entry->key, &ctx->key, sizeof (OVS_CT_KEY));
> +    NdisMoveMemory(&entry->rev_key, &ctx->key, sizeof (OVS_CT_KEY));
[Alin Gabriel Serdean: ] Maybe use RtlCopyMemory
> +    OvsCtKeyReverse(&entry->rev_key);
> +    InsertHeadList(&ovsConntrackTable[ctx->hash &
> CT_HASH_TABLE_MASK],
> +                   &entry->link);
> +}
> +
>  static __inline POVS_CT_ENTRY
> -OvsCtEntryCreate(const TCPHdr *tcp,
> -                 PNET_BUFFER_LIST curNbl,
> +OvsCtEntryCreate(PNET_BUFFER_LIST curNbl,
> +                 UINT8 ipProto,
> +                 UINT32 l4Offset,
>                   OvsConntrackKeyLookupCtx *ctx,
>                   OvsFlowKey *key,
>                   BOOLEAN commit,
> @@ -156,26 +167,71 @@ OvsCtEntryCreate(const TCPHdr *tcp,  {
>      POVS_CT_ENTRY entry = NULL;
>      UINT32 state = 0;
> -    if (!OvsConntrackValidateTcpPacket(tcp)) {
> -        state |= OVS_CS_F_INVALID;
> -        OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL);
> -        return entry;
> -    }
> +    switch (ipProto)
> +    {
> +        case IPPROTO_TCP:
> +        {
> +            TCPHdr tcpStorage;
> +            const TCPHdr *tcp;
> +            tcp = OvsGetTcp(curNbl, l4Offset, &tcpStorage);
> +            if (!OvsConntrackValidateTcpPacket(tcp)) {
> +                goto invalid;
> +            }
> +
> +            state |= OVS_CS_F_NEW;
> +            if (commit) {
> +                entry = OvsConntrackCreateTcpEntry(tcp, curNbl, currentTime);
> +                OvsCtAddEntry(entry, ctx);
> +            }
> +
> +            OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL);
> +            return entry;
> +        }
> +        case IPPROTO_ICMP:
> +        case IPPROTO_UDP:
> +            state |= OVS_CS_F_NEW;
> +            if (commit) {
> +                entry = OvsConntrackCreateOtherEntry(currentTime);
> +                OvsCtAddEntry(entry, ctx);
> +            }
> 
> -    state |= OVS_CS_F_NEW;
> -    if (commit) {
> -        entry = OvsConntrackCreateTcpEntry(tcp, curNbl, currentTime);
> -        NdisMoveMemory(&entry->key, &ctx->key, sizeof (OVS_CT_KEY));
> -        NdisMoveMemory(&entry->rev_key, &ctx->key, sizeof (OVS_CT_KEY));
> -        OvsCtKeyReverse(&entry->rev_key);
> -        InsertHeadList(&ovsConntrackTable[ctx->hash &
> CT_HASH_TABLE_MASK],
> -                       &entry->link);
> +            OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL);
> +            return entry;
> +        default:
> +            goto invalid;
>      }
> 
> +invalid:
> +    state |= OVS_CS_F_INVALID;
>      OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL);
>      return entry;
>  }
> 
> +static enum CT_UPDATE_RES
> +OvsCtUpdateEntry(OVS_CT_ENTRY* entry,
> +                        PNET_BUFFER_LIST nbl,
> +                        UINT8 ipProto,
> +                        UINT32 l4Offset,
> +                        BOOLEAN reply,
> +                        UINT64 now)
> +{
> +    switch (ipProto)
> +    {
> +        case IPPROTO_TCP:
> +        {
> +            TCPHdr tcpStorage;
> +            const TCPHdr *tcp;
> +            tcp = OvsGetTcp(nbl, l4Offset, &tcpStorage);
[Alin Gabriel Serdean: ] add in if (tcp) in the case it may be null
> +            return OvsConntrackUpdateTcpEntry(entry, tcp, nbl, reply, now);
> +        }
> +        case IPPROTO_ICMP:
> +        case IPPROTO_UDP:
> +            return OvsConntrackUpdateOtherEntry(entry, reply, now);
> +        default:
> +            return CT_UPDATE_INVALID;
> +    }
> +}
> +