diff mbox series

[ovs-dev,1/2] datapath: Properly set L4 keys on "later" IP fragments

Message ID 1567036207-14322-1-git-send-email-gvrose8192@gmail.com
State Accepted
Headers show
Series [ovs-dev,1/2] datapath: Properly set L4 keys on "later" IP fragments | expand

Commit Message

Gregory Rose Aug. 28, 2019, 11:50 p.m. UTC 
Upstream commit:
    commit ad06a566e118e57b852cab5933dbbbaebb141de3
    Author: Greg Rose <gvrose8192@gmail.com>
    Date:   Tue Aug 27 07:58:09 2019 -0700

    openvswitch: Properly set L4 keys on "later" IP fragments

    When IP fragments are reassembled before being sent to conntrack, the
    key from the last fragment is used.  Unless there are reordering
    issues, the last fragment received will not contain the L4 ports, so the
    key for the reassembled datagram won't contain them.  This patch updates
    the key once we have a reassembled datagram.

    The handle_fragments() function works on L3 headers so we pull the L3/L4
    flow key update code from key_extract into a new function
    'key_extract_l3l4'.  Then we add a another new function
    ovs_flow_key_update_l3l4() and export it so that it is accessible by
    handle_fragments() for conntrack packet reassembly.

    Co-authored-by: Justin Pettit <jpettit@ovn.org>
    Signed-off-by: Greg Rose <gvrose8192@gmail.com>
    Acked-by: Pravin B Shelar <pshelar@ovn.org>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Cc: Justin Pettit <jpettit@ovn.org>
Signed-off-by: Greg Rose <gvrose8192@gmail.com>
---
 datapath/conntrack.c |   5 ++
 datapath/flow.c      | 157 +++++++++++++++++++++++++++++----------------------
 datapath/flow.h      |   1 +
 3 files changed, 95 insertions(+), 68 deletions(-)

Comments

Yi-Hung Wei Aug. 29, 2019, 12:05 a.m. UTC | #1 
On Wed, Aug 28, 2019 at 4:50 PM Greg Rose <gvrose8192@gmail.com> wrote:
>
> Upstream commit:
>     commit ad06a566e118e57b852cab5933dbbbaebb141de3
>     Author: Greg Rose <gvrose8192@gmail.com>
>     Date:   Tue Aug 27 07:58:09 2019 -0700
>
>     openvswitch: Properly set L4 keys on "later" IP fragments
>
>     When IP fragments are reassembled before being sent to conntrack, the
>     key from the last fragment is used.  Unless there are reordering
>     issues, the last fragment received will not contain the L4 ports, so the
>     key for the reassembled datagram won't contain them.  This patch updates
>     the key once we have a reassembled datagram.
>
>     The handle_fragments() function works on L3 headers so we pull the L3/L4
>     flow key update code from key_extract into a new function
>     'key_extract_l3l4'.  Then we add a another new function
>     ovs_flow_key_update_l3l4() and export it so that it is accessible by
>     handle_fragments() for conntrack packet reassembly.
>
>     Co-authored-by: Justin Pettit <jpettit@ovn.org>
>     Signed-off-by: Greg Rose <gvrose8192@gmail.com>
>     Acked-by: Pravin B Shelar <pshelar@ovn.org>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
>
> Cc: Justin Pettit <jpettit@ovn.org>
> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
> ---

Thanks for the backport.

Acked-by: Yi-Hung Wei <yihung.wei@gmail.com>
Justin Pettit Aug. 29, 2019, 12:07 a.m. UTC | #2 
> On Aug 28, 2019, at 5:05 PM, Yi-Hung Wei <yihung.wei@gmail.com> wrote:
> 
> Thanks for the backport.
> 
> Acked-by: Yi-Hung Wei <yihung.wei@gmail.com>

Thanks, Yi-Hung.  I already pushed the backports.

--Justin
Justin Pettit Aug. 29, 2019, 12:17 a.m. UTC | #3 
> On Aug 28, 2019, at 4:50 PM, Greg Rose <gvrose8192@gmail.com> wrote:
> 
> Upstream commit:
>    commit ad06a566e118e57b852cab5933dbbbaebb141de3
>    Author: Greg Rose <gvrose8192@gmail.com>
>    Date:   Tue Aug 27 07:58:09 2019 -0700
> 
>    openvswitch: Properly set L4 keys on "later" IP fragments
> 
>    When IP fragments are reassembled before being sent to conntrack, the
>    key from the last fragment is used.  Unless there are reordering
>    issues, the last fragment received will not contain the L4 ports, so the
>    key for the reassembled datagram won't contain them.  This patch updates
>    the key once we have a reassembled datagram.
> 
>    The handle_fragments() function works on L3 headers so we pull the L3/L4
>    flow key update code from key_extract into a new function
>    'key_extract_l3l4'.  Then we add a another new function
>    ovs_flow_key_update_l3l4() and export it so that it is accessible by
>    handle_fragments() for conntrack packet reassembly.
> 
>    Co-authored-by: Justin Pettit <jpettit@ovn.org>
>    Signed-off-by: Greg Rose <gvrose8192@gmail.com>
>    Acked-by: Pravin B Shelar <pshelar@ovn.org>
>    Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> Cc: Justin Pettit <jpettit@ovn.org>
> Signed-off-by: Greg Rose <gvrose8192@gmail.com>

Thanks, Greg.  I was able to cleanly apply this to master and branch-2.12.  Would you be able to provide backports to older OVS versions?  I think fragment reassembly was introduce in OVS 2.5.

Thanks!

--Justin
Gregory Rose Aug. 29, 2019, 12:29 a.m. UTC | #4 
On 8/28/2019 5:17 PM, Justin Pettit wrote:
>> On Aug 28, 2019, at 4:50 PM, Greg Rose <gvrose8192@gmail.com> wrote:
>>
>> Upstream commit:
>>     commit ad06a566e118e57b852cab5933dbbbaebb141de3
>>     Author: Greg Rose <gvrose8192@gmail.com>
>>     Date:   Tue Aug 27 07:58:09 2019 -0700
>>
>>     openvswitch: Properly set L4 keys on "later" IP fragments
>>
>>     When IP fragments are reassembled before being sent to conntrack, the
>>     key from the last fragment is used.  Unless there are reordering
>>     issues, the last fragment received will not contain the L4 ports, so the
>>     key for the reassembled datagram won't contain them.  This patch updates
>>     the key once we have a reassembled datagram.
>>
>>     The handle_fragments() function works on L3 headers so we pull the L3/L4
>>     flow key update code from key_extract into a new function
>>     'key_extract_l3l4'.  Then we add a another new function
>>     ovs_flow_key_update_l3l4() and export it so that it is accessible by
>>     handle_fragments() for conntrack packet reassembly.
>>
>>     Co-authored-by: Justin Pettit <jpettit@ovn.org>
>>     Signed-off-by: Greg Rose <gvrose8192@gmail.com>
>>     Acked-by: Pravin B Shelar <pshelar@ovn.org>
>>     Signed-off-by: David S. Miller <davem@davemloft.net>
>>
>> Cc: Justin Pettit <jpettit@ovn.org>
>> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
> Thanks, Greg.  I was able to cleanly apply this to master and branch-2.12.  Would you be able to provide backports to older OVS versions?  I think fragment reassembly was introduce in OVS 2.5.
>
> Thanks!

I can do that.

- Greg
>
> --Justin
>
>
diff mbox series

Patch

diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index 292febb..4c96d90 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -570,6 +570,11 @@  static int handle_fragments(struct net *net, struct sw_flow_key *key,
 		return -EPFNOSUPPORT;
 	}
 
+	/* The key extracted from the fragment that completed this datagram
+	 * likely didn't have an L4 header, so regenerate it.
+	 */
+	ovs_flow_key_update_l3l4(skb, key);
+
 	key->ip.frag = OVS_FRAG_TYPE_NONE;
 	skb_clear_hash(skb);
 	skb->ignore_df = 1;
diff --git a/datapath/flow.c b/datapath/flow.c
index 618c25e..dc93581 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -541,79 +541,14 @@  static int parse_nsh(struct sk_buff *skb, struct sw_flow_key *key)
 }
 
 /**
- * key_extract - extracts a flow key from an Ethernet frame.
+ * key_extract_l3l4 - extracts L3/L4 header information.
  * @skb: sk_buff that contains the frame, with skb->data pointing to the
- * Ethernet header
+ *       L3 header
  * @key: output flow key
- *
- * The caller must ensure that skb->len >= ETH_HLEN.
- *
- * Returns 0 if successful, otherwise a negative errno value.
- *
- * Initializes @skb header fields as follows:
- *
- *    - skb->mac_header: the L2 header.
- *
- *    - skb->network_header: just past the L2 header, or just past the
- *      VLAN header, to the first byte of the L2 payload.
- *
- *    - skb->transport_header: If key->eth.type is ETH_P_IP or ETH_P_IPV6
- *      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)
+static int key_extract_l3l4(struct sk_buff *skb, struct sw_flow_key *key)
 {
 	int error;
-	struct ethhdr *eth;
-
-	/* Flags are always used as part of stats */
-	key->tp.flags = 0;
-
-	skb_reset_mac_header(skb);
-
-	/* Link layer. */
-	clear_vlan(key);
-	if (ovs_key_mac_proto(key) == MAC_PROTO_NONE) {
-		if (unlikely(eth_type_vlan(skb->protocol)))
-			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);
-		ether_addr_copy(key->eth.dst, eth->h_dest);
-
-		__skb_pull(skb, 2 * ETH_ALEN);
-		/* We are going to push all headers that we pull, so no need to
-		 * update skb->csum here.
-		 */
-
-		if (unlikely(parse_vlan(skb, key)))
-			return -ENOMEM;
-
-		key->eth.type = parse_ethertype(skb);
-		if (unlikely(key->eth.type == htons(0)))
-			return -ENOMEM;
-
-		/* Multiple tagged packets need to retain TPID to satisfy
-		 * skb_vlan_pop(), which will later shift the ethertype into
-		 * skb->protocol.
-		 */
-		if (key->eth.cvlan.tci & htons(VLAN_CFI_MASK))
-			skb->protocol = key->eth.cvlan.tpid;
-		else
-			skb->protocol = key->eth.type;
-
-		skb_reset_network_header(skb);
-		__skb_push(skb, skb->data - skb_mac_header(skb));
-	}
-
-	skb_reset_mac_len(skb);
 
 	/* Network layer. */
 	if (key->eth.type == htons(ETH_P_IP)) {
@@ -814,6 +749,92 @@  static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 	return 0;
 }
 
+/**
+ * key_extract - extracts a flow key from an Ethernet frame.
+ * @skb: sk_buff that contains the frame, with skb->data pointing to the
+ * Ethernet header
+ * @key: output flow key
+ *
+ * The caller must ensure that skb->len >= ETH_HLEN.
+ *
+ * Returns 0 if successful, otherwise a negative errno value.
+ *
+ * Initializes @skb header fields as follows:
+ *
+ *    - skb->mac_header: the L2 header.
+ *
+ *    - skb->network_header: just past the L2 header, or just past the
+ *      VLAN header, to the first byte of the L2 payload.
+ *
+ *    - skb->transport_header: If key->eth.type is ETH_P_IP or ETH_P_IPV6
+ *      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)
+{
+	struct ethhdr *eth;
+
+	/* Flags are always used as part of stats */
+	key->tp.flags = 0;
+
+	skb_reset_mac_header(skb);
+
+	/* Link layer. */
+	clear_vlan(key);
+	if (ovs_key_mac_proto(key) == MAC_PROTO_NONE) {
+		if (unlikely(eth_type_vlan(skb->protocol)))
+			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);
+		ether_addr_copy(key->eth.dst, eth->h_dest);
+
+		__skb_pull(skb, 2 * ETH_ALEN);
+		/* We are going to push all headers that we pull, so no need to
+		 * update skb->csum here.
+		 */
+
+		if (unlikely(parse_vlan(skb, key)))
+			return -ENOMEM;
+
+		key->eth.type = parse_ethertype(skb);
+		if (unlikely(key->eth.type == htons(0)))
+			return -ENOMEM;
+
+		/* Multiple tagged packets need to retain TPID to satisfy
+		 * skb_vlan_pop(), which will later shift the ethertype into
+		 * skb->protocol.
+		 */
+		if (key->eth.cvlan.tci & htons(VLAN_CFI_MASK))
+			skb->protocol = key->eth.cvlan.tpid;
+		else
+			skb->protocol = key->eth.type;
+
+		skb_reset_network_header(skb);
+		__skb_push(skb, skb->data - skb_mac_header(skb));
+	}
+
+	skb_reset_mac_len(skb);
+
+	/* Fill out L3/L4 key info, if any */
+	return key_extract_l3l4(skb, key);
+}
+
+/* In the case of conntrack fragment handling it expects L3 headers,
+ * add a helper.
+ */
+int ovs_flow_key_update_l3l4(struct sk_buff *skb, struct sw_flow_key *key)
+{
+	return key_extract_l3l4(skb, key);
+}
+
 int ovs_flow_key_update(struct sk_buff *skb, struct sw_flow_key *key)
 {
 	int res;
diff --git a/datapath/flow.h b/datapath/flow.h
index 4884c00..1a5df38 100644
--- a/datapath/flow.h
+++ b/datapath/flow.h
@@ -283,6 +283,7 @@  u64 ovs_flow_used_time(unsigned long flow_jiffies);
 
 /* Update the non-metadata part of the flow key using skb. */
 int ovs_flow_key_update(struct sk_buff *skb, struct sw_flow_key *key);
+int ovs_flow_key_update_l3l4(struct sk_buff *skb, struct sw_flow_key *key);
 int ovs_flow_key_extract(const struct ip_tunnel_info *tun_info,
 			 struct sk_buff *skb,
 			 struct sw_flow_key *key);