diff mbox

[net-next,2/2] openvswitch: Fix skb->protocol for vlan frames.

Message ID 1479874174-75329-2-git-send-email-jarno@ovn.org
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Jarno Rajahalme Nov. 23, 2016, 4:09 a.m. UTC 
Do not set skb->protocol to be the ethertype of the L3 header, unless
the packet only has the L3 header.  For a non-hardware offloaded VLAN
frame skb->protocol needs to be one of the VLAN ethertypes.

Any VLAN offloading is undone on the OVS netlink interface.  Due to
this all VLAN packets sent to openvswitch module from userspace are
non-offloaded.

Incorrect skb->protocol value on a full-size non-offloaded VLAN skb
causes packet drop due to failing MTU check, as the VLAN header should
not be counted in when considering MTU in ovs_vport_send().

Fixes: 5108bbaddc ("openvswitch: add processing of L3 packets")
Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
---
 net/openvswitch/datapath.c |  1 -
 net/openvswitch/flow.c     | 20 +++++++++++---------
 2 files changed, 11 insertions(+), 10 deletions(-)

Comments

Jiri Benc Nov. 24, 2016, 4:10 p.m. UTC | #1 
On Tue, 22 Nov 2016 20:09:34 -0800, Jarno Rajahalme wrote:
> Do not set skb->protocol to be the ethertype of the L3 header, unless
> the packet only has the L3 header.  For a non-hardware offloaded VLAN
> frame skb->protocol needs to be one of the VLAN ethertypes.
> 
> Any VLAN offloading is undone on the OVS netlink interface.  Due to
> this all VLAN packets sent to openvswitch module from userspace are
> non-offloaded.

This is exactly why I wanted to always accelerate the vlan tag, the
same way it is done in other parts of the networking stack: to prevent
all those weird corner cases.

Looks to me this is the only real way forward.

This patch is wrong, it would leave skb->protocol as ETH_P_TEB for L2
frames received via ARPHRD_NONE interface.

 Jiri
diff mbox

Patch

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 2d4c4d3..9c62b63 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -606,7 +606,6 @@  static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
 	rcu_assign_pointer(flow->sf_acts, acts);
 	packet->priority = flow->key.phy.priority;
 	packet->mark = flow->key.phy.skb_mark;
-	packet->protocol = flow->key.eth.type;
 
 	rcu_read_lock();
 	dp = get_dp_rcu(net, ovs_header->dp_ifindex);
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 08aa926..9be9fda 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -477,12 +477,17 @@  static int parse_icmpv6(struct sk_buff *skb, struct sw_flow_key *key,
 }
 
 /**
- * key_extract - extracts a flow key from an Ethernet frame.
+ * key_extract - extracts a flow key from a packet with or without an
+ * Ethernet header.
  * @skb: sk_buff that contains the frame, with skb->data pointing to the
- * Ethernet header
+ * beginning of the packet.
  * @key: output flow key
  *
- * The caller must ensure that skb->len >= ETH_HLEN.
+ * 'key->mac_proto' must be initialized to indicate the frame type.
+ * For an L3 frame 'key->mac_proto' must equal 'MAC_PROTO_NONE', and the
+ * caller must ensure that 'skb->protocol' is set to the ethertype of the L3
+ * header.  Otherwise the presence of an Ethernet header is assumed and
+ * the caller must ensure that skb->len >= ETH_HLEN.
  *
  * Returns 0 if successful, otherwise a negative errno value.
  *
@@ -497,9 +502,6 @@  static int parse_icmpv6(struct sk_buff *skb, struct sw_flow_key *key,
  *      on output, then just past the IP header, if one is present and
  *      of a correct length, otherwise the same as skb->network_header.
  *      For other key->eth.type values it is left untouched.
- *
- *    - skb->protocol: the type of the data starting at skb->network_header.
- *      Equals to key->eth.type.
  */
 static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 {
@@ -518,6 +520,7 @@  static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 			return -EINVAL;
 
 		skb_reset_network_header(skb);
+		key->eth.type = skb->protocol;
 	} else {
 		eth = eth_hdr(skb);
 		ether_addr_copy(key->eth.src, eth->h_source);
@@ -531,15 +534,14 @@  static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 		if (unlikely(parse_vlan(skb, key)))
 			return -ENOMEM;
 
-		skb->protocol = parse_ethertype(skb);
-		if (unlikely(skb->protocol == htons(0)))
+		key->eth.type = parse_ethertype(skb);
+		if (unlikely(key->eth.type == htons(0)))
 			return -ENOMEM;
 
 		skb_reset_network_header(skb);
 		__skb_push(skb, skb->data - skb_mac_header(skb));
 	}
 	skb_reset_mac_len(skb);
-	key->eth.type = skb->protocol;
 
 	/* Network layer. */
 	if (key->eth.type == htons(ETH_P_IP)) {