Ben, because we're considering to cover NSH md type 2 case, for NSH TLV, now we provide it by the below way.
encap(hdr=nsh,prop(class=nsh,type=md_type,val=2),prop(class=nsh,type=tlv,val(0x1000,10,0x12345678)),prop(class=nsh,type=tlv,val(0x2000,20,0xfedcba9876543210))) Can you help provide a format you prefer to handle this use case? -----Original Message----- From: Ben Pfaff [mailto:b...@ovn.org] Sent: Tuesday, August 1, 2017 11:56 PM To: Yang, Yi Y <yi.y.y...@intel.com> Cc: d...@openvswitch.org; Jan Scheurich <jan.scheur...@ericsson.com>; Zoltan Balogh <zoltan.bal...@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.scheur...@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.scheur...@ericsson.com> > Signed-off-by: Yi Yang <yi.y.y...@intel.com> > Signed-off-by: Zoltan Balogh <zoltan.bal...@ericsson.com> > Co-authored-by: Zoltan Balogh <zoltan.bal...@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: _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev