[ovs-dev] [PATCH v2] dpif-linux: Fix build with certain 64-bit kernel/userspace combinations.

Ben Pfaff blp at nicira.com
Thu Oct 13 18:29:50 UTC 2011


Pravin's testing found a bug in my patch.  v2 fixes up the memset and
memcpy calls, like so:

    -    memset(&dst, 0, sizeof dst);
    -    memcpy(&dst, &src, 64);
    +    memset(dst, 0, sizeof *dst);
    +    memcpy(dst, src, 64);

--8<--------------------------cut here-------------------------->8--

From: Ben Pfaff <blp at nicira.com>
Date: Thu, 13 Oct 2011 11:22:44 -0700
Subject: [PATCH] dpif-linux: Fix build with certain 64-bit kernel/userspace
 combinations.

Unix 64-bit ABIs have two 64-bit types: "long" and "long long".  Either of
these is a reasonable choice for uint64_t (the userspace type) and for
__u64 (the kernel type).  Unfortunately, kernel and userspace don't
necessarily agree on the choice, and in fact the choice varies across
kernel versions and architectures.

Now that OVS is actually using kernel types in its kernel header, this
can make a difference: when __u64 and uint64_t differ, passing a pointer
to __u64 to OVS function get_unaligned_u64() yields a compiler warning
or error.

This commit fixes up the problems of this type found in OVS, by avoiding
calls to get_unaligned_u64() on these types.

I suspect that this problem will recur in the future.  I'm open to
suggestions on how we can making "uint64_t *" and "__u64 *" always
incompatible from the viewpoint of sparse.

Reported-by: Pravin Shelar <pshelar at nicira.com>
---
 lib/dpif-linux.c   |   19 +++++--------------
 lib/netdev-vport.c |   49 ++++++++++++++++++++++++++++---------------------
 lib/util.h         |    4 ++++
 3 files changed, 37 insertions(+), 35 deletions(-)

diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
index 43c2161..9566dcb 100644
--- a/lib/dpif-linux.c
+++ b/lib/dpif-linux.c
@@ -102,17 +102,13 @@ struct dpif_linux_flow {
 
     /* Attributes.
      *
-     * The 'stats' member points to 64-bit data that might only be aligned on
-     * 32-bit boundaries, so get_unaligned_u64() should be used to access its
-     * values.
-     *
      * If 'actions' is nonnull then OVS_FLOW_ATTR_ACTIONS will be included in
      * the Netlink version of the command, even if actions_len is zero. */
     const struct nlattr *key;           /* OVS_FLOW_ATTR_KEY. */
     size_t key_len;
     const struct nlattr *actions;       /* OVS_FLOW_ATTR_ACTIONS. */
     size_t actions_len;
-    const struct ovs_flow_stats *stats; /* OVS_FLOW_ATTR_STATS. */
+    struct ovs_flow_stats stats;        /* OVS_FLOW_ATTR_STATS. */
     const uint8_t *tcp_flags;           /* OVS_FLOW_ATTR_TCP_FLAGS. */
     const ovs_32aligned_u64 *used;      /* OVS_FLOW_ATTR_USED. */
     bool clear;                         /* OVS_FLOW_ATTR_CLEAR. */
@@ -1622,7 +1618,8 @@ dpif_linux_flow_from_ofpbuf(struct dpif_linux_flow *flow,
         flow->actions_len = nl_attr_get_size(a[OVS_FLOW_ATTR_ACTIONS]);
     }
     if (a[OVS_FLOW_ATTR_STATS]) {
-        flow->stats = nl_attr_get(a[OVS_FLOW_ATTR_STATS]);
+        memcpy(&flow->stats, nl_attr_get(a[OVS_FLOW_ATTR_STATS]),
+               sizeof flow->stats);
     }
     if (a[OVS_FLOW_ATTR_TCP_FLAGS]) {
         flow->tcp_flags = nl_attr_get(a[OVS_FLOW_ATTR_TCP_FLAGS]);
@@ -1658,7 +1655,6 @@ dpif_linux_flow_to_ofpbuf(const struct dpif_linux_flow *flow,
     }
 
     /* We never need to send these to the kernel. */
-    assert(!flow->stats);
     assert(!flow->tcp_flags);
     assert(!flow->used);
 
@@ -1711,13 +1707,8 @@ static void
 dpif_linux_flow_get_stats(const struct dpif_linux_flow *flow,
                           struct dpif_flow_stats *stats)
 {
-    if (flow->stats) {
-        stats->n_packets = get_unaligned_u64(&flow->stats->n_packets);
-        stats->n_bytes = get_unaligned_u64(&flow->stats->n_bytes);
-    } else {
-        stats->n_packets = 0;
-        stats->n_bytes = 0;
-    }
+    stats->n_packets = flow->stats.n_packets;
+    stats->n_bytes = flow->stats.n_bytes;
     stats->used = flow->used ? get_32aligned_u64(flow->used) : 0;
     stats->tcp_flags = flow->tcp_flags ? *flow->tcp_flags : 0;
 }
diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
index 301bb43..868c935 100644
--- a/lib/netdev-vport.c
+++ b/lib/netdev-vport.c
@@ -369,27 +369,34 @@ static void
 netdev_stats_from_ovs_vport_stats(struct netdev_stats *dst,
                                   const struct ovs_vport_stats *src)
 {
-    dst->rx_packets = get_unaligned_u64(&src->rx_packets);
-    dst->tx_packets = get_unaligned_u64(&src->tx_packets);
-    dst->rx_bytes = get_unaligned_u64(&src->rx_bytes);
-    dst->tx_bytes = get_unaligned_u64(&src->tx_bytes);
-    dst->rx_errors = get_unaligned_u64(&src->rx_errors);
-    dst->tx_errors = get_unaligned_u64(&src->tx_errors);
-    dst->rx_dropped = get_unaligned_u64(&src->rx_dropped);
-    dst->tx_dropped = get_unaligned_u64(&src->tx_dropped);
-    dst->multicast = 0;
-    dst->collisions = 0;
-    dst->rx_length_errors = 0;
-    dst->rx_over_errors = 0;
-    dst->rx_crc_errors = 0;
-    dst->rx_frame_errors = 0;
-    dst->rx_fifo_errors = 0;
-    dst->rx_missed_errors = 0;
-    dst->tx_aborted_errors = 0;
-    dst->tx_carrier_errors = 0;
-    dst->tx_fifo_errors = 0;
-    dst->tx_heartbeat_errors = 0;
-    dst->tx_window_errors = 0;
+    /* ovs_vport_stats and netdev_stats start with members that have the same
+     * meaning in the same order, so we can just memcpy() them across as a
+     * block.  There's more at play here than an optimization: 'src' might be
+     * misaligned, so we can't just do memberwise assignments.  Second, we
+     * can't just use get_unaligned_u64() on the members of 'src' because they
+     * are "__u64"s, which type might be different from "uint64_t" (e.g. "long"
+     * versus "long long") and thus we'd get a compiler warning or error about
+     * type mismatch.
+     *
+     * The following macro gunge checks that in fact all of the common members
+     * have identical sizes and offets. */
+#define VERIFY_STAT_MEMBER(MEMBER, OFFSET) \
+    BUILD_ASSERT_DECL(offsetof(struct netdev_stats, MEMBER) == (OFFSET)); \
+    BUILD_ASSERT_DECL(offsetof(struct ovs_vport_stats, MEMBER) == (OFFSET)); \
+    BUILD_ASSERT_DECL(MEMBER_SIZEOF(struct netdev_stats, MEMBER) == 8); \
+    BUILD_ASSERT_DECL(MEMBER_SIZEOF(struct ovs_vport_stats, MEMBER) == 8)
+
+    VERIFY_STAT_MEMBER(rx_packets,  0);
+    VERIFY_STAT_MEMBER(tx_packets,  8);
+    VERIFY_STAT_MEMBER(rx_bytes,   16);
+    VERIFY_STAT_MEMBER(tx_bytes,   24);
+    VERIFY_STAT_MEMBER(rx_errors,  32);
+    VERIFY_STAT_MEMBER(tx_errors,  40);
+    VERIFY_STAT_MEMBER(rx_dropped, 48);
+    VERIFY_STAT_MEMBER(tx_dropped, 56);
+
+    memset(dst, 0, sizeof *dst);
+    memcpy(dst, src, 64);
 }
 
 /* Copies 'src' into 'dst', performing format conversion in the process. */
diff --git a/lib/util.h b/lib/util.h
index 5ae0775..4ff2734 100644
--- a/lib/util.h
+++ b/lib/util.h
@@ -88,6 +88,10 @@ extern const char *program_name;
 #define STRINGIZE(ARG) STRINGIZE2(ARG)
 #define STRINGIZE2(ARG) #ARG
 
+/* Expands to the size of MEMBER within TYPE, which must be a structure type
+ * such that appending "*" yields a pointer to TYPE. */
+#define MEMBER_SIZEOF(STRUCT, MEMBER) (sizeof ((STRUCT *) 0)->MEMBER)
+
 /* Given a pointer-typed lvalue OBJECT, expands to a pointer type that may be
  * assigned to OBJECT. */
 #ifdef __GNUC__
-- 
1.7.4.4




More information about the dev mailing list