On Fri, Aug 1, 2014 at 3:19 PM, Lori Jakab <loja...@cisco.com> wrote:
> On 8/1/14, 3:25 AM, Jesse Gross wrote:
>>
>> On Thu, Jul 31, 2014 at 5:45 AM, Lori Jakab <loja...@cisco.com> wrote:
>>>
>>> On 7/31/14, 10:09 AM, Jesse Gross wrote:
>>>>
>>>> On Fri, Jun 27, 2014 at 6:21 AM, Lorand Jakab <loja...@cisco.com> wrote:
>>>>>
>>>>> diff --git a/datapath/actions.c b/datapath/actions.c
>>>>> index cb26ad5..20c66f5 100644
>>>>> --- a/datapath/actions.c
>>>>> +++ b/datapath/actions.c
>>>>> +static int pop_eth(struct sk_buff *skb)
>>>>> +{
>>>>> + skb_pull_rcsum(skb, skb_network_offset(skb));
>>>>> + skb_reset_mac_header(skb);
>>>>> + vlan_set_tci(skb, 0);
>>>>> +
>>>>> + OVS_CB(skb)->is_layer3 = true;
>>>>
>>>> I think we should probably also set skb->mac_len to 0 here (and on
>>>> push as well).
>>>
>>>
>>> OK. I made the following changes:
>>>
>>> --- a/datapath/actions.c
>>> +++ b/datapath/actions.c
>>> @@ -245,6 +245,7 @@ static int pop_eth(struct sk_buff *skb)
>>> {
>>> skb_pull_rcsum(skb, skb_network_offset(skb));
>>> skb_reset_mac_header(skb);
>>> + skb->mac_len = 0;
>>> vlan_set_tci(skb, 0);
>>
>> I realized that setting the mac_len to zero isn't correct in the
>> presence of MPLS. I think we can just use skb_reset_mac_len() here as
>> well.
>>
>> This also means that we shouldn't pull up to the network offset since
>> this will also pull off any MPLS labels, which I don't think that we
>> want.
>>
>> Going further, what happens if packet comes in with multiple Ethernet
>> headers on it (such as PBB)? This will pull off all of the headers but
>> that doesn't seem right. If we were to only pull off one, what are the
>> semantics with respect to VLANs?
>
>
> I have a feeling we may need to rename this action, since the my intention
> actually was to remove everything up to the layer 3 header. That includes
> MPLS header(s) and any number of Ethernet headers. The reason is, I need an
> action that guarantees that the resulting packet is layer 3, and can be sent
> through a layer 3 port. I think you're right that the expectation from a
> pop_eth() action would be what you describe above: if more then one Ethernet
> header is present, remove the outer header only, keep MPLS headers.
>
> How about renaming this action to extract_l3_packet() or similar?
I'm not excited about this. Thomas Morin was already talking about
using this code to do MPLS over GRE, which would need exactly the
semantics of popping an Ethernet header. I would like to find a way to
do this now, otherwise it is likely that we will have to solve the
same problem with another new action very soon.
>>>>> +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);
>>>>> +
>>>>> + ether_addr_copy(eth_hdr(skb)->h_source,
>>>>> ethh->addresses.eth_src);
>>>>> + ether_addr_copy(eth_hdr(skb)->h_dest, ethh->addresses.eth_dst);
>>>>> +
>>>>> + eth_hdr(skb)->h_proto = ethh->eth_type;
>>>>> + skb->protocol = ethh->eth_type;
>>>>> +
>>>>> + ovs_skb_postpush_rcsum(skb, skb->data, ETH_HLEN);
>>>>> +
>>>>> + OVS_CB(skb)->is_layer3 = false;
>>>>
>>>> What happens if there is something in skb->vlan_tci? I think the
>>>> effect will be that it still on the outer Ethernet header, which
>>>> doesn't seem correct.
>>>
>>>
>>> I missed that. Addressed with:
>>>
>>> --- 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.
>>>>> diff --git a/datapath/datapath.h b/datapath/datapath.h
>>>>> index fcd8e86..07ae0c8 100644
>>>>> --- a/datapath/datapath.h
>>>>> +++ b/datapath/datapath.h
>>>>> @@ -107,6 +107,7 @@ struct ovs_skb_cb {
>>>>> struct sw_flow_key *pkt_key;
>>>>> struct ovs_tunnel_info *tun_info;
>>>>> struct vport *input_vport;
>>>>> + bool is_layer3;
>>>>
>>>> We have run into a problem with ovs_skb_cb: it is currently full on
>>>> kernels < 3.11 due to compatibility code.
>>>
>>>
>>> What can we do about it?
>>
>> It may actually be possible to remove 'flow' from ovs_skb_cb. It is
>> really only used in ovs_execute_actions() and it should be easy enough
>> to pass in the actions as an argument there.
>
>
> Should I propose a separate patch to do this?
Sure.
>>>>> @@ -600,8 +601,10 @@ static int ovs_key_from_nlattrs(struct
>>>>> sw_flow_match
>>>>> *match, u64 attrs,
>>>>> eth_key->eth_src, ETH_ALEN, is_mask);
>>>>> SW_FLOW_KEY_MEMCPY(match, eth.dst,
>>>>> eth_key->eth_dst, ETH_ALEN, is_mask);
>>>>> + SW_FLOW_KEY_PUT(match, phy.noeth, false, is_mask);
>>>>> attrs &= ~(1ULL << OVS_KEY_ATTR_ETHERNET);
>>>>> - }
>>>>> + } else if (!is_mask)
>>>>> + SW_FLOW_KEY_PUT(match, phy.noeth, true, is_mask);
>>>>
>>>> This will never set a mask for flows without an Ethernet header, which
>>>> means that an IP flow will match things both with and without a
>>>> header. Is that the intention? (It seems to me that it would be better
>>>> to force a match on whatever form we are using - this could result in
>>>> additional flow setups for mixed traffic but it seems unlikely that
>>>> this wouldn't be forced already by something else, like the port
>>>> number.)
>>>
>>>
>>> I removed the "if".
>>
>> I still have a question to some degree. If I have a packet that came
>> in on a physical Ethernet interface and I wildcard the addresses,
>
>
> You mean the src/dst MAC addresses?
Yes.
>> should the resulting flow match a packet that came in on a pure L3
>> port? It conceivable could but are there any issues that might come
>> up? (in particular, are there any issues with multiple Ethernet
>> headers, etc.?)
>
>
> I think if the flow key has wildcarded Ethernet addresses, the flow should
> match only on L2 packets.
In that case, I think we need to always unwildcard 'noeth' rather than
making it conditional on whether you have a match on the MAC
addresses.
> 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)? Normally we reject things that can never make sense.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
