On Thu, Feb 28, 2013 at 08:33:41PM -0800, Jesse Gross wrote:
> On Thu, Feb 28, 2013 at 12:57 AM, Simon Horman <ho...@verge.net.au> wrote:
> > On Wed, Feb 27, 2013 at 07:39:02AM -0800, Jesse Gross wrote:
> >> On Tue, Feb 26, 2013 at 11:10 PM, Simon Horman <ho...@verge.net.au> wrote:
> >> > On Tue, Feb 26, 2013 at 02:49:47PM -0800, Jesse Gross wrote:
> >> >> > diff --git a/datapath/flow.c b/datapath/flow.c
> >> >> > index fad9e19..27e1920 100644
> >> >> > --- a/datapath/flow.c
> >> >> > +++ b/datapath/flow.c
> >> >> > @@ -728,6 +728,17 @@ int ovs_flow_extract(struct sk_buff *skb, u16
> >> >> > in_port, struct sw_flow_key *key,
> >> >> > memcpy(key->ipv4.arp.tha, arp->ar_tha,
> >> >> > ETH_ALEN);
> >> >> > key_len = SW_FLOW_KEY_OFFSET(ipv4.arp);
> >> >> > }
> >> >> > + } else if (eth_p_mpls(key->eth.type)) {
> >> >> > + error = check_header(skb, MPLS_HLEN);
> >> >> > + if (unlikely(error))
> >> >> > + goto out;
> >> >> > +
> >> >> > + key_len = SW_FLOW_KEY_OFFSET(mpls.top_lse);
> >> >> > + memcpy(&key->mpls.top_lse, skb_network_header(skb),
> >> >> > MPLS_HLEN);
> >> >> > +
> >> >> > + /* Update network header */
> >> >> > + skb_set_network_header(skb, skb_network_header(skb) -
> >> >> > + skb->data + MPLS_HLEN);
> >> >>
> >> >> Is it possible to actually use this network header pointer?
> >> >
> >> > It is possible to use it in ovs_flow_extract_l3_onwards() in conjunction
> >> > with the inner/outer flow changes I have in other patches (and you have
> >> > asked me to drop).
> >> >
> >> > It does seem to me that it may be incorrect if there is more than one
> >> > MPLS
> >> > LSE present in the frame.
> >> >
> >> > But I'm not sure if either of those answers relate to the point you are
> >> > making. Are there use-cases which concern you?
> >>
> >> I was just unsure of when it would be useful. It seems like if we're
> >> not going to try to act beyond the MPLS labels that we can parse then
> >> it makes the most sense to just leave the network layer unset (maybe
> >> we can use it to point to the start of the MPLS stack instead of
> >> needing a special l2 length then?).
> >
> > I think that removing the call to skb_set_network_header() you have
> > isolated above is harmless enough as I don't think it is used because
> > if frame is MPLS then no l3+ processing is done.
> >
> > I looked at making use of the network header to mark the top of
> > the MPLS stack and not adding the special l2 length, however, I believe
> > it breaks down in the situation I will now describe.
> >
> > Suppose an IP frame has two actions applied, dec_ttl and push_mpls.
> > This is valid because an IP match may be made and ovs-vswitchd
> > can install set(ip) and push_mpls actions in the datapath.
> >
> > Now, for some reason that I must confess that I don't fully understand
> > at this moment the actions are installed as push_mpls and then set(ip).
> >
> > I believe that for the scheme you suggest above to work push_mpls (and
> > pop_mpls) need to update the network_header such that it always points to
> > the to of the MPLS stack. This is to allow multiple mpls actions to behave
> > correctly, e.g. mpls_push then mpls_set.
> >
> > The problem being that set(ip) uses the network_header and if it has been
> > set to the top of the MPLS stack by push_mpls then the set(ip) action will
> > corrupt the packet.
>
> I see. The reason why the actions are output like this is because
> userspace simply accumulates changes to the flow until they can
> possibly have an effect (when a packet is output) and then applies the
> minimal set of actions to transform the packet from its current state
> into the desired state. Since the changes up to this point have all
> been independent of each other, ordering doesn't matter and userspace
> simply creates the actions in a fixed (but arbitrary) order.
>
> I think it would be reasonable and fairly easy to enforce the
> constraint that userspace must emit actions in the correct order (IP
> then MPLS). After all, if a packet arrived as MPLS then we wouldn't
> allow a set IP action so it doesn't really make sense to support push
> MPLS and then set IP since it is effectively the same thing.
>
> Does that avoid the problem? The more that I think about it, the
> nicer it seems to use the network header pointer here.
I believe that would avoid the problem I describe above and
I'm not aware of any other problems.
I am a slightly wary of using the network header as an l2_5 header.
But the patch that I came up with (which didn't work for the reason
described above) was rather clean.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
