[ovs-dev] [PATCH v2.29] datapath: Add basic MPLS support to kernel

[Simon Horman]

On Tue, May 21, 2013 at 09:07:06AM -0700, Jesse Gross wrote:
> On Mon, May 20, 2013 at 5:55 PM, Simon Horman <horms at verge.net.au> wrote:
> > On Fri, May 17, 2013 at 04:14:56PM -0700, Jesse Gross wrote:
> >> On Fri, May 17, 2013 at 12:06 AM, Simon Horman <horms at verge.net.au> wrote:
> >> > +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);

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

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