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: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