[ovs-dev] [PATCH v4 3/3] datapath: add layer 3 flow/port support

Jesse Gross jesse at nicira.com
Mon Aug 25 00:33:13 UTC 2014

On Thu, Aug 21, 2014 at 12:24 PM, Lori Jakab <lojakab at cisco.com> wrote:
> On 8/6/14 4:37 AM, Jesse Gross wrote:
>> Besides the fact that it would be nice to unify things, I'm not sure
>> that it is actually correct to pull off VLAN, MPLS, etc. tags that we
>> don't understand. Presumably, these are being used to create some kind
>> of isolation and dropping all the tags that we don't understand would
>> just blindly merge things together. Also, from an implementation
>> perspective, we don't really parse beyond what we understand (this is
>> not quite true for MPLS but it is for VLANs and any unknown protocols)
>> so we wouldn't necessarily actually get to an L3 header. On the other
>> hand, if we only deal with tags that we do understand then can't we
>> have more atomic instructions that pull them off one-by-one?
> Atomic instructions make a lot of sense, and we definitely should support
> them.  However, if a packet is sent to an L3 port, it should not have any
> Ethernet, VLAN or MPLS header. Note that for now these actions are not user
> visible, and cannot be added with OpenFlow, they are added automatically by
> the user space code, when a packet is sent from an L2->L3 port or L3->L2
> port.
> I agreed with Ben that this behavior will be changed in the future, based on
> discussions in the ONF's EXT-112 proposal, where the consensus was to have
> explicit push_eth and pop_eth OpenFlow actions.  However, the current
> behavior of the LISP code is to add/remove L2 headers automatically, so we
> want to keep that in the first phase, until I implement OpenFlow support.
> Once automatic header popping is not used, we can remove the
> "extract_l3_packet()" action.
> From the implementation perspective, this action would rely on the
> skb->network_header being set correctly.

I'm not sure that this really addresses the concerns that I have from
both policy and implementation perspectives.

>From the policy perspective: Will a Cisco implementation of LISP
accept a packet on an unknown VLAN, throw away the tag when it does L2
processing, and then continue on to do L3 processing and LISP? Somehow
I doubt it.

>From an implementation perspective: There is no mechanism to rely on
skb->network_header being set in the general case. It will only be set
in situations where OVS can parse the packet. OVS doesn't know about
QinQ for example so you the action that you have defined won't
actually give you an L3 packet.

Finally, from a Linux perspective, removing kernel interfaces isn't
something that is OK as a matter of policy.

The fact that the OVS kernel module doesn't know how to inherently
find the L3 is not as bad as it seems. Conveniently, it means that we
can pop off exactly the same headers as we know how to parse so there
is no loss of functionality to doing this individually. As bonus,
since userspace needs to generate these actions, the policy can be
enforced there as well.

>>>>>>> --- a/datapath/actions.c
>>>>>>> +++ b/datapath/actions.c
>>>>>>> @@ -258,6 +258,7 @@ static void push_eth(struct sk_buff *skb, const
>>>>>>> struct
>>>>>>> ovs_action_push_eth *ethh
>>>>>>>            skb_push(skb, ETH_HLEN);
>>>>>>>            skb_reset_mac_header(skb);
>>>>>>>            skb_reset_mac_len(skb);
>>>>>>> +       vlan_set_tci(skb, 0);
>>>>>> I don't think it's right to throw away the vlan tag. My assumption is
>>>>>> that it should be placed into the packet before we push on the
>>>>>> Ethernet header (this actually should never happen since we disallow
>>>>>> pushing multiple Ethernet headers but it still seems like the right
>>>>>> thing to do).
>>>>> That's not what I would expect from a simple push_eth() action.  If
>>>>> someone
>>>>> wanted to push a VLAN tag, that should be an explicit action.  So I
>>>>> would
>>>>> just throw away whatever there is in skb->vlan_tci.
>>>> The contents of skb->vlan_tci were already pushed explicitly (look at
>>>> the implementation of push_vlan()). From a user's perspective, there
>>>> is no difference - it is just an internal optimization to pull the tag
>>>> out of the packet, so I can't see any justification for throwing away
>>>> that tag.
>>> OK.  I will remove that line.
>> But just to be clear, I think we still need to explicitly push the
>> vlan tag on the packet. Otherwise, it will migrate from the inner
>> Ethernet header to the outer.
> Still not sure what I need to do here.  If skb->vlan_tci was set by a
> push_vlan action, then the vlan tag should already have been pushed on the
> packet, right?

Yes, the user has requested that a vlan tag be applied. skb->vlan_tci
is a form of offloading that should be transparent and will no longer
work in this case, so it needs to be de-offloaded. Please take a look
at the OVS vlan operations and other vlan code in the kernel if this
isn't clear.

>>>>> BTW, for a packet with multiple Ethernet headers, is there already
>>>>> support
>>>>> for matching on anything after the outer header?  Does that need
>>>>> recirculation?
>>>> There isn't currently any support on matching on multiple headers but
>>>> it seems like a natural possibility if we introduce the capability to
>>>> push/pop Ethernet headers. Once those are there, it seems like you
>>>> could do it with recirculation.
>>>>>> Somewhat related, what happens if someone specifies just an EtherType
>>>>>> but no Ethernet addresses? Maybe we should just reject this?
>>>>> In a flow key?  I don't have a strong opinion on this, but I think I
>>>>> wouldn't reject it.
>>>> Is there any situation where it could be used (what meaning would it
>>>> have)?
>>> Not that I can think of right now.
>>>> Normally we reject things that can never make sense.
>>> If that's the policy, I'll change the implementation.
>> OK, I think that probably makes sense (we can loosen it later if we
>> come up with a use case and add support for it).
> I just want to note that the push_eth action has a single struct with all
> the data.  So the netlink message will always have the Ethertype AND the
> Ethernet addresses.  The only thing I can think of validating here is if the
> Ethernet addresses are not all zero, but I think we should allow even that.

I'm not sure that I understand this comment. When pushing on a new
Ethernet header, wouldn't you expect that both the addresses and the
EtherType is present?

Before I was talking about validating the match, not the action part.

More information about the dev mailing list