[ovs-dev] [mpls v2 1/2] Implement OpenFlow support for MPLS, for up to 3 labels.

[Simon Horman]

On Tue, Jan 07, 2014 at 09:30:54PM -0500, Jesse Gross wrote:
> On Mon, Jan 6, 2014 at 10:53 PM, Simon Horman <horms at verge.net.au> wrote:
> > On Thu, Jan 02, 2014 at 11:51:15AM -0500, Jesse Gross wrote:
> >> On Sun, Dec 29, 2013 at 2:50 AM, Ben Pfaff <blp at nicira.com> wrote:
> >> > I've been a little frustrated with the current approach to MPLS, because it
> >> > seems quite difficult to understand.  One particularly difficult bit for
> >> > me is the variables used during translation, e.g. mpls_depth_delta and
> >> > pre_push_mpls_lse.  And what we end up with is support for a single MPLS
> >> > label, which I don't think is going to make any real-world users happy.
> >> >
> >> > This commit attempts to implement something easier to understand and more
> >> > powerful by just keeping track of all the labels in struct flow.
> >> >
> >> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> >> > Co-authored-by: Simon Horman <horms at verge.net.au>
> >> > Signed-off-by: Simon Horman <horms at verge.net.au>
> >>
> >> I have some general comments:
> >>
> >> * I'm not sure that flow_count_mpls_labels() is megaflow-safe since it
> >> reads fields without updating the wildcards. I think the only real way
> >> to handle this is with automatic tracking.
> >> * It seems like we might want a harder failure mechanism for cases
> >> where we exceed the number of labels that we are able to handle. I
> >> don't think that this is the same as trying to change L4 ports when
> >> you don't have an L4 header because an MPLS packet could still be
> >> interpreted, just differently.
> >
> > Are you specifically referring to pushing and popping MPLS labels when
> > there are more labels present than we can handle? If so, do you think it
> > would be appropriate to send a controller action and stop processing
> > actions for the current table, the behaviour that occurs when an action
> > tries to decrement the IP TTL when it is already one or less.
> 
> That is the situation I was talking about.
> 
> I guess I was actually just thinking about dropping packets in this
> case. This seems a little different from an invalid TTL because the
> problem is with the flows the controller installed, not the network
> traffic. I think ideally we would just reject the flow when it is
> installed but in practice I don't think we can really validate it that
> precisely.

I am comfortable with the idea of simply dropping the packet
in the case where the flow wasn't rejected for some reason.

> >> Other things, assuming that the goal is to eventually hook this up to
> >> the kernel patches:
> >>
> >> * How do we handle cases where userspace supports parsing more labels
> >> than the kernel (which is actually true at the moment)? I think that
> >> it would just fail at flow installation time right now.
> >> * Somewhat related to the above, the kernel's action validator doesn't
> >> trust userspace to provide a valid EtherType for pop actions, so it
> >> doesn't allow multiple consecutive pops in cases where there are more
> >> tags than it knows about (which is 1).
> >
> > Prior to Ben publishing this patch my assumption was that such cases
> > would always be handled by recirculation. Which I believe implies
> > that the kernel would trust itself to extract the flow from a packet
> > after a pop, based on the ether type included in the pop action.
> >
> > Furthermore, the same assumption was made to allow poping to
> > IP and then allowing IP actions to occur after recirculation.
> >
> > Perhaps that assumption was invalid too.
> >
> > I may be missing the point but I'm a little unsure how much can occur after
> > an mpls pop action if the kernel doesn't trust the ethertype supplied in
> > the action.
> 
> I think if you use recirculation then it's fine to "trust" userspace's
> EtherType because the packet will be sent through ovs_flow_extract()
> again, which does careful validation so there isn't much trust
> involved. This is definitely the case where you pop off an MPLS header
> to reveal an IP packet.

Thanks, that puts my mind at ease.

> I was talking about the case where there is a single pass and you have
> multiple MPLS pop actions. This previously wasn't possible because
> userspace only supported a single layer of MPLS and therefore couldn't
> issue such actions. However, with this patch it could potentially do
> that and the action validator that you wrote would reject the flow
> because there is no further checking of the underlying protocol.

I guess a simple solution would be to defer such cases to recirculation.
That was, as I understand things, the plan before this patch came along.