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