[ovs-dev] [PATCH] Add Geneve support on Windows datapath.

[Jesse Gross]

On Tue, May 17, 2016 at 5:02 PM, Yin Lin <linyi at vmware.com> wrote:
> diff --git a/datapath-windows/ovsext/Geneve.c b/datapath-windows/ovsext/Geneve.c
> new file mode 100644
> index 0000000..d3eed86
> --- /dev/null
> +++ b/datapath-windows/ovsext/Geneve.c
> +NDIS_STATUS OvsEncapGeneve(POVS_VPORT_ENTRY vport,
> +                           PNET_BUFFER_LIST curNbl,
> +                           OvsIPv4TunnelKey *tunKey,
> +                           POVS_SWITCH_CONTEXT switchContext,
> +                           POVS_PACKET_HDR_INFO layers,
> +                           PNET_BUFFER_LIST *newNbl)
[...]

I'm not planning on reviewing the whole patch since I don't know the
Windows implementation well but I have a couple of quick comments on
the protocol implementation itself:

> +        /* UDP header */
> +        udpHdr = (UDPHdr *)((PCHAR)ipHdr + sizeof *ipHdr);
> +        udpHdr->source = htons(tunKey->flow_hash | MAXINT16);
> +        udpHdr->dest = htons(vportGeneve->dstPort);
> +        udpHdr->len = htons(NET_BUFFER_DATA_LENGTH(curNb) - headRoom +
> +                            sizeof *udpHdr + sizeof *geneveHdr + tunKey->tunOptLen);
> +        udpHdr->check = 0;

Is the UDP checksum computed anywhere? If userspace requests it then
we should fill it in. Conversely, on the receive side, it looks like
the checksum is verified if present but OVS_TNL_F_CSUM is not set to
report this to userspace.

It's possible that I might be missing something though since I also
don't see where this is done for other tunneling protocols.

> +        /* Geneve header */
> +        geneveHdr = (GeneveHdr *)((PCHAR)udpHdr + sizeof *udpHdr);
> +        geneveHdr->version = GENEVE_VER;
> +        geneveHdr->optLen = tunKey->tunOptLen / 4;
> +        geneveHdr->oam = !!(tunKey->flags & OVS_TNL_F_OAM);
> +        geneveHdr->critical = 0;
> +        geneveHdr->reserved1 = 0;
> +        geneveHdr->protocol = htons(ETH_P_TEB);
> +        geneveHdr->vni = GENEVE_TUNNELID_TO_VNI(tunKey->tunnelId);
> +        geneveHdr->reserved2 = 0;
> +
> +        /* Geneve header options */
> +        optHdr = (GeneveOptionHdr *)(geneveHdr + 1);
> +        memcpy(optHdr, TunnelKeyGetOptions(tunKey), tunKey->tunOptLen);
> +        geneveHdr->critical = OvsTunnelKeyIsCritical(tunKey) ? 1 : 0;
> +        for (i = 0; i < tunKey->tunOptLen; i++) {
> +            if (optHdr[i].type & GENEVE_CRIT_OPT_TYPE) {
> +                geneveHdr->critical = 1;
> +            }
> +        }
> +    }

Looking at the way that the geneveHdr->critical bit is generated, a
few things seem suspicious:
 * The options are themselves variable length, so we can't just treat
them as an array of headers - we'll need to look at the length for
each option to find the next header. (The for loop counter also seems
to be comparing an index against a byte length, causing us to read out
of bounds.)
 * It looks like the check is done twice - once in
OvsTunnelKeyIsCritical and again in the for loop below it.
 * It would be nice to do this operation once per flow at setup time
instead of per packet.