Thanks for the confirmation Yi. I will post the fix straight away. The other fix for double encap() is also ready.
BR, Jan > -----Original Message----- > From: Yang, Yi [mailto:[email protected]] > Sent: Monday, 26 March, 2018 03:42 > To: Jan Scheurich <[email protected]> > Cc: [email protected]; Zoltán Balogh <[email protected]> > Subject: Re: [ovs-dev] OVS will hit an assert if encap(nsh) is done in bucket > of group > > I tried the below fix patch you mentioned, it did fix this issue. > > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c > index db85716..87797bc 100644 > --- a/lib/ofp-actions.c > +++ b/lib/ofp-actions.c > @@ -6985,9 +6985,6 @@ ofpact_is_set_or_move_action(const struct ofpact *a) > case OFPACT_SET_TUNNEL: > case OFPACT_SET_VLAN_PCP: > case OFPACT_SET_VLAN_VID: > - case OFPACT_ENCAP: > - case OFPACT_DECAP: > - case OFPACT_DEC_NSH_TTL: > return true; > case OFPACT_BUNDLE: > case OFPACT_CLEAR_ACTIONS: > @@ -7025,6 +7022,9 @@ ofpact_is_set_or_move_action(const struct ofpact *a) > case OFPACT_WRITE_METADATA: > case OFPACT_DEBUG_RECIRC: > case OFPACT_DEBUG_SLOW: > + case OFPACT_ENCAP: > + case OFPACT_DECAP: > + case OFPACT_DEC_NSH_TTL: > return false; > default: > OVS_NOT_REACHED(); > > On Mon, Mar 26, 2018 at 12:45:46AM +0000, Yang, Yi Y wrote: > > Jan, thank you so much, very exhaustive analysis :), I'll double check your > > fix patch. > > > > From: Jan Scheurich [mailto:[email protected]] > > Sent: Sunday, March 25, 2018 9:09 AM > > To: Yang, Yi Y <[email protected]> > > Cc: [email protected]; Zoltán Balogh <[email protected]> > > Subject: RE: OVS will hit an assert if encap(nsh) is done in bucket of group > > > > > > Hi Yi, > > > > > > > > Part of the seemingly strange behavior of the encap(nsh) action in a group > > is caused by the (often forgotten) fact that group buckets > do not contain action *lists* but action *sets*. I have no idea why it was > defined like this when groups were first introduced in > OpenFlow 1.1. In my view it was a bad decision and causes a lot of limitation > for using groups. But that's the way it is. > > > > > > > > In action sets there can only be one action of a kind (except for > > set_field, where there can be one action per target field). If there > are multiple actions of the same kind specified, only the last one taken, the > earlier ones ignored. > > > > > > > > Furthermore, the order of execution of the actions in the action set is not > > given by the order in which they are listed but defined by > the OpenFlow standard (see chapter 5.6 of OF spec 1.5.1). Of course the > generic encap() and decap() actions are not standardized yet, > so the OF spec doesn't specify where to put them in the sequence. We had to > implement something that follows the spirit of the > specification, knowing that whatever we chose may fit some but won't fit many > other legitimate use cases. > > > > > > > > OVS's order is defined in ofpacts_execute_action_set() in ofp-actions.c: > > > > OFPACT_STRIP_VLAN > > > > OFPACT_POP_MPLS > > > > OFPACT_DECAP > > > > OFPACT_ENCAP > > > > OFPACT_PUSH_MPLS > > > > OFPACT_PUSH_VLAN > > > > OFPACT_DEC_TTL > > > > OFPACT_DEC_MPLS_TTL > > > > OFPACT_DEC_NSH_TTL > > > > All OFP_ACT SET_FIELD and OFP_ACT_MOVE (target) > > > > OFPACT_SET_QUEUE > > > > > > > > Now, your specific group bucket use case: > > > > > > > > encap(nsh),set_field:<val>->nsh_xxx,output:vxlan_gpe_port > > > > > > > > should be a lucky fit and execute as expected, whereas the analogous use > > case > > > > > > > > encap(nsh),set_field:<val>->nsh_xxx,encap(ethernet), output:ethernet_port > > > > > > > > fails with the error > > > > > > > > Dropping packet as encap(ethernet) is not supported for packet type > > ethernet. > > > > > > > > because the second encap(ethernet) action replaces the encap(nsh) in the > > action set and is executed first on the original received > Ethernet packet. Boom! > > > > > > > > So, why does your valid use case cause an assertion failure? It's a > > consequence of two faults: > > > > > > 1. In the conversion of the group bucket's action list to the bucket > > action set in ofpacts_execute_action_set() the action list is > filtered with ofpact_is_set_or_move_action() to select the set_field actions. > This function incorrectly flagged OFPACT_ENCAP, > OFPACT_DECAP and OFPACT_DEC_NSH_TTL as set_field actions. That's why the > encap(nsh) action is wrongly copied twice to the > action set. > > > > 2. The translation of the second encap(nsh) action in the action set > > doesn't change the packet_type as it is already (1,0x894f). > Hence, the commit_packet_type_change() triggered at output to vxlan_gpe port > misses to generate a second encap_nsh datapath > action. The logic here is obviously not complete to cover the NSH in NSH use > case that we intended to support and must be enhanced. > > > > > > The commit of the changes to the NSH header in commit_set_nsh_action() then > > triggers assertion failure because the translation of > the second encap(nsh) action did overwrite the original nsh_np (0x3 for > Ethernet in NSH) in the flow with 0x4 (for NSH in NSH). Since it > is not allowed to modify the nsh_np with set_field this is what triggers the > assertion. > > > > > > I believe this assertion to be correct. It did detect the combination of > > the above two faults. > > > > > > > > The solution to 1 is trivial. I'll post a bug fix straight away. That > > should suffice for your problem. > > > > The solution to 2 requires a bit more thinking. I will send a fix when I > > have found it. > > > > > > > > BR, Jan _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
