On Tue, Mar 19, 2013 at 5:31 AM, Simon Horman <ho...@verge.net.au> wrote:
> On Tue, Mar 19, 2013 at 11:14:58AM +0900, Simon Horman wrote:
>> 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.
>
> Thinking about this, I think it should be possible to resolve the problem
> by constraining the check above further.
>
> In the case of an mpls_push action there should not be a problem
> with a subsequent set action modifying mon-existent data as any
> SET actions that modify l3 or l4 should occur before an mpls_push action.
>
> In the case of an mpls_pop action recirculation should occur
> if there is a set action that modifies l3 or l3 data.
>
> So I think that validate_and_copy_actions() should be modified to disallow
> set actions after mpls_push and mpls_pop.
Yes, I think that's right (although we could allow mpls_set after
mpls_push easily enough). Probably the simplest solution is to just
set current_eth_type to 0 after an MPLS pop.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
