[ovs-dev] OVS will hit an assert if encap(nsh) is done in bucket of group

Jan Scheurich jan.scheurich at ericsson.com
Mon Mar 26 07:11:09 UTC 2018


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:yi.y.yang at intel.com]
> Sent: Monday, 26 March, 2018 03:42
> To: Jan Scheurich <jan.scheurich at ericsson.com>
> Cc: dev at openvswitch.org; Zoltán Balogh <zoltan.balogh at ericsson.com>
> 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:jan.scheurich at ericsson.com]
> > Sent: Sunday, March 25, 2018 9:09 AM
> > To: Yang, Yi Y <yi.y.yang at intel.com>
> > Cc: dev at openvswitch.org; Zoltán Balogh <zoltan.balogh at ericsson.com>
> > 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



More information about the dev mailing list