On Tue, Nov 8, 2011 at 4:02 PM, Jesse Gross <je...@nicira.com> wrote:
> On Tue, Nov 8, 2011 at 3:11 PM, Ansis Atteka <aatt...@nicira.com> wrote:
> > The function flow_metadata_from_nlattrs() is very restrictive
> > about the ordering and type of metadata attributes that it receives.
> > This patch will change flow_metadata_from_nlattrs() behavior by
> > ignoring attributes that it does not understand and allowing them
> > to be passed in arbitrary order.
> >
> > Issue #8167
>
> Can you please add a signed-off-by line for kernel code?
>
> Ok.
> > diff --git a/datapath/flow.c b/datapath/flow.c
> > index 2dc87ae..287816f 100644
> > --- a/datapath/flow.c
> > +++ b/datapath/flow.c
> > @@ -1166,43 +1166,36 @@ int flow_metadata_from_nlattrs(u32 *priority,
> u16 *in_port, __be64 *tun_id,
> > const struct nlattr *attr)
> > {
> > const struct nlattr *nla;
> > - u16 prev_type;
> > int rem;
> >
> > *in_port = USHRT_MAX;
> > *tun_id = 0;
> > *priority = 0;
> >
> > - prev_type = OVS_KEY_ATTR_UNSPEC;
> > nla_for_each_nested(nla, attr, rem) {
> > int type = nla_type(nla);
> >
> > if (type > OVS_KEY_ATTR_MAX || nla_len(nla) !=
> ovs_key_lens[type])
> > return -EINVAL;
>
> If the type is greater than OVS_KEY_ATTR_MAX then we should just skip
> this attribute: it's simply an unknown.
>
Ok.
> > - switch (TRANSITION(prev_type, type)) {
> > - case TRANSITION(OVS_KEY_ATTR_UNSPEC,
> OVS_KEY_ATTR_PRIORITY):
> > + switch (type) {
> > + case OVS_KEY_ATTR_PRIORITY:
> > *priority = nla_get_u32(nla);
> > break;
> >
> > - case TRANSITION(OVS_KEY_ATTR_UNSPEC,
> OVS_KEY_ATTR_TUN_ID):
> > - case TRANSITION(OVS_KEY_ATTR_PRIORITY,
> OVS_KEY_ATTR_TUN_ID):
> > + case OVS_KEY_ATTR_TUN_ID:
> > *tun_id = nla_get_be64(nla);
> > break;
> >
> > - case TRANSITION(OVS_KEY_ATTR_UNSPEC,
> OVS_KEY_ATTR_IN_PORT):
> > - case TRANSITION(OVS_KEY_ATTR_PRIORITY,
> OVS_KEY_ATTR_IN_PORT):
> > - case TRANSITION(OVS_KEY_ATTR_TUN_ID,
> OVS_KEY_ATTR_IN_PORT):
> > + case OVS_KEY_ATTR_IN_PORT:
> > if (nla_get_u32(nla) >= DP_MAX_PORTS)
> > return -EINVAL;
> > *in_port = nla_get_u32(nla);
> > break;
> >
> > default:
> > - return 0;
> > + break;
>
> If we don't hit one of the above cases then we should not break but
> keep on parsing attributes.
>
Not sure that I understood this one correctly. The only two places where we
prematurely exit from nla_for_each_nested() loop are:
1. (type > OVS_KEY_ATTR_MAX || nla_len(nla) != ovs_key_lens[type]); and
2. (nla_get_u32(nla) >= DP_MAX_PORTS)
As per your comment I will remove the first one.
Thanks,
Ansis
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
