[ovs-dev] [PATCH 1/2] mpls: Allow l3 and l4 actions to prior to a push_mpls action

Simon Horman horms at verge.net.au
Fri Mar 22 00:41:30 UTC 2013


On Wed, Mar 20, 2013 at 03:31:51PM -0700, Jesse Gross wrote:
> On Wed, Mar 20, 2013 at 6:18 AM, Simon Horman <horms at verge.net.au> wrote:
> > * Update the order in which actions are committed and thus
> >   appear in the datapath such that MPLS actions appear after
> >   l3 and l4 (nw and port) actions.
> >
> >   In the case where an mpls_push action is present it should ensure
> >   that l3 and l4 actions occur first, which seems logical as
> >   once a mpls_push has occur the frame will be MPLS rather
> >   than IPv4 or IPv6.
> >
> >   In the case where there is an mpls_pop action is present this should
> >   not make any difference as the frame will have been MPLS to start with
> >   and thus not satisfy the pre-requisites for  l3 or l4 actions.
> >
> > * Update commit_set_nw_action() to use the base ethertype when considering
> >   eligibility to commit l3 (nw) actions. This allows l3 actions to be
> >   applied so long as the frame was originally IPv4 or IPv6, even if
> >   an mpls_push action will be applied and thus flow indicates the
> >   frame will be MPLS.
> >
> > * Make actions that may modify port or network information conditional on
> >   the flow's ethernet type being an IP ethernet type. This is to ensure
> >   that actions that modify network and port information do not occur
> >   on non IP packets, for example if an mpls_push action has changed a
> >   packet from IP to MPLS.
> >
> >   Note that modification of network data is already prevented by
> >   virtue of commit_set_nw_action() only having cases for when the
> >   ethernet type of the flow is  IPV4 or IPV6. The conditionality
> >   of network actions on the ethernet type has been added to
> >   do_xlate_actions() to make it explicit.
> >
> > Signed-off-by: Simon Horman <horms at verge.net.au>
> 
> Applied, thanks.

Thanks!

> I realized that there is one more possible case that could cause
> problems here: if something like reg_move is used to load a value into
> the transport port field without using one of the field specific
> actions.  Completely fixing this seemed like more trouble than it is
> worth for the time being, so I essentially combined the two strategies
> - the one you have here to give nice behavior in the common case and
> blocking it at the commit_set_port_action() level to provide final
> protection.  The idea that you had earlier of just checking that the
> base flow is IP seemed cleanest, so I just did that.

Thanks, that sounds reasonable to me.

I think that the way that actions are marshalled by execute_* functions
and then turned into ODP actions using commit_* has worked well for now
and serves to provide a reasonably minimal set of ODP actions. However
it is difficult to make that approach work well if the ethernet type
of the packet is allowed to change, as is the case with MPLS. So it doesn't
surprise me that we are deploying checks multiple locations in order to
ensure the ODP actions are valid.



More information about the dev mailing list