diff mbox

[ovs-dev,v3,1/2] OF support and translation of generic encap and decap

Message ID 79BBBFE6CB6C9B488C1A45ACD284F51961C16D4B@SHSMSX103.ccr.corp.intel.com
State Superseded
Headers show

Commit Message

Yang, Yi Aug. 1, 2017, 12:32 p.m. UTC 
Hi, Ben

I have posted v4, they are:

https://mail.openvswitch.org/pipermail/ovs-dev/2017-August/336504.html
https://mail.openvswitch.org/pipermail/ovs-dev/2017-August/336505.html
https://mail.openvswitch.org/pipermail/ovs-dev/2017-August/336506.html

I have fixed most of issues you commented, I need to clarify the below issues, actually they are not issues. I have explained the other issues in last email.

#1.
[Ben] I don't see anything in decode_ofp_prop() or its caller that checks that the property fits in the available space.  It looks like an integer overflow error.

[Yi] Currently it is a stub function, it doesn't handle any property, so it doesn't put any property into buf, so we mustn't check space at this point. Our NSH patch series will add more pieces for this. Let us check 

enum ofperr
decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop,
               struct ofpbuf *out OVS_UNUSED,
               size_t *remaining)

We used ofpbuf_put_uninit(out, ...) to put the property to buffer, so we needn't care the available space, ofpbuf_put_uninit will allocate on demand.

#2.
[Ben] I suspect that decode_NXAST_RAW_DECAP() should report an error if properties are present, since it doesn't support properties.

[Yi] It is impossible. 

static enum ofperr
decode_NXAST_RAW_DECAP(const struct nx_action_decap *nad,
                       enum ofp_version ofp_version OVS_UNUSED,
                       struct ofpbuf *ofpacts)
{
    struct ofpact_decap * decap;

    decap = ofpact_put_DECAP(ofpacts);
    decap->ofpact.raw = NXAST_RAW_DECAP;
    decap->new_pkt_type = nad->new_pkt_type;
    return 0;
}

struct ofpact_decap {
    struct ofpact ofpact;

    /* New packet type.
     *
     * The special value (0,0xFFFE) "Use next proto" is used to request OVS to
     * automatically set the new packet type based on the decap'ed header's
     * next protocol.
     */
    ovs_be32 new_pkt_type;
};

struct nx_action_decap {
    ovs_be16 type;         /* OFPAT_VENDOR. */
    ovs_be16 len;          /* Total size including any property TLVs. */
    ovs_be32 vendor;       /* NX_VENDOR_ID. */
    ovs_be16 subtype;      /* NXAST_DECAP. */
    uint8_t pad[2];        /* 2 bytes padding */

    /* Packet type or result.
     *
     * The special value (0,0xFFFE) "Use next proto"
     * is used to request OVS to automatically set the new packet type based
     * on the decap'ed header's next protocol.
     */
    ovs_be32 new_pkt_type;
};


-----Original Message-----
From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Ben Pfaff
Sent: Monday, July 31, 2017 11:43 PM
To: Zoltán Balogh <zoltan.balogh.eth@gmail.com>
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH v3 1/2] OF support and translation of generic encap and decap

On Fri, Jul 21, 2017 at 07:08:25PM +0200, Zoltán Balogh wrote:
> From: Jan Scheurich <jan.scheurich@ericsson.com>
> 
> This commit adds support for the OpenFlow actions generic encap and 
> decap (as specified in ONF EXT-382) to the OVS control plane.
> 
> CLI syntax for encap action with properties:
>   encap(hdr=<header>)
>   encap(hdr=<header>,
>         prop(class=<class>,type=<type>,val=<simple>),
>         prop(class=<class>,type=<type>,val(<complex>)))
> 
> CLI syntax for decap action:
>   decap()
>   decap(packet_type(ns=<pt_ns>,type=<pt_type>))
> 
> The first header supported for encap and decap is "ethernet" to 
> convert packets between packet_type (1,Ethertype) and (0,0).
> 
> This commit also implements a skeleton for the translation of generic 
> encap and decap actions in ofproto-dpif and adds support to encap and 
> decap an Ethernet header.
> 
> In general translation of encap commits pending actions and then 
> rewrites struct flow in accordance with the new packet type and 
> header. In the case of encap(ethernet) it suffices to change the 
> packet type from (1, Ethertype) to (0,0) and set the dl_type 
> accordingly. A new pending_encap flag in xlate ctx is set to mark that 
> an corresponding datapath encap action must be triggered at the next 
> commit. In the case of encap(ethernet) ofproto generetas a push_eth action.
> 
> The general case for translation of decap() is to emit a datapath 
> action to decap the current outermost header and then recirculate the 
> packet to reparse the inner headers. In the special case of an 
> Ethernet packet,
> decap() just changes the packet type from (0,0) to (1, dl_type) 
> without a need to recirculate. The emission of the pop_eth action for 
> the datapath is postponed to the next commit.
> 
> Hence encap(ethernet) and decap() on an Ethernet packet are OF octions 
> that only incur a cost in the dataplane when a modifed packet is 
> actually committed, e.g. because it is sent out. They can freely be 
> used for normalizing the packet type in the OF pipeline without 
> degrading performance.
> 
> Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com>
> Signed-off-by: Yi Yang <yi.y.yang@intel.com>
> Signed-off-by: Zoltan Balogh <zoltan.balogh@ericsson.com>
> Co-authored-by: Zoltan Balogh <zoltan.balogh@ericsson.com>

Thanks for working on this.  I think that it will be a valuable feature.

First, I'm having a really hard time figuring out what we actually mean when we say EXT-382.  The ONF repo has 8 branches with 382 in the name:

  remotes/extensibility/ext/mszczesn/ext-382
  remotes/extensibility/spec/benmc/EXT-382-566
  remotes/extensibility/spec/benmc/ext-382
  remotes/extensibility/spec/benmc/ext-382-112
  remotes/extensibility/spec/jean/ext-382
  remotes/extensibility/spec/jean/ext-382-112
  remotes/extensibility/spec/jean/ext-382-112-mark-ben
  remotes/extensibility/spec/mszczesn/ext-382-112

The EXT-382 ticket in the ONF JIRA has over 20 attachments.  All of them are over a year old.

There doesn't seem to be anything that's actually going to be released as EXT-382, nor is any progress being made toward OpenFlow 1.6, nor was
EXT-382 incorporated into OpenFlow before progress stopped.  I think that we need to be more specific about what spec we're implementing here.  Possibly, we need to document in detail what OVS is actually implementing, since it's hard for users to find the spec.

Given that the spec isn't clear, we need to be really clear in the source code.  One place we aren't is padding.  The comment on struct ofp_ed_prop_header says that "These must be padded to a multiple of 4 bytes." but decode_ed_prop(), format_ed_props(), and probably other code actually uses multiples of 8 bytes.

I don't see anything in decode_ofp_prop() or its caller that checks that the property fits in the available space.  It looks like an integer overflow error.

I still get a pair of alignment errors (use ALIGNED_CAST, please):

    ../lib/ofp-ed-props.c:42:17: error: cast from 'char *' to 'const struct ofp_ed_prop_header *' increases required alignment from 1 to 2 [-Werror,-Wcast-align]
    ../lib/ofp-ed-props.c:58:13: error: cast from 'char *' to 'const struct ofpact_ed_prop *' increases required alignment from 1 to 2 [-Werror,-Wcast-align]


In struct ofpact_decap, thanks for the detailed comments.  For long comments on individual members, please put them above the member instead of next to it, because it is easier to read that way.  Also, we don't use the abbreviation "OvS": please say "OVS" instead.

Similar feedback for the long comment in enum ofp_ed_prop_class.

It's not clear what OFPACT_ENCAP_MAX_PROP_SIZE is for.  It's not used anywhere.

The new actions need to be documented in the ovs-ofctl manpage.

The string format of the encap actions seems rather user unfriendly.  Is there a good reason why it should be the very generic
prop(class=<class>,type=<type>,val=<val>) rather than something more tailored to the actual properties that users will want to set?  It is hard to tell without any actual examples.

In decode_NXAST_RAW_ENCAP() and parse_ENCAP(), shouldn't there be validation of the header size value?  Most header types will only have a few acceptable header sizes.

The internal representation of ofpact_ed_prop, has no examples so far.
How confident are you that it actually needs to be variable-length?  If it does, then using a member 'n_props' in struct ofpact_encap to count the number of properties seems risky: it implicitly encourages programmers to try to index the props[] array from 0 to n_props-1.  I'd encourage, in that case, switching to a length member, or just eliminating the member if ofpact_ed_props can be padded to 8-byte multiples.  But it's easier if we can just make ofpact_ed_prop fixed-length for now; then we can just index props[] as an array.

However, I question whether the properties actually need to be internally represented as properties at all.  Presumably, OVS is only going to support a specific set of properties.  Probably, it's easy to just add specific members to struct ofpact_encap that represent the values of those properties.  Did you consider that?  That approach usually simplifies code greatly; properties are usually needed only for external representations.

I suspect that decode_NXAST_RAW_DECAP() should report an error if properties are present, since it doesn't support properties.

In encode_DECAP(), please use sizeof instead of hard-coding a size of 16.

I can't find anything in the EXT-382 draft that talks about where encap and decap go in the action set execution order.  How did you decide?
The description of the action set execution order in ovs-ofctl(8) should be updated (mentioning that encap and decap are nonstandard).

Thanks again, and I'll look forward to the next version.

I'm appending an incremental that implements a few of the more stylistic requests above plus a few other minor things I noticed.

--8<--------------------------cut here-------------------------->8--

Comments

Ben Pfaff Aug. 1, 2017, 3:43 p.m. UTC | #1 
On Tue, Aug 01, 2017 at 08:26:07AM -0700, Ben Pfaff wrote:
> On Tue, Aug 01, 2017 at 12:32:20PM +0000, Yang, Yi Y wrote:
> > #2.
> > [Ben] I suspect that decode_NXAST_RAW_DECAP() should report an error if properties are present, since it doesn't support properties.
> > 
> > [Yi] It is impossible. 
> 
> What is impossible?  It is easy to detect that properties are present
> and report an error:
> 
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index d0437f20922a..7be302a4005d 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -4263,6 +4263,11 @@ decode_NXAST_RAW_DECAP(const struct nx_action_decap *nad,
>                         enum ofp_version ofp_version OVS_UNUSED,
>                         struct ofpbuf *ofpacts)
>  {
> +    if (ntohs(nad->len) > sizeof *nad) {
> +        /* No properties supported yet. */
> +        return OFPERR_OFPBPC_BAD_TYPE;
> +    }

Looking again, I guess that OFPERR_NXBAC_UNKNOWN_ED_PROP is better.
diff mbox

Patch

diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h
index 7b9f6c199968..1bab42976482 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -976,9 +976,7 @@  struct ofpact_unroll_xlate {
 
 /* OFPACT_ENCAP.
  *
- * Used for OFPAT_ENCAP. */
-
-#define OFPACT_ENCAP_MAX_PROP_SIZE 256
+ * Used for NXAST_ENCAP. */
 
 struct ofpact_encap {
     struct ofpact ofpact;
@@ -990,14 +988,16 @@  struct ofpact_encap {
 
 /* OFPACT_DECAP.
  *
- * Used for OFPAT_DECAP. */
+ * Used for NXAST_DECAP. */
 struct ofpact_decap {
     struct ofpact ofpact;
-    ovs_be32 new_pkt_type;         /* New packet type. The special value
-                                      (0,0xFFFE) "Use next proto" is used to
-                                      request OvS to automatically set the
-                                      new packet type based on the decap'ed
-                                      header's next protocol.*/
+
+    /* New packet type.
+     *
+     * The special value (0,0xFFFE) "Use next proto" is used to request OVS to
+     * automatically set the new packet type based on the decap'ed header's
+     * next protocol. */
+    ovs_be32 new_pkt_type;
 };
 
 /* Converting OpenFlow to ofpacts. */
diff --git a/include/openvswitch/ofp-ed-props.h b/include/openvswitch/ofp-ed-props.h
index 1db7e56dfa4b..d45aa51a71a9 100644
--- a/include/openvswitch/ofp-ed-props.h
+++ b/include/openvswitch/ofp-ed-props.h
@@ -23,21 +23,21 @@ 
 
 enum ofp_ed_prop_class {
     OFPPPC_BASIC = 0,            /* ONF Basic class. */
-    OFPPPC_MPLS  = 1,            /* MPLS property  class. */
-    OFPPPC_GRE   = 2,            /* GRE property  class. */
-    OFPPPC_GTP   = 3,            /* GTP property  class. */
-    /*  new values go here  */
-    OFPPPC_EXPERIMENTER=0xffff,  /* Experimenter property class.
-                                  * First 32 bits of property data is exp
-                                  * id after that is the experimenter
-                                  * property data. */
+    OFPPPC_MPLS  = 1,            /* MPLS property class. */
+    OFPPPC_GRE   = 2,            /* GRE property class. */
+    OFPPPC_GTP   = 3,            /* GTP property class. */
+
+    /* Experimenter property class.
+     *
+     * First 32 bits of property data is experimenter ID, after that is the
+     * experimenter property data. */
+    OFPPPC_EXPERIMENTER = 0xffff,
 };
 
 /*
  * External representation of encap/decap properties.
  * These must be padded to a multiple of 4 bytes.
  */
-
 struct ofp_ed_prop_header {
     ovs_be16 prop_class;
     uint8_t type;
@@ -47,7 +47,6 @@  struct ofp_ed_prop_header {
 /*
  * Internal representation of encap/decap properties
  */
-
 struct ofpact_ed_prop {
     uint16_t prop_class;
     uint8_t type;
diff --git a/lib/odp-util.c b/lib/odp-util.c index a9a99486a9f1..84ca8ed6e04f 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -6339,8 +6339,7 @@  commit_packet_type_change(const struct flow *flow,
         switch (ntohl(flow->packet_type)) {
         case PT_ETH: {
             /* push_eth */
-            odp_put_push_eth_action(odp_actions, &flow->dl_src,
-                    &flow->dl_dst);
+            odp_put_push_eth_action(odp_actions, &flow->dl_src, 
+ &flow->dl_dst);
             base_flow->packet_type = flow->packet_type;
             base_flow->dl_src = flow->dl_src;
             base_flow->dl_dst = flow->dl_dst; diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index e3e84100e0f2..94f2d0ba760b 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -4028,9 +4028,9 @@  format_FIN_TIMEOUT(const struct ofpact_fin_timeout *a,
     ds_put_format(s, "%s)%s", colors.paren, colors.end);  }
 
-/* Action structure for OFPAT_ENCAP */
+/* Action structure for NXAST_ENCAP */
 struct nx_action_encap {
-    ovs_be16 type;         /* OFPAT_ENCAP */
+    ovs_be16 type;         /* OFPAT_VENDOR. */
     ovs_be16 len;          /* Total size including any property TLVs. */
     ovs_be32 vendor;       /* NX_VENDOR_ID. */
     ovs_be16 subtype;      /* NXAST_ENCAP. */
@@ -4235,9 +4235,9 @@  format_ENCAP(const struct ofpact_encap *a,
     ds_put_format(s, "%s)%s", colors.paren, colors.end);  }
 
-/* Action structure for OFPAT_DECAP */
+/* Action structure for NXAST_DECAP */
 struct nx_action_decap {
-    ovs_be16 type;         /* OFPAT_DECAP */
+    ovs_be16 type;         /* OFPAT_VENDOR */
     ovs_be16 len;          /* Total size including any property TLVs. */
     ovs_be32 vendor;       /* NX_VENDOR_ID. */
     ovs_be16 subtype;      /* NXAST_DECAP. */
diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c index d2d91bebfb9a..3b9f5b923605 100644
--- a/lib/ofp-ed-props.c
+++ b/lib/ofp-ed-props.c
@@ -39,8 +39,8 @@  decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop,
     }
 
     *remaining -= pad_len;
-    *ofp_prop = (const struct ofp_ed_prop_header *)
-            ((char *)(*ofp_prop) + pad_len);
+    *ofp_prop = ALIGNED_CAST(const struct ofp_ed_prop_header *,
+                             (char *) *ofp_prop + pad_len);
     return 0;
 }
 
@@ -55,7 +55,8 @@  encode_ed_prop(const struct ofpact_ed_prop **prop,
         return OFPERR_OFPBAC_BAD_ARGUMENT;
     }
 
-    *prop = (const struct ofpact_ed_prop *) ((char *)(*prop) + prop_len);
+    *prop = ALIGNED_CAST(const struct ofpact_ed_prop *,
+                         (char *) *prop + prop_len);
     return 0;
 }