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

On Tue, May 21, 2013 at 09:07:06AM -0700, Jesse Gross wrote:

On Mon, May 20, 2013 at 5:55 PM, Simon Horman [off-list ref] wrote:

quoted

On Fri, May 17, 2013 at 04:14:56PM -0700, Jesse Gross wrote:

quoted

On Fri, May 17, 2013 at 12:06 AM, Simon Horman [off-list ref] wrote:

quoted

+static int push_mpls(struct sk_buff *skb,
+                    const struct ovs_action_push_mpls *mpls)
+{
+       __be32 *new_mpls_lse;
+       int err;
+
+       err = make_writable(skb, skb->mac_len + MPLS_HLEN);
+       if (unlikely(err))
+               return err;
+
+       skb_push(skb, MPLS_HLEN);

What happens if there isn't enough headroom?

Good point. How about this?

        if (unlikely(skb->data<skb->head))
                return -EINVAL;
        skb_push(skb, MPLS_HLEN);

The amount of headroom is an internal kernel property so we can't
reject actions on the basis of it. We need to expand the headroom,
similar to __vlan_put_tag().

Thanks. I have changed the code around and it is now as follows.
I am a little unsure if the combination of make_writable() and
skb_cow_head() is sensible.

	err = make_writable(skb, skb->mac_len + MPLS_HLEN);
	if (unlikely(err))
		return err;

	if (skb_cow_head(skb, MPLS_HLEN) < 0)
		return -ENOMEM;

	skb_push(skb, MPLS_HLEN);

quoted

quoted

quoted

diff --git a/datapath/datapath.c b/datapath/datapath.c
index 42af315..3a1c203 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c

It's not clear to that using the depth is actually sufficient to
capture all possible combinations because the more common case is that
actions are a list, not nested. For example, consider the following
(invalid) action list on an IP input packet:

push_mpls, sample(pop_mpls), pop_mpls

I don't believe that this will be rejected since by the time we get to
the second pop_mpls we will have forgotten about the sample action.

My intention when writing the code was that such a case would be rejected
as follows:

1. When the nested pop_mpls is validated the following call is
   made with depth = 1:

  eth_types_set(eth_types, depth, htons(0));

  This sets:
        eth_types->depth = 1
        eth_types[1] = htons(0);

2. When the second pop_mpls is validated the following
   is executed (with the fix that you suggested below):

   for (i = 0; i < eth_types->depth; i++)
        if (!eth_p_mpls(eth_types->types[i]))
                return -EINVAL;

   This will return -EINVAL due to the values of eth_type members set in 1.

OK, I see that the depth does't get reset for the next check. However,
what about this sequence?

push_mpls, sample(pop_mpls), sample(push_mpls), pop_mpls

My point is that in cases where sample actions aren't nested, the
depth doesn't capture all the combinations.

Thanks, I see your point. And I agree that the code does not seem
to handle that case correctly. I will work on this.

quoted

quoted

quoted

diff --git a/datapath/flow.c b/datapath/flow.c
index 3ce926e..2a86f90 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c

@@ -848,6 +880,9 @@ const int ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {
        [OVS_KEY_ATTR_ARP] = sizeof(struct ovs_key_arp),
        [OVS_KEY_ATTR_ND] = sizeof(struct ovs_key_nd),
        [OVS_KEY_ATTR_TUNNEL] = -1,
+
+       /* Not upstream. */
+       [OVS_KEY_ATTR_MPLS] = sizeof(struct ovs_key_mpls),

At this point, we can probably treat this patch as the point where the
MPLS data structures are finalized and upstreamable. Therefore, this
patch can move the attribute to a final location.

Thanks, I will squash the following incremental change into the next
posting of this patch.

diff --git a/datapath/flow.c b/datapath/flow.c
index 2a86f90..d67d6bf 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c

@@ -880,8 +880,6 @@ const int ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {
        [OVS_KEY_ATTR_ARP] = sizeof(struct ovs_key_arp),
        [OVS_KEY_ATTR_ND] = sizeof(struct ovs_key_nd),
        [OVS_KEY_ATTR_TUNNEL] = -1,
-
-       /* Not upstream. */
        [OVS_KEY_ATTR_MPLS] = sizeof(struct ovs_key_mpls),
 };

diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
index e890fd8..d90f585 100644
--- a/include/linux/openvswitch.h
+++ b/include/linux/openvswitch.h

@@ -287,7 +287,7 @@ enum ovs_key_attr {
        OVS_KEY_ATTR_IPV4_TUNNEL,  /* struct ovs_key_ipv4_tunnel */
 #endif

-       OVS_KEY_ATTR_MPLS = 62, /* array of struct ovs_key_mpls.
+       OVS_KEY_ATTR_MPLS,      /* array of struct ovs_key_mpls.
                                 * The implementation may restrict
                                 * the accepted length of the array. */
        __OVS_KEY_ATTR_MAX

I think this belongs more appropriately directly after OVS_KEY_ATTR_TUNNEL.

Sure, I will move it.