On Mon, Mar 18, 2013 at 05:45:52PM -0700, Jesse Gross wrote:
> On Mon, Mar 18, 2013 at 12:37 AM, Simon Horman <ho...@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 <rke...@gmail.com>
> > Cc: Leo Alterman <lalter...@nicira.com>
> > Reviewed-by: Isaku Yamahata <yamah...@valinux.co.jp>
> > Signed-off-by: Simon Horman <ho...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
