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?
>>> +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).
>>> 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.
>>> @@ -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,
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.?)
Somewhat related, what happens if someone specifies just an EtherType
but no Ethernet addresses? Maybe we should just reject this?
The other changes look OK to me.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
