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.
> 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).
>> > @@ -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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
