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

[Jesse Gross]

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

> 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()?

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

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.

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