Re: [PATCH v2.58] datapath: Add basic MPLS support to kernel

On Tue, Jun 03, 2014 at 03:40:27PM -0700, Jesse Gross wrote:

quoted

On Mon, Jun 2, 2014 at 9:04 PM, Simon Horman [off-list ref] wrote:

quoted

Hi Jesse,

thanks for your feedback.

On Mon, Jun 02, 2014 at 05:58:10PM -0700, Jesse Gross wrote:

quoted

On Sun, May 25, 2014 at 5:22 PM, Simon Horman [off-list ref] wrote:

quoted

diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
index 803a94c..8ce596c 100644
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
+               case OVS_ACTION_ATTR_POP_MPLS:
+                       if (!eth_p_mpls(eth_type))
+                               return -EINVAL;

Should this also take into account the VLAN tag? It's really part of
the EtherType although it has been stripped out here. Actually, maybe
it's better to not track the vlan_tci separately at all during
validation but just fold it into the EtherType.

The practical implication of this is that you wouldn't be able to pop
out from underneath a VLAN. This may be a good thing if we are trying
to avoid tag order issues - after all, you can't push underneath a
VLAN either. I'm not sure what effects this has on the need to track
mac_len, if any.

My thinking is that the ordering problem only surfaces in relation
to push MPLS actions where should it go in relation to VLAN tags.
For pop actions it seems to me that the outermost tag should be removed
regardless of its position in relation to other tags.

So I think that the code above is safe. Though now you mention
it I do notice that it only allows pop MPLS if there is at most
one VLAN tag present.

That said, I would not mind particularly disabling pop MPLS in the
presence of VLAN tags. At the very least it is related to the
painful issue of tag ordering.

I agree that it is safe but my thought was the it avoids a number of
potential corner cases such as:
 * Difference between push and pop underneath vlan tags.
 * Pop with multiple vlan tags
 * Differences with varying EtherTypes used for vlans

quoted

I explored your idea of tracking only eth_type rather than both
it and vlan_tci. I did this by adding the following logic to
ovs_nla_copy_actions().

        if (key->eth.tci & htons(VLAN_TAG_PRESENT))
                eth_type = htons(ETH_P_8021Q);
        else
                eth_type = key->eth.type;

I then updated the usage of eth_type in ovs_nla_copy_actions__() accordingly.
One problem that I have run into is what to do about pop VLAN.

I don't believe its possible to know what the new eth type is.
This makes subsequent checks of the eth type for validate_set()
a little awkward. And seems to indicate that an extra parameter would
be needed.

For this reason I am inclined to leave the eth_type and vlan_tci
parameters in place. In this case there is no problem with pop VLAN
as the ether type inside the VLAN tag should be the value of eth_type.

Can't we populate eth_type with the EtherType from the flow key in
pop_vlan? This doesn't provide us with the ability to look arbitrarily
deep into the packet but it should at least retain the functionality
that we have today.

I think that things are still a bit tricky for at least two reasons.

1. Tracking eth_type if there are more push VLAN actions than pop VLAN actions

   e.g.: A packet with no VLAN tags with the following
         actions applied: push VLAN, push VLAN, pop VLAN

   I believe that with the change your propose eth_type would end up being
   that of the original packet than of the packet with one VLAN tag removed
   after two are added.

   I think would allow a push MPLS action but it should be rejected.

   I think this could be resolved by setting eth_type to a bogus value
   (e.g. 0) but that would may cause subsequent calls to validate_set()
   to fail when they should pass.

2. The use of eth_type in validate_set().

   Suppose we have an IPv6 packet with one VLAN.

   validate_set() allows an IPv6 set operation if the eth_type is IPv6.
   Which would be true if the eth_type is that of the inner rather
   than the outer packet. However, I believe with the scheme
   you are proposing this becomes a bit tricky as the eth_type will
   be that of the outer packet.

   I suppose this particular problem could be resolved using something like
   this in validate_set():

        if (eth_type == htons(ETH_P_8021Q))
                eth_type = flow_key.eth_type;

   But that may get a bit tricky. For example suppose there is
   an IPv6 packet with no VLAN tags and the following actions are applied.

   push MPLS, push VLAN, set IPv6

   validate_set() would be passed:
   eth_type: htons(ETH_P_8021Q)
   flow_key.eth_type: htons(ETH_P_IPV6)

   But validate_set should be checking against an MPLS ethtype.

   I think this particular problem could be resolved with a new
   restriction, prohibiting push VLAN in the presence of MPLS.

My general feeling is that tracking both eth_type and vlan_tci gives
us a richer set of information to work with for various cases the
code handles.

I would like to propose leaving the eth_type and vlan_tci scheme in place
and using something like the following (untested) to address your concern
about pop MPLS.

diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
index 8ce596c..bcd05b3 100644
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c

@@ -1571,7 +1571,8 @@ static int ovs_nla_copy_actions__(const struct nlattr *attr,
                }

                case OVS_ACTION_ATTR_POP_MPLS:
-                       if (!eth_p_mpls(eth_type))
+                       if (vlan_tci & htons(VLAN_TAG_PRESENT) ||
+                           !eth_p_mpls(eth_type))
                                return -EINVAL;

                        /* Disallow subsequent L2.5+ set and mpls_pop actions