On Mon, Mar 18, 2013 at 06:35:48PM -0700, Jesse Gross wrote:
> On Mon, Mar 18, 2013 at 6:22 PM, Simon Horman <ho...@verge.net.au> wrote:
> > On Mon, Mar 18, 2013 at 05:45:52PM -0700, Jesse Gross wrote:
> >> 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.
>
> It's not really a code change (I don't think it would even break the
> ABI if we made the change in the future, only the API). I just think
> that we should write include/linux/openvswitch.h as if we supported
> multiple layers but then restrict it in the implementation. So just
> something like this:
>
> struct ovs_key_mpls {
> __be32 mpls_lse[];
> };
>
> plus appropriate comments.
Thanks, I understand.
>
> > 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.
>
> That actually is what I have in mind (assuming that you mean rejet if
> number of elements is greater than 1).
Sorry for my typo, yes I meant 1 :)
I'll make it so.
> >> > @@ -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.
>
> No other action allows anything that would affect other layers to be
> changed - for example, set IPv4 doesn't allow the next protocol to be
> changed. Therefore, the validation that has already been performed by
> ovs_flow_extract() is still valid.
Thanks. I'll have a think about how to fix this.
But I wonder if it needs to be handled at the time that actions are executed.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
