[ovs-dev] [PATCH v2] dpif-linux: Fix build with certain 64-bit kernel/userspace combinations.
Pravin Shelar
pshelar at nicira.com
Thu Oct 13 12:50:26 PDT 2011
On Thu, Oct 13, 2011 at 11:29 AM, Ben Pfaff <blp at nicira.com> wrote:
> 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);
>
This change fixes the issue.
--pravin.
> --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