[ovs-dev] [PATCH 2/2] datapath: Add basic MPLS support to kernel

[Simon Horman]

On Mon, Mar 18, 2013 at 05:45:52PM -0700, Jesse Gross wrote:
> On Mon, Mar 18, 2013 at 12:37 AM, Simon Horman <horms at verge.net.au> wrote:
> > Allow datapath to recognize and extract MPLS labels into flow keys
> > and execute actions which push, pop, and set labels on packets.
> >
> > Based heavily on work by Leo Alterman and Ravi K.
> >
> > Cc: Ravi K <rkerur at gmail.com>
> > Cc: Leo Alterman <lalterman at nicira.com>
> > Reviewed-by: Isaku Yamahata <yamahata at valinux.co.jp>
> > Signed-off-by: Simon Horman <horms at verge.net.au>
> 
> I got a sparse warning (although I think it is just an annotation problem):
>   CHECK   /home/jgross/openvswitch/datapath/linux/datapath.c
> /home/jgross/openvswitch/datapath/linux/datapath.c:777:42: warning:
> incorrect type in assignment (different base types)
> /home/jgross/openvswitch/datapath/linux/datapath.c:777:42:    expected
> restricted __be16 [assigned] [usertype] current_eth_type
> /home/jgross/openvswitch/datapath/linux/datapath.c:777:42:    got unsigned int

Strange. I always check sparse. Clearly not very well.

> > diff --git a/datapath/actions.c b/datapath/actions.c
> > index 0dac658..f272e8d 100644
> > --- a/datapath/actions.c
> > +++ b/datapath/actions.c
> > +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;
> > +
> > +       if (skb_cow_head(skb, MPLS_HLEN) < 0)
> > +               return -ENOMEM;
> 
> Why do we need both make_writable() and skb_cow_head()?

My mistake, I'll remove skb_cow_head().

> 
> > diff --git a/datapath/datapath.c b/datapath/datapath.c
> > index a40ff47..fa0214b 100644
> > --- a/datapath/datapath.c
> > +++ b/datapath/datapath.c
> > @@ -593,8 +594,7 @@ static int validate_set(const struct nlattr *a,
> >                 return -EINVAL;
> >
> >         if (key_type > OVS_KEY_ATTR_MAX ||
> > -           (ovs_key_lens[key_type] != nla_len(ovs_key) &&
> > -            ovs_key_lens[key_type] != -1))
> > +           ovs_flow_verify_key_len(key_type, ovs_key))
> >                 return -EINVAL;
> 
> I think we could move the check for key_type > OVS_KEY_ATTR_MAX into
> ovs_flow_verify_key_len() as well.

Sure, will do.

> I don't think that we want to allow an array of MPLS labels at this
> point in time, since we'll just silently ignore the ones that we don't
> expect, which isn't good.  However, we should define the interface in
> such a way that anticipates this extension.  For example, I don't
> think that it's good to call the struct member mpls_top_lse if it is
> potentially a stack of labels.

I'm not sure that I understand what you want the interface to look like.

Of course we can change the name of the struct member, to say mpls_lse.
But my understanding was that you simply wanted an array of these structs,
which is what I have implemented.

I could add a check to reject a flow if the number of elements is
greater than zero. That would avoid silently ignoring subsequent members
while providing an interface that allows them. But I sense that this
is not what you have in mind.

> > @@ -755,6 +763,19 @@ static int validate_and_copy_actions(const struct nlattr *attr,
> >                                 return -EINVAL;
> >                         break;
> >
> > +               case OVS_ACTION_ATTR_PUSH_MPLS: {
> > +                       const struct ovs_action_push_mpls *mpls = nla_data(a);
> > +                       if (!eth_p_mpls(mpls->mpls_ethertype))
> > +                               return -EINVAL;
> > +                       current_eth_type = mpls->mpls_ethertype;
> > +                       break;
> > +               }
> > +
> > +               case OVS_ACTION_ATTR_POP_MPLS:
> > +                       if (!eth_p_mpls(current_eth_type))
> > +                               return -EINVAL;
> > +                       current_eth_type = nla_get_u32(a);
> 
> I don't think it is safe to assume that the provided EtherType is
> correct: it's possible that the packet is not long enough to actually
> contain that protocol.  Since all length checking happens at flow
> extraction time, a later set could write off the end of the packet.

I'm curious to know why this problem doesn't also exist
for other set actions. For example set ipv4.

> I think we also need to handle the sample action more robustly as
> well: effectively right now we're validating the not-taken sample path
> but we should validate the other path as well.
> 
> > @@ -1189,8 +1211,10 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, struct genl_info *info)
> >         if (!a[OVS_FLOW_ATTR_KEY])
> >                 goto error;
> >         error = ovs_flow_from_nlattrs(&key, &key_len, a[OVS_FLOW_ATTR_KEY]);
> > -       if (error)
> > +       if (error) {
> > +               printk("%s: ovs_flow_from_nlattrs failed\n", __func__);
> 
> We should try to keep debugging log messages out of the final code.

Sorry, I will remove that.