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

[Ben Pfaff]

On Tue, Aug 01, 2017 at 08:06:59PM +0800, Yi Yang wrote:
> From: Jan Scheurich <jan.scheurich at 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 at ericsson.com>
> Signed-off-by: Yi Yang <yi.y.yang at intel.com>
> Signed-off-by: Zoltan Balogh <zoltan.balogh at ericsson.com>
> Co-authored-by: Zoltan Balogh <zoltan.balogh at ericsson.com>

Thanks for iterating so quickly!

Besides the syntax for properties, which I still think should be
simplified to e.g. nsh(md_type=1), I have only a few remaining comments.

I don't think there's any value in the n_props member in
nx_action_encap.  (This is about the OpenFlow wire format now, not the
internal format.)  The properties are the whole tail of the structure
and having a count doesn't make anything easier.  Can you remove it?  It
will allow us to drop 8 bytes from the action structure due to padding.
(In case it can't be removed, I'm providing a spelling fix.)

decode_ed_prop() still doesn't check the length properly, so I'm
providing a fix.

I'm appending my suggested incremental.

Thanks again!

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

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index d0437f20922a..c7d47eb5dd71 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -4036,7 +4036,7 @@ struct nx_action_encap {
     ovs_be16 subtype;      /* NXAST_ENCAP. */
     ovs_be16 hdr_size;     /* Header size in bytes, 0 = 'not specified'.*/
     ovs_be32 new_pkt_type; /* Header type to add and PACKET_TYPE of result. */
-    ovs_be16 n_props;      /* Number of the following endecap properties. */
+    ovs_be16 n_props;      /* Number of the following encap properties. */
     uint8_t pad[6];        /* Align to 8 bytes boundary */
     struct ofp_ed_prop_header props[];  /* Encap TLV properties. */
 };
@@ -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;
+    }
+
     struct ofpact_decap * decap;
 
     decap = ofpact_put_DECAP(ofpacts);
diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c
index 260adc30acf0..6bba3ac7d982 100644
--- a/lib/ofp-ed-props.c
+++ b/lib/ofp-ed-props.c
@@ -32,6 +32,9 @@ decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop,
     uint16_t prop_class = ntohs((*ofp_prop)->prop_class);
     size_t len = (*ofp_prop)->len;
     size_t pad_len = ROUND_UP(len, 8);
+    if (pad_len > *remaining) {
+        return OFPERR_OFPBAC_BAD_LEN;
+    }
 
     switch (prop_class) {
     default: