diff mbox

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

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

Commit Message

Yang, Yi Aug. 1, 2017, 11:06 p.m. UTC 
About why we need n_props in nx_action_encap, I added this for Opendaylight to deserialize the wire format from OVS, n_props can clearly tell Opendaylight if there is any property. Otherwise how do we check if there is a property following?

-----Original Message-----
From: Ben Pfaff [mailto:blp@ovn.org] 
Sent: Tuesday, August 1, 2017 11:56 PM
To: Yang, Yi Y <yi.y.yang@intel.com>
Cc: dev@openvswitch.org; Jan Scheurich <jan.scheurich@ericsson.com>; Zoltan Balogh <zoltan.balogh@ericsson.com>
Subject: Re: [PATCH v4 1/2] OF support and translation of generic encap and decap

On Tue, Aug 01, 2017 at 08:06:59PM +0800, Yi Yang 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 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--

Comments

Ben Pfaff Aug. 1, 2017, 11:14 p.m. UTC | #1 
An action has an embedded length.  For nx_action_encap, if the embedded
length ->len is longer than sizeof(struct nx_action_encap), then
properties follow struct nx_action_encap until the length has been
exhausted.

On Tue, Aug 01, 2017 at 11:06:21PM +0000, Yang, Yi Y wrote:
> About why we need n_props in nx_action_encap, I added this for Opendaylight to deserialize the wire format from OVS, n_props can clearly tell Opendaylight if there is any property. Otherwise how do we check if there is a property following?
> 
> -----Original Message-----
> From: Ben Pfaff [mailto:blp@ovn.org] 
> Sent: Tuesday, August 1, 2017 11:56 PM
> To: Yang, Yi Y <yi.y.yang@intel.com>
> Cc: dev@openvswitch.org; Jan Scheurich <jan.scheurich@ericsson.com>; Zoltan Balogh <zoltan.balogh@ericsson.com>
> Subject: Re: [PATCH v4 1/2] OF support and translation of generic encap and decap
> 
> On Tue, Aug 01, 2017 at 08:06:59PM +0800, Yi Yang 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 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:
diff mbox

Patch

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: