[ovs-dev] [PATCH] datapath: Kernel flow metadata parsing should be less restrictive

[Ansis Atteka]

On Tue, Nov 8, 2011 at 4:02 PM, Jesse Gross <jesse at nicira.com> wrote:

> On Tue, Nov 8, 2011 at 3:11 PM, Ansis Atteka <aatteka at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20111109/3d37b76e/attachment-0003.html>