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?
+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.
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?
@@ -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?
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.
BTW, for a packet with multiple Ethernet headers, is there already
support for matching on anything after the outer header? Does that need
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.
The other changes look OK to me.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
