[ovs-dev] [capwap key: 1/1] datapath: add key support to CAPWAP tunnel
Jesse Gross
jesse at nicira.com
Mon Mar 28 20:34:53 UTC 2011
On Wed, Mar 2, 2011 at 1:11 PM, Valient Gough <valient at gmail.com> wrote:
>
> When testing OVS within Amazon's public EC2 cloud, I found my tests were limited by being unable to use GRE tunnels (the EC2 network transfers only TCP,UDP, and ICMP protocols). Although Capwap can be used, it is not a compete replacement without support for tunnel keys. This patch adds tunnel key support to the capwap vport, so it can be used in place of GRE in such networks.
>
>
> Testing was done on Fedora 13, Ubuntu 10.10, and Amazon EC2 instances (Amazon 32bit Linux AMI). Testing is limited to connections with other OVS instances, which would not catch mis-reads of the Capwap spec.
>
> I still have some testing to do - such as testing fragment handling, but I would appreciate feedback on this approach or technical concerns.
>
>
> Example of an encapsulated packet, with a key of FF00 1100 2200 3300:
>
> ethertype IPv4 (0x0800), length 104: 10.0.2.15.58881 > 10.149.11.3.58882: UDP, length 62
> 0x0000: 4500 005a 0000 4000 4011 18ed 0a00 020f E..Z.. at .@.......
> 0x0010: 0a95 0b03 e601 e602 0046 0000 0030 3c20 .........F...0<.
> 0x0020: 0000 0000 0b80 0000 ff00 1100 2200 3300 ............".3.
> 0x0030: ffff ffff ffff 0023 20c9 60f1 0806 0001 .......#..`.....
> 0x0040: 0800 0604 0001 0023 20c9 60f1 c0a8 0401 .......#..`.....
> 0x0050: 0000 0000 0000 c0a8 0402
>
> Example of an encapsulated packet on a virtual port with no key specified (compatible with existing capwap implementation):
>
> ethertype IPv4 (0x0800), length 92: 10.0.2.15.58881 > 10.149.11.3.58882: UDP, length 50
> 0x0000: 4500 004e 0000 4000 4011 18f9 0a00 020f E..N.. at .@.......
> 0x0010: 0a95 0b03 e601 e602 003a 0000 0010 0200 .........:......
> 0x0020: 0000 0000 ffff ffff ffff 0023 20a8 18cc ...........#....
> 0x0030: 0806 0001 0800 0604 0001 0023 20a8 18cc ...........#....
> 0x0040: c0a8 0201 0000 0000 0000 c0a8 0202 ..............
>
> The primary difference is that bytes 0x1d-0x1f are modified to indicate a larger capwap header, WSI added, along with a modified WBID field (see CAPWAP.txt in patch below), and then a WSI field consisting of 0b80 0000 + [key].
>
>
>
> Valient Gough (1):
> datapath: add key support to CAPWAP tunnel
>
> datapath/CAPWAP.txt | 80 +++++++++++++++++++++++++
> datapath/vport-capwap.c | 151 ++++++++++++++++++++++++++++++++++++++++------
> lib/netdev-vport.c | 13 +++-
> 3 files changed, 220 insertions(+), 24 deletions(-)
> create mode 100644 datapath/CAPWAP.txt
>
> From 9c65ab088e7a43928097ab143edd5d8a8fd310c4 Mon Sep 17 00:00:00 2001
> From: Valient Gough <vgough at pobox.com>
> Date: Tue, 1 Mar 2011 21:24:47 -0800
> Subject: [PATCH] datapath: add key support to CAPWAP tunnel
>
> Add tunnel key support to CAPWAP vport. Uses the optional WSI field in a
> CAPWAP header for storing a 64bit key. It can also be used without keys, in
> which case it is backward compatible with the old code. Documentation about
> the format of the WSI field is in CAPWAP.txt.
>
> Signed-off-by: Valient Gough <vgough at pobox.com>
Sorry about the delay in reviewing this, thank you for being patient.
The general approach and wire-format looks good to me, I have a few
comments and issues that I ran into when test applying this.
Can you cleanup the commit message formatting? When I apply this, I
get all of the above as the commit message, include the diffstats.
I also received some whitespace errors when applying this.
> diff --git a/datapath/CAPWAP.txt b/datapath/CAPWAP.txt
> new file mode 100644
> index 0000000..e59aa71
> --- /dev/null
> +++ b/datapath/CAPWAP.txt
Thanks for documenting this so well. However, it needs to be included
in the list of files in Modules.mk, otherwise make dist fails. When
applying this, I did that by adding an openvswitch_extras and
dist_extras field to the file, since everything is currently either a
source file or a header file.
> + WBID: A 5-bit field that is the wireless binding identifier. The
> + identifier will indicate the type of wireless packet associated
> + with the radio. The following values are defined:
> +
> + 0 - Reserved
> + 1 - IEEE 802.11
> + 2 - Reserved
> + 3 - EPCGlobal [EPCGlobal]
> +
> + When OpenVSwitch uses this field, it writes the value:
> + 30 - OpenVSwitch data
Proper capitalization is Open vSwitch.
> diff --git a/datapath/vport-capwap.c b/datapath/vport-capwap.c
> index 65f1f1b..640a279 100644
> --- a/datapath/vport-capwap.c
> +++ b/datapath/vport-capwap.c
> +/* Old capwap code is hard-coded to look for a WBID value of 2.
> + * When we insert WSI field, use WBID value of 30, which has been
> + * proposed for all "experimental" usage - users with no reserved WBID value
> + * of their own.
> +*/
> +#define CAPWAP_WBID_30 __cpu_to_be32(0x00003C00)
> +#define CAPWAP_WBID_2 __cpu_to_be32(0x00000200)
> +#define CAPWAP_WBID_MASK __cpu_to_be32(0x00003F00)
Is this the right mask? WBID is 5 bits, so I think it should be 0x00003E00.
>
> -#define NO_FRAG_HDR (CAPWAP_BEGIN_HLEN | CAPWAP_BEGIN_WBID)
> +/*
> + * CAPWAP allows an optional 'Wireless Specific Information' field, which is
> + * length prefixed and can contain any data. If keys are configured for the
> + * vport, then the key will be placed in the WSI field and recovered by the
> + * receiver.
> + */
> +#define CAPWAP_FLAG_WSI __cpu_to_be32(0x00000020)
> +
> +#define NO_FRAG_HDR 0
Is there much value in defining this anymore?
> +/*
> + * We use the WSI field to hold additional tunnel data.
> + * The first eight bits store the size of the wsi data in bytes.
> + */
> +struct capwaphdr_wsi {
> + unsigned char wsi_len;
> + unsigned char flags;
Please use u8 to be more precise in field layout definitions.
> static int capwap_hdr_len(const struct tnl_mutable_config *mutable)
> {
> + int size = CAPWAP_HLEN;
Perhaps CAPWAP_HLEN should be renamed to reflect that it is no longer
necessarily the size of the header. For example, I see it still used
in fragment() but I don't believe that it is correct anymore.
> + /* if keys are specified, then add WSI field */
> + if (mutable->out_key || (mutable->flags & TNL_F_OUT_KEY_ACTION))
> + size += 12; /* wsi framing overhead + 8 byte key */
You could use sizeof(struct capwaphdr_wsi) to make the code a little
more self documenting. In general, it would be nice to see more of
that type of thing, instead of special numbers.
> @@ -145,6 +175,35 @@ static void capwap_build_header(const struct vport *vport,
> cwh->begin = NO_FRAG_HDR;
> 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);
> + __be32 *options = (__be32 *)(wsi + 1);
> +
> + cwh->begin |= CAPWAP_FLAG_WSI | CAPWAP_WBID_30;
> +
> + wsi->wsi_len = 3; /* initial wsi header */
> + wsi->flags = 0;
> + wsi->reserved_padding = 0;
> +
> + if (mutable->out_key) {
> + __be64 *options64 = (__be64 *)options;
> + *options64 = mutable->out_key;
> + options += 2;
There's a somewhat strange mixture here (and elsewhere in this file)
between being generic and specific with regards to extensions in the
WSI. I think it makes sense to be generic in the format of the data
that is on the wire, especially since the space is there anyways.
However, for the implementation I'm not sure that there is much
benefit to being generic. For example, what does the arithmetic on
the options get us? There aren't any 32-bit options and in fact there
aren't any other options at all (and I suspect that realistically
there probably won't be more in the future), so why bother? GRE has
something analogous but the situation is different there because it
actually does have several 32-bit options.
> @@ -187,25 +254,69 @@ static int capwap_rcv(struct sock *sk, struct sk_buff *skb)
> - if (unlikely(!pskb_may_pull(skb, CAPWAP_HLEN + ETH_HLEN)))
> + if (unlikely(!pskb_may_pull(skb, hdr_len + ETH_HLEN)))
> goto error;
>
> - __skb_pull(skb, CAPWAP_HLEN);
> - skb_postpull_rcsum(skb, skb_transport_header(skb), CAPWAP_HLEN + ETH_HLEN);
Moving this will break fragmentation reassembly, which assumes that
the header has already been removed.
> + if (cwh->begin & CAPWAP_FLAG_WSI) {
This block should really be in a separate function for clarity.
> + struct capwaphdr_wsi *wsi = (struct capwaphdr_wsi *)(cwh + 1);
> + __be32 *options;
> + int remaining_words;
> +
> + /* minimum wsi length - 4 bytes */
> + if (unlikely(!pskb_may_pull(skb, hdr_len + ETH_HLEN + 4)))
> + goto error;
> +
> + /* len prefix + data must be multiple of 4 bytes */
> + if (unlikely((wsi->wsi_len & 0x3) != 0x3))
> + goto error;
> +
> + hdr_len += 1 + (unsigned int)wsi->wsi_len;
> +
> + if (unlikely(!pskb_may_pull(skb, hdr_len + ETH_HLEN)))
> + goto error;
> +
> + /* don't parse WBID types we don't know about */
> + if (unlikely((cwh->begin & CAPWAP_WBID_MASK) != CAPWAP_WBID_30))
> + goto skip_wsi;
> +
> + /* parse wsi field */
> + remaining_words = (hdr_len >> 2) - 1;
> + options = (__be32 *)(wsi + 1);
These operations and some of the other offsets are too "magical".
> +
> + if ((wsi->flags & CAPWAP_WSI_FLAG_KEY64) && (remaining_words >= 2) ) {
> + __be64 *options64 = (__be64 *)options;
> + key = *options64;
> + options += 2;
> + remaining_words -= 2;
Why update values that are never going to be read again?
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index ae043c2..e63de90 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -624,6 +624,7 @@ parse_tunnel_config(const char *name, const char *type,
> {
> bool is_gre = false;
> bool is_ipsec = false;
> + bool supports_keys = false;
If all protocols now support keys, why not drop the check entirely?
The schema documentation in vswitchd/vswitch.xml should also be
updated to reflect the fact that keys are now supported by capwap via
an extension.
More information about the dev
mailing list