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

Jesse Gross jesse at nicira.com
Wed Jan 8 02:30:54 UTC 2014


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.

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

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.



More information about the dev mailing list