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

[Jesse Gross]

On Sun, Jul 3, 2011 at 5:01 PM, Valient Gough <vgough at pobox.com> wrote:
> Add tunnel key support to CAPWAP vport.  Uses the optional WSI field in a
> CAPWAP header to store a 64bit key.  It can also be used without keys, in which
> case it is backward compatible with the old code.  Documentation about the
> WSI field format is in CAPWAP.txt.
>
> more capwap fixes

Probably can drop the comment on the end from the final commit message.

I some whitespace errors when I apply this:
Applying: datapath: add key support to CAPWAP tunnel
/home/jesse/openvswitch/.git/rebase-apply/patch:144: trailing whitespace.
 *  When we insert WSI field, use WBID value of 30, which has been
/home/jesse/openvswitch/.git/rebase-apply/patch:165: trailing whitespace.

/home/jesse/openvswitch/.git/rebase-apply/patch:233: trailing whitespace.
		size += sizeof(struct capwaphdr_wsi) +
/home/jesse/openvswitch/.git/rebase-apply/patch:250: trailing whitespace.

/home/jesse/openvswitch/.git/rebase-apply/patch:324: trailing whitespace.
static int process_capwap_wsi(struct sk_buff *skb, __be64 *key)
warning: squelched 3 whitespace errors
warning: 8 lines add whitespace errors.

> diff --git a/datapath/vport-capwap.c b/datapath/vport-capwap.c
> index f0bb327..d136a67 100644
> --- a/datapath/vport-capwap.c
> +++ b/datapath/vport-capwap.c
> +#define FRAG_HDR (CAPWAP_BEGIN_FRAG)
> +#define FRAG_LAST_HDR (FRAG_HDR | CAPWAP_BEGIN_FRAG_LAST)

I'm not sure these add much value - the first is an exact duplicate
and the second is only used in one place.

> @@ -142,9 +181,36 @@ static void capwap_build_header(const struct vport *vport,
>        udph->dest = htons(CAPWAP_DST_PORT);
>        udph->check = 0;
>
> -       cwh->begin = NO_FRAG_HDR;
> +       cwh->begin = 0;
>        cwh->frag_id = 0;
>        cwh->frag_off = 0;
> +
> +       if (mutable->out_key || (mutable->flags & TNL_F_OUT_KEY_ACTION)) {
> +               struct capwaphdr_wsi *wsi = (struct capwaphdr_wsi *)(cwh + 1);
> +
> +               cwh->begin |= CAPWAP_FLAG_WSI | CAPWAP_WBID_30;

This could be just an assignment (also the one in the else block) and
then we could drop the assignment above, right?

> +
> +               wsi->wsi_len = sizeof(struct capwaphdr_wsi) - 1;

Can you add a comment to describe where that - 1 comes from?

> +               wsi->flags = 0;
> +               wsi->reserved_padding = 0;
> +
> +               if (mutable->out_key) {
> +                       struct capwaphdr_wsi_key *opt = (struct capwaphdr_wsi_key *)(wsi + 1);
> +                       opt->key = mutable->out_key;
> +
> +                       wsi->wsi_len += sizeof(struct capwaphdr_wsi_key);
> +                       wsi->flags |= CAPWAP_WSI_FLAG_KEY64;
> +               } else {
> +                       /* space left intentionally blank, to be filled in
> +                          by capwap_update_header */
> +               }

If the key is being set through an action then we can still setup the
header here and avoid some duplicate code.

> +
> +               /* set hlen field, easy since we only support 1 option. */
> +               cwh->begin |= CAPWAP_BEGIN_HLEN_5;

Why not combine this with the previous assignment?

> @@ -166,19 +243,39 @@ static inline struct sk_buff *process_capwap_proto(struct sk_buff *skb)
[...]
> +               bool last_frag = (cwh->begin & CAPWAP_BEGIN_FRAG_LAST);
> +               return defrag(skb, last_frag);

This causes a sparse warning:
  CHECK   /home/jesse/openvswitch/datapath/linux/vport-capwap.c
/home/jesse/openvswitch/datapath/linux/vport-capwap.c:249:46: warning:
incorrect type in initializer (different base types)
/home/jesse/openvswitch/datapath/linux/vport-capwap.c:249:46:
expected bool [unsigned] [usertype] last_frag
/home/jesse/openvswitch/datapath/linux/vport-capwap.c:249:46:    got
restricted __be32

It's not really a problem but we could avoid the warning by adding a
cast of (__force bool).

Also, this code no longer checks whether even the most basic elements
of the capwap header are present.  Right now, it will accept anything
without the fragmentation bit set.

> +static int process_capwap_wsi(struct sk_buff *skb, __be64 *key)
> +{
> +       struct capwaphdr *cwh = capwap_hdr(skb);
> +       struct capwaphdr_wsi *wsi;
> +       int min_wsi_len = sizeof(struct capwaphdr_wsi);
> +       int wsi_len;
> +
> +       /* ensure we have at least a minimal wsi header */
> +       if (unlikely(!pskb_may_pull(skb, ETH_HLEN + CAPWAP_MIN_HLEN + min_wsi_len)))
> +               return 0;

The ETH_HLEN here refers to the inner Ethernet header.  You might want
to reorder them to make this clear.

> +       /* read wsi header to find out how big it really is */
> +       wsi = (struct capwaphdr_wsi *)(cwh + 1);
> +       wsi_len = 1 + (unsigned int)wsi->wsi_len;
> +       if (unlikely(!pskb_may_pull(skb, ETH_HLEN + CAPWAP_MIN_HLEN + wsi_len)))
> +               return 0;
> +
> +       /* parse wsi field */
> +       if ((wsi->flags & CAPWAP_WSI_FLAG_KEY64) &&
> +           (wsi_len > sizeof(struct capwaphdr_wsi_key))) {
> +               struct capwaphdr_wsi_key *opt = (struct capwaphdr_wsi_key *)(wsi + 1);
> +               *key = opt->key;
>        }

Should we just drop the packet if it indicates that the key should be
there and it isn't long enough?

> +
> +       return 1;

Usually integer error codes return 0 on success.  If you want to do it
this way, I would use a bool.

> @@ -187,25 +284,44 @@ static int capwap_rcv(struct sock *sk, struct sk_buff *skb)
> +       cwh = capwap_hdr(skb);
> +       if (cwh->begin & CAPWAP_FLAG_WSI && (
> +           cwh->begin & CAPWAP_WBID_MASK) == CAPWAP_WBID_30) {

The trailing parenthesis is somewhat weird here.

>  static struct sk_buff *fragment(struct sk_buff *skb, const struct vport *vport,
> -                               struct dst_entry *dst)
> +                               struct dst_entry *dst, const struct tnl_mutable_config *mutable)
[...]
> +       if (unlikely(capwap_len <= 0)) {
> +               pr_warn("invalid capwap header length: %d", capwap_len);
> +               goto error;
> +       }

This condition should never occur because it would indicate that
configuration is invalid, which would prevent the tunnel from being
created in the first place.

> +/* FIXME: fragment reassembly is broken */

The problem is that you moved the call to __skb_pull() from before
process_capwap_proto() to afterwards.  This means that you will try to
reassemble packets with the capwap header as part of the payload.

Otherwise, I don't think that reassembly needs to change too much to
handle keys.  One issue is what happens if the key is different in
different fragments.