More accurate wildcarding and fragment handling.

This patch gets rid of the need for having explicit padding in struct flow as new fields are being added. flow_wildcards_init_exact(), which used to set bits in both compiler generated and explicit padding, is removed. match_wc_init() is now used instead, which generates the mask based on a given flow, setting bits only in fields which make sense. Places where random bits were placed in struct flow have been changed to only set random bits on fields that are significant in the given context. This avoids setting padding bits. - lib/flow: - Properly initialize struct flow also in places we used to zero out padding before. - Add flow_random_hash_fields() used for testing. - Remove flow_wildcards_init_exact() to avoid initializing masks where compiler generated padding has bits set. - lib/match.c match_wc_init(): Wildcard transport layer fields for later fragments, remove match_init_exact(), which used flow_wildcards_init_exact(). - tests/test-flows.c: use match_wc_init() instead of match_init_exact() - tests/flowgen.pl: generate more accurate packets and flows when fragmenting, mark unavailable fields as wildcarded. Signed-off-by: Jarno Rajahalme <[email protected]> Signed-off-by: Ben Pfaff <[email protected]>
leonshaw · Oct 17, 2013 · 9463996 · 9463996
1 parent a5ae88f
commit 9463996
Show file tree

Hide file tree

Showing 9 changed files with 107 additions and 72 deletions.
diff --git a/lib/flow.c b/lib/flow.c
@@ -35,6 +35,7 @@
 #include "ofpbuf.h"
 #include "openflow/openflow.h"
 #include "packets.h"
+#include "random.h"
 #include "unaligned.h"
 
 COVERAGE_DEFINE(flow_extract);
@@ -602,14 +603,6 @@ flow_wildcards_init_catchall(struct flow_wildcards *wc)
     memset(&wc->masks, 0, sizeof wc->masks);
 }
 
-/* Initializes 'wc' as an exact-match set of wildcards; that is, 'wc' does not
- * wildcard any bits or fields. */
-void
-flow_wildcards_init_exact(struct flow_wildcards *wc)
-{
-    memset(&wc->masks, 0xff, sizeof wc->masks);
-}
-
 /* Returns true if 'wc' matches every packet, false if 'wc' fixes any bits or
  * fields. */
 bool
@@ -799,6 +792,46 @@ flow_hash_symmetric_l4(const struct flow *flow, uint32_t basis)
     return jhash_bytes(&fields, sizeof fields, basis);
 }
 
+/* Initialize a flow with random fields that matter for nx_hash_fields. */
+void
+flow_random_hash_fields(struct flow *flow)
+{
+    uint16_t rnd = random_uint16();
+
+    /* Initialize to all zeros. */
+    memset(flow, 0, sizeof *flow);
+
+    eth_addr_random(flow->dl_src);
+    eth_addr_random(flow->dl_dst);
+
+    flow->vlan_tci = (OVS_FORCE ovs_be16) (random_uint16() & VLAN_VID_MASK);
+
+    /* Make most of the random flows IPv4, some IPv6, and rest random. */
+    flow->dl_type = rnd < 0x8000 ? htons(ETH_TYPE_IP) :
+        rnd < 0xc000 ? htons(ETH_TYPE_IPV6) : (OVS_FORCE ovs_be16)rnd;
+
+    if (dl_type_is_ip_any(flow->dl_type)) {
+        if (flow->dl_type == htons(ETH_TYPE_IP)) {
+            flow->nw_src = (OVS_FORCE ovs_be32)random_uint32();
+            flow->nw_dst = (OVS_FORCE ovs_be32)random_uint32();
+        } else {
+            random_bytes(&flow->ipv6_src, sizeof flow->ipv6_src);
+            random_bytes(&flow->ipv6_dst, sizeof flow->ipv6_dst);
+        }
+        /* Make most of IP flows TCP, some UDP or SCTP, and rest random. */
+        rnd = random_uint16();
+        flow->nw_proto = rnd < 0x8000 ? IPPROTO_TCP :
+            rnd < 0xc000 ? IPPROTO_UDP :
+            rnd < 0xd000 ? IPPROTO_SCTP : (uint8_t)rnd;
+        if (flow->nw_proto == IPPROTO_TCP ||
+            flow->nw_proto == IPPROTO_UDP ||
+            flow->nw_proto == IPPROTO_SCTP) {
+            flow->tp_src = (OVS_FORCE ovs_be16)random_uint16();
+            flow->tp_dst = (OVS_FORCE ovs_be16)random_uint16();
+        }
+    }
+}
+
 /* Masks the fields in 'wc' that are used by the flow hash 'fields'. */
 void
 flow_mask_hash_fields(const struct flow *flow, struct flow_wildcards *wc,

diff --git a/lib/flow.h b/lib/flow.h
@@ -78,13 +78,17 @@ union flow_in_port {
 };
 
 /*
-* A flow in the network.
-*
-* The meaning of 'in_port' is context-dependent.  In most cases, it is a
-* 16-bit OpenFlow 1.0 port number.  In the software datapath interface (dpif)
-* layer and its implementations (e.g. dpif-linux, dpif-netdev), it is instead
-* a 32-bit datapath port number.
-*/
+ * A flow in the network.
+ *
+ * Must be initialized to all zeros to make any compiler-induced padding
+ * zeroed.  Helps also in keeping unused fields (such as mutually exclusive
+ * IPv4 and IPv6 addresses) zeroed out.
+ *
+ * The meaning of 'in_port' is context-dependent.  In most cases, it is a
+ * 16-bit OpenFlow 1.0 port number.  In the software datapath interface (dpif)
+ * layer and its implementations (e.g. dpif-linux, dpif-netdev), it is instead
+ * a 32-bit datapath port number.
+ */
 struct flow {
     struct flow_tnl tunnel;     /* Encapsulating tunnel parameters. */
     ovs_be64 metadata;          /* OpenFlow Metadata. */
@@ -110,15 +114,17 @@ struct flow {
     uint8_t arp_sha[6];         /* ARP/ND source hardware address. */
     uint8_t arp_tha[6];         /* ARP/ND target hardware address. */
     uint8_t nw_ttl;             /* IP TTL/Hop Limit. */
-    uint8_t nw_frag;            /* FLOW_FRAG_* flags. */
+    uint8_t nw_frag;            /* FLOW_FRAG_* flags. Keep last for the
+                                   BUILD_ASSERT_DECL below */
 };
 BUILD_ASSERT_DECL(sizeof(struct flow) % 4 == 0);
 
 #define FLOW_U32S (sizeof(struct flow) / 4)
 
 /* Remember to update FLOW_WC_SEQ when changing 'struct flow'. */
-BUILD_ASSERT_DECL(sizeof(struct flow) == sizeof(struct flow_tnl) + 152 &&
-                  FLOW_WC_SEQ == 21);
+BUILD_ASSERT_DECL(offsetof(struct flow, nw_frag) + 1
+                  == sizeof(struct flow_tnl) + 152
+                  && FLOW_WC_SEQ == 21);
 
 /* Represents the metadata fields of struct flow. */
 struct flow_metadata {
@@ -238,7 +244,6 @@ struct flow_wildcards {
 };
 
 void flow_wildcards_init_catchall(struct flow_wildcards *);
-void flow_wildcards_init_exact(struct flow_wildcards *);
 
 bool flow_wildcards_is_catchall(const struct flow_wildcards *);
 
@@ -262,6 +267,8 @@ bool flow_wildcards_equal(const struct flow_wildcards *,
                           const struct flow_wildcards *);
 uint32_t flow_hash_symmetric_l4(const struct flow *flow, uint32_t basis);
 
+/* Initialize a flow with random fields that matter for nx_hash_fields. */
+void flow_random_hash_fields(struct flow *);
 void flow_mask_hash_fields(const struct flow *, struct flow_wildcards *,
                            enum nx_hash_fields);
 uint32_t flow_hash_fields(const struct flow *, enum nx_hash_fields,

diff --git a/lib/match.c b/lib/match.c
@@ -110,6 +110,10 @@ match_wc_init(struct match *match, const struct flow *flow)
 
         if (flow->nw_frag) {
             memset(&wc->masks.nw_frag, 0xff, sizeof wc->masks.nw_frag);
+            if (flow->nw_frag & FLOW_NW_FRAG_LATER) {
+                /* No transport layer header in later fragments. */
+                return;
+            }
         }
 
         if (flow->nw_proto == IPPROTO_ICMP ||
@@ -128,15 +132,6 @@ match_wc_init(struct match *match, const struct flow *flow)
     return;
 }
 
-/* Converts the flow in 'flow' into an exact-match match in 'match'. */
-void
-match_init_exact(struct match *match, const struct flow *flow)
-{
-    match->flow = *flow;
-    match->flow.skb_priority = 0;
-    flow_wildcards_init_exact(&match->wc);
-}
-
 /* Initializes 'match' as a "catch-all" match that matches every packet. */
 void
 match_init_catchall(struct match *match)

diff --git a/lib/match.h b/lib/match.h
@@ -38,7 +38,6 @@ void match_init(struct match *,
                 const struct flow *, const struct flow_wildcards *);
 void match_wc_init(struct match *match, const struct flow *flow);
 void match_init_catchall(struct match *);
-void match_init_exact(struct match *, const struct flow *);
 
 void match_zero_wildcarded_fields(struct match *);
 

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
@@ -4628,9 +4628,7 @@ rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto,
         cls_rule = classifier_lookup(cls, &ofpc_normal_flow, wc);
     } else if (frag && ofproto->up.frag_handling == OFPC_FRAG_DROP) {
         cls_rule = &ofproto->drop_frags_rule->up.cr;
-        if (wc) {
-            flow_wildcards_init_exact(wc);
-        }
+        /* Frag mask in wc already set above. */
     } else {
         cls_rule = classifier_lookup(cls, flow, wc);
     }

diff --git a/tests/flowgen.pl b/tests/flowgen.pl
@@ -109,7 +109,8 @@ sub output {
 
     # Compose packet.
     my $packet = '';
-    my $wildcards = 0;
+    my $wildcards = 1 << 5 | 1 << 6 | 1 << 7 | 32 << 8 | 32 << 14 | 1 << 21;
+
     $packet .= pack_ethaddr($flow{DL_DST});
     $packet .= pack_ethaddr($flow{DL_SRC});
     if ($flow{DL_VLAN} != 0xffff) {
@@ -139,6 +140,7 @@ sub output {
                           0,               # checksum
                           0x0a00020f,      # source
                           0xc0a80114);     # dest
+            $wildcards &= ~( 1 << 5 | 63 << 8 | 63 << 14 | 1 << 21);
             if ($attrs{IP_OPTIONS} eq 'yes') {
                 substr($ip, 0, 1) = pack('C', (4 << 4) | 8);
                 $ip .= pack('CCnnnCCCx',
@@ -151,46 +153,51 @@ sub output {
                             2,
                             3);
             }
+
             if ($attrs{IP_FRAGMENT} ne 'no') {
                 my (%frag_map) = ('first' => 0x2000, # more frags, ofs 0
                                   'middle' => 0x2111, # more frags, ofs 0x888
                                   'last' => 0x0222); # last frag, ofs 0x1110
                 substr($ip, 6, 2)
                   = pack('n', $frag_map{$attrs{IP_FRAGMENT}});
             }
-
-            if ($attrs{TP_PROTO} =~ '^TCP') {
-                my $tcp = pack('nnNNnnnn',
-                               $flow{TP_SRC},     # source port
-                               $flow{TP_DST},     # dest port
-                               87123455,          # seqno
-                               712378912,         # ackno
-                               (5 << 12) | 0x02 | 0x10, # hdrlen, SYN, ACK
-                               5823,                    # window size
-                               18923,                   # checksum
-                               12893); # urgent pointer
-                if ($attrs{TP_PROTO} eq 'TCP+options') {
-                    substr($tcp, 12, 2) = pack('n', (6 << 12) | 0x02 | 0x10);
-                    $tcp .= pack('CCn', 2, 4, 1975); # MSS option
+            if ($attrs{IP_FRAGMENT} eq 'no' || $attrs{IP_FRAGMENT} eq 'first') {
+                if ($attrs{TP_PROTO} =~ '^TCP') {
+                    my $tcp = pack('nnNNnnnn',
+                                   $flow{TP_SRC},     # source port
+                                   $flow{TP_DST},     # dest port
+                                   87123455,          # seqno
+                                   712378912,         # ackno
+                                   (5 << 12) | 0x02 | 0x10, # hdrlen, SYN, ACK
+                                   5823,                    # window size
+                                   18923,                   # checksum
+                                   12893); # urgent pointer
+                    if ($attrs{TP_PROTO} eq 'TCP+options') {
+                        substr($tcp, 12, 2) = pack('n', (6 << 12) | 0x02 | 0x10);
+                        $tcp .= pack('CCn', 2, 4, 1975); # MSS option
+                    }
+                    $tcp .= 'payload';
+                    $ip .= $tcp;
+                    $wildcards &= ~(1 << 6 | 1 << 7);
+                } elsif ($attrs{TP_PROTO} eq 'UDP') {
+                    my $len = 15;
+                    my $udp = pack('nnnn', $flow{TP_SRC}, $flow{TP_DST}, $len, 0);
+                    $udp .= chr($len) while length($udp) < $len;
+                    $ip .= $udp;
+                    $wildcards &= ~(1 << 6 | 1 << 7);
+                } elsif ($attrs{TP_PROTO} eq 'ICMP') {
+                    $ip .= pack('CCnnn',
+                                8,        # echo request
+                                0,        # code
+                                0,        # checksum
+                                736,      # identifier
+                                931);     # sequence number
+                    $wildcards &= ~(1 << 6 | 1 << 7);
+                } elsif ($attrs{TP_PROTO} eq 'other') {
+                    $ip .= 'other header';
+                } else {
+                    die;
                 }
-                $tcp .= 'payload';
-                $ip .= $tcp;
-            } elsif ($attrs{TP_PROTO} eq 'UDP') {
-                my $len = 15;
-                my $udp = pack('nnnn', $flow{TP_SRC}, $flow{TP_DST}, $len, 0);
-                $udp .= chr($len) while length($udp) < $len;
-                $ip .= $udp;
-            } elsif ($attrs{TP_PROTO} eq 'ICMP') {
-                $ip .= pack('CCnnn',
-                            8,        # echo request
-                            0,        # code
-                            0,        # checksum
-                            736,      # identifier
-                            931);     # sequence number
-            } elsif ($attrs{TP_PROTO} eq 'other') {
-                $ip .= 'other header';
-            } else {
-                die;
             }
             substr($ip, 2, 2) = pack('n', length($ip));
             $packet .= $ip;

diff --git a/tests/test-bundle.c b/tests/test-bundle.c
@@ -23,7 +23,6 @@
 #include "flow.h"
 #include "ofp-actions.h"
 #include "ofpbuf.h"
-#include "random.h"
 
 #include "util.h"
 
@@ -116,7 +115,6 @@ main(int argc, char *argv[])
     int old_active;
 
     set_program_name(argv[0]);
-    random_init();
 
     if (argc != 2) {
         ovs_fatal(0, "usage: %s bundle_action", program_name);
@@ -140,7 +138,7 @@ main(int argc, char *argv[])
     /* Generate flows. */
     flows = xmalloc(N_FLOWS * sizeof *flows);
     for (i = 0; i < N_FLOWS; i++) {
-        random_bytes(&flows[i], sizeof flows[i]);
+        flow_random_hash_fields(&flows[i]);
         flows[i].regs[0] = ofp_to_u16(OFPP_NONE);
     }
 

diff --git a/tests/test-flows.c b/tests/test-flows.c
@@ -70,7 +70,7 @@ main(int argc OVS_UNUSED, char *argv[])
 
         in_port_.ofp_port = u16_to_ofp(1);
         flow_extract(packet, 0, 0, NULL, &in_port_, &flow);
-        match_init_exact(&match, &flow);
+        match_wc_init(&match, &flow);
         ofputil_match_to_ofp10_match(&match, &extracted_match);
 
         if (memcmp(&expected_match, &extracted_match, sizeof expected_match)) {

diff --git a/tests/test-multipath.c b/tests/test-multipath.c
@@ -26,7 +26,6 @@
 
 #include "flow.h"
 #include "ofp-actions.h"
-#include "random.h"
 #include "util.h"
 
 int
@@ -39,7 +38,6 @@ main(int argc, char *argv[])
     int n;
 
     set_program_name(argv[0]);
-    random_init();
 
     if (argc != 2) {
         ovs_fatal(0, "usage: %s multipath_action", program_name);
@@ -65,7 +63,7 @@ main(int argc, char *argv[])
             struct flow_wildcards wc;
             struct flow flow;
 
-            random_bytes(&flow, sizeof flow);
+            flow_random_hash_fields(&flow);
 
             mp.max_link = n - 1;
             multipath_execute(&mp, &flow, &wc);