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

Ben Pfaff blp at ovn.org
Wed Aug 2 15:00:16 UTC 2017


Sounds nice.  I'll take a look.

On Wed, Aug 02, 2017 at 08:14:20AM +0000, Yang, Yi Y wrote:
> Hi, Ben
> 
> I have posted v5 which included your incremental patch and encap reformat, here they are:
> 
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-August/336594.html
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-August/336595.html
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-August/336596.html
> 
> encap is reformatted as below
> 
> encap(ethernet)
> encap(nsh)
> encap(nsh(md_type=1))
> encap(nsh(md_type=2,tlv(0x1000,10,0x12345678),tlv(0x2000,20,0xfedcba9876543210)))
> 
> No property for Ethernet encap.
> 
> -----Original Message-----
> From: Ben Pfaff [mailto:blp at ovn.org] 
> Sent: Tuesday, August 1, 2017 11:56 PM
> To: Yang, Yi Y <yi.y.yang at intel.com>
> Cc: dev at openvswitch.org; Jan Scheurich <jan.scheurich at ericsson.com>; Zoltan Balogh <zoltan.balogh at 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 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:


More information about the dev mailing list