[ovs-dev] [PATCH 1/1] datapath: add key support to CAPWAP tunnel

[Jesse Gross]

On Wed, Jul 13, 2011 at 10:05 PM, Valient Gough <vgough at pobox.com> wrote:
> diff --git a/datapath/vport-capwap.c b/datapath/vport-capwap.c
> index f0bb327..161c6d4 100644
> --- a/datapath/vport-capwap.c
> +++ b/datapath/vport-capwap.c
> +/*
> + * Should be sized as multiple of u32, for hash functions.
> + */
>  struct frag_match {
>        __be32 saddr;
>        __be32 daddr;
>        __be16 id;
> +       __be16 reserved;
> +       __be64 key;
>  };

I think this violates the CAPWAP spec because it changes the
parameters that reassembly is based on.  A device that understands
keys can now put together packets in a different way than one that
does not if there are overlapping IDs.  If you go down the route of
checking that all of the fragments have the same ID, I think you need
to do reassembly as before and check to see whether all segments have
the same key and drop if not.  Probably the simpler implementation
would be to just declare that only the key in the first fragment
matters.

> +static int process_capwap_wsi(struct sk_buff *skb, __be64 *key, int *hdr_len)
>  {
>        struct capwaphdr *cwh = capwap_hdr(skb);
> +       struct capwaphdr_wsi *wsi = (struct capwaphdr_wsi *)(cwh + 1);
> +       int min_wsi_len = sizeof(struct capwaphdr_wsi);
> +       int wsi_len;

You can't use these points after the calls to pskb_may_pull() below as
they may reallocate memory and cause the pointers to be invalid.

> -       if (likely(cwh->begin == NO_FRAG_HDR))
> -               return skb;
> -       else if (cwh->begin == FRAG_HDR)
> -               return defrag(skb, false);
> -       else if (cwh->begin == FRAG_LAST_HDR)
> -               return defrag(skb, true);
> -       else {
> -               if (net_ratelimit())
> -                       pr_warn("unparsable packet receive on capwap socket\n");
> +       if (unlikely(!pskb_may_pull(skb, CAPWAP_MIN_HLEN + min_wsi_len + ETH_HLEN)))
> +               return -ENOMEM;
>
> -               kfree_skb(skb);
> -               return NULL;
> +       /* read wsi header to find out how big it really is */
> +       wsi = (struct capwaphdr_wsi *)(cwh + 1);
> +       /* +1 for length byte not included in wsi_len */
> +       wsi_len = 1 + (unsigned int)wsi->wsi_len;
> +       *hdr_len += wsi_len;
> +
> +       if (unlikely(!pskb_may_pull(skb, CAPWAP_MIN_HLEN + wsi_len + ETH_HLEN)))
> +               return -ENOMEM;
> +
> +       /* parse wsi field */
> +       if (wsi->flags & CAPWAP_WSI_F_KEY64) {
> +               struct capwaphdr_wsi_key *opt = (struct capwaphdr_wsi_key *)(wsi + 1);
> +               if (unlikely(wsi_len <= sizeof(struct capwaphdr_wsi_key)))
> +                       return -EINVAL;

Is this length check correct?  struct capwaphdr_wsi_key is just the 8
byte value but wsi_len includes struct capwaphdr_wsi as well.

> +static inline struct sk_buff *process_capwap_proto(struct sk_buff *skb,
> +                                                  __be64 *key)
> +{
> +       struct capwaphdr *cwh = capwap_hdr(skb);
> +       int hdr_len = CAPWAP_MIN_HLEN;
> +       int wbid = cwh->begin & CAPWAP_WBID_MASK;

This triggers sparse errors; it can be fixed by using __be32 instead.

> @@ -628,7 +768,8 @@ static int capwap_frag_match(struct inet_frag_queue *ifq, void *a_)
>        struct frag_match *a = a_;
>        struct frag_match *b = &ifq_cast(ifq)->match;
>
> -       return a->id == b->id && a->saddr == b->saddr && a->daddr == b->daddr;
> +       return a->id == b->id && a->saddr == b->saddr &&
> +               a->daddr == b->daddr && a->key == b->key;

Might as well use memcmp() here now that the struct won't have any
extra padding in it.