On Tue, Jan 07, 2014 at 09:30:54PM -0500, Jesse Gross wrote: > On Mon, Jan 6, 2014 at 10:53 PM, Simon Horman <ho...@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 <b...@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 <b...@nicira.com> > >> > Co-authored-by: Simon Horman <ho...@verge.net.au> > >> > Signed-off-by: Simon Horman <ho...@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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
