diff mbox series

[ovs-dev,09/10] ofp-actions: Make all actions a multiple of OFPACT_ALIGNTO bytes.

Message ID 20181115060534.7146-9-blp@ovn.org
State Accepted
Headers show
Series [ovs-dev,01/10] ovsdb-idl: Avoid sending transactions when the DB is not synced up. | expand

Commit Message

Ben Pfaff Nov. 15, 2018, 6:05 a.m. UTC
The functions to put ofpacts into ofpbufs have always padded them to
OFPACT_ALIGNTO boundaries, but the underlying structures weren't
necessarily padded out.  That led to difficulties in a few places where
structures were allocated on the stack instead in an ofpbuf, because
functions like ofpact_init_*() would access beyond the end of the actual
structure.  This is true, for example, in test_multipath_main() in
tests/test-multipath.c, which allocates a struct ofpact_multipath on the
stack, and in lswitch_handshake() in learning-switch.c, which allocates
a struct ofpact_output on the stack.

It's possible to fix these individual cases, but it's possible that there
are others that haven't been identified.  This commit addresses the issue
another way, by padding all of the ofpact structures to a full multiple
of OFPACT_ALIGNTO and adding assertions to ensure that it can't be screwed
up in the future.

This commit removes the OFPACT_*_SIZE enums, because they are now
equivalent to sizeof(struct ofpact_*) in every case.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 include/openvswitch/ofp-actions.h | 393 +++++++++++++++++++++++---------------
 lib/ofp-actions.c                 |   2 +-
 ofproto/connmgr.c                 |   2 +-
 ofproto/fail-open.c               |   2 +-
 4 files changed, 237 insertions(+), 162 deletions(-)

Comments

0-day Robot Nov. 15, 2018, 7:10 a.m. UTC | #1
Bleep bloop.  Greetings Ben Pfaff, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


build:
     BUILD_ASSERT_DECL(offsetof(struct STRUCT, MEMBER)                   \
     ^
./include/openvswitch/ofp-actions.h:69:5: note: in expansion of macro 'OFPACT'
     OFPACT(BUNDLE,          ofpact_bundle,      slaves, "bundle")       \
     ^
./include/openvswitch/ofp-actions.h:1279:1: note: in expansion of macro 'OFPACTS'
 OFPACTS
 ^
./include/openvswitch/compiler.h:255:33: error: static assertion failed: "!offsetof(struct ofpact_bundle, slaves) || (offsetof(struct ofpact_bundle, slaves) == sizeof(struct ofpact_bundle))"
 #define BUILD_ASSERT_DECL(EXPR) _Static_assert(EXPR, #EXPR)
                                 ^
./include/openvswitch/ofp-actions.h:1240:5: note: in expansion of macro 'BUILD_ASSERT_DECL'
     BUILD_ASSERT_DECL(!offsetof(struct STRUCT, MEMBER)                  \
     ^
./include/openvswitch/ofp-actions.h:69:5: note: in expansion of macro 'OFPACT'
     OFPACT(BUNDLE,          ofpact_bundle,      slaves, "bundle")       \
     ^
./include/openvswitch/ofp-actions.h:1279:1: note: in expansion of macro 'OFPACTS'
 OFPACTS
 ^
make[2]: *** [lib/bfd.lo] Error 1
make[2]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make: *** [all] Error 2


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot
Ben Pfaff Nov. 15, 2018, 2:28 p.m. UTC | #2
On Thu, Nov 15, 2018 at 02:10:23AM -0500, 0-day Robot wrote:
> Bleep bloop.  Greetings Ben Pfaff, I am a robot and I have tried out your patch.
> Thanks for your contribution.
> 
> I encountered some error that I wasn't expecting.  See the details below.
> 
> 
> build:
>      BUILD_ASSERT_DECL(offsetof(struct STRUCT, MEMBER)                   \
>      ^
> ./include/openvswitch/ofp-actions.h:69:5: note: in expansion of macro 'OFPACT'
>      OFPACT(BUNDLE,          ofpact_bundle,      slaves, "bundle")       \
>      ^
> ./include/openvswitch/ofp-actions.h:1279:1: note: in expansion of macro 'OFPACTS'
>  OFPACTS
>  ^
> ./include/openvswitch/compiler.h:255:33: error: static assertion failed: "!offsetof(struct ofpact_bundle, slaves) || (offsetof(struct ofpact_bundle, slaves) == sizeof(struct ofpact_bundle))"
>  #define BUILD_ASSERT_DECL(EXPR) _Static_assert(EXPR, #EXPR)

Oops, here's an incremental fix for 64-bit architectures:

diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h
index bdd3b28de886..4daf5ad071da 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -395,8 +395,8 @@ struct ofpact_bundle {
 
         /* Slaves for output. */
         unsigned int n_slaves;
-        ofp_port_t slaves[];
     );
+    ofp_port_t slaves[];
 };
 
 /* OFPACT_SET_VLAN_VID.
diff mbox series

Patch

diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h
index e0588afad854..bdd3b28de886 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -275,16 +275,20 @@  ofpact_find_type_flattened(const struct ofpact *a, enum ofpact_type type,
  * Action structure for actions that do not have any extra data beyond the
  * action type. */
 struct ofpact_null {
-    struct ofpact ofpact;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+    );
 };
 
 /* OFPACT_OUTPUT.
  *
  * Used for OFPAT10_OUTPUT. */
 struct ofpact_output {
-    struct ofpact ofpact;
-    ofp_port_t port;            /* Output port. */
-    uint16_t max_len;           /* Max send len, for port OFPP_CONTROLLER. */
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        ofp_port_t port;        /* Output port. */
+        uint16_t max_len;       /* Max send len, for port OFPP_CONTROLLER. */
+    );
 };
 
 #define NX_CTLR_NO_METER 0
@@ -321,27 +325,33 @@  struct ofpact_controller {
  *
  * Used for OFPAT10_ENQUEUE. */
 struct ofpact_enqueue {
-    struct ofpact ofpact;
-    ofp_port_t port;
-    uint32_t queue;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        ofp_port_t port;
+        uint32_t queue;
+    );
 };
 
 /* OFPACT_OUTPUT_REG.
  *
  * Used for NXAST_OUTPUT_REG. */
 struct ofpact_output_reg {
-    struct ofpact ofpact;
-    uint16_t max_len;
-    struct mf_subfield src;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        uint16_t max_len;
+        struct mf_subfield src;
+    );
 };
 
 /* OFPACT_OUTPUT_TRUNC.
  *
  * Used for NXAST_OUTPUT_TRUNC. */
 struct ofpact_output_trunc {
-    struct ofpact ofpact;
-    ofp_port_t port;            /* Output port. */
-    uint32_t max_len;           /* Max send len. */
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        ofp_port_t port;        /* Output port. */
+        uint32_t max_len;       /* Max send len. */
+    );
 };
 
 /* Bundle slave choice algorithm to apply.
@@ -371,20 +381,22 @@  enum nx_bd_algorithm {
  *
  * Used for NXAST_BUNDLE. */
 struct ofpact_bundle {
-    struct ofpact ofpact;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
 
-    /* Slave choice algorithm to apply to hash value. */
-    enum nx_bd_algorithm algorithm;
+        /* Slave choice algorithm to apply to hash value. */
+        enum nx_bd_algorithm algorithm;
 
-    /* What fields to hash and how. */
-    enum nx_hash_fields fields;
-    uint16_t basis;             /* Universal hash parameter. */
+        /* What fields to hash and how. */
+        enum nx_hash_fields fields;
+        uint16_t basis;         /* Universal hash parameter. */
 
-    struct mf_subfield dst;
+        struct mf_subfield dst;
 
-    /* Slaves for output. */
-    unsigned int n_slaves;
-    ofp_port_t slaves[];
+        /* Slaves for output. */
+        unsigned int n_slaves;
+        ofp_port_t slaves[];
+    );
 };
 
 /* OFPACT_SET_VLAN_VID.
@@ -396,10 +408,12 @@  struct ofpact_bundle {
  *
  * Used for OFPAT10_SET_VLAN_VID and OFPAT11_SET_VLAN_VID. */
 struct ofpact_vlan_vid {
-    struct ofpact ofpact;
-    uint16_t vlan_vid;          /* VLAN VID in low 12 bits, 0 in other bits. */
-    bool push_vlan_if_needed;   /* OF 1.0 semantics if true. */
-    bool flow_has_vlan;         /* VLAN present at action validation time? */
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        uint16_t vlan_vid;        /* VLAN VID in low 12 bits, other bits 0. */
+        bool push_vlan_if_needed; /* OF 1.0 semantics if true. */
+        bool flow_has_vlan;       /* VLAN present at action validation time? */
+    );
 };
 
 /* OFPACT_SET_VLAN_PCP.
@@ -411,84 +425,104 @@  struct ofpact_vlan_vid {
  *
  * Used for OFPAT10_SET_VLAN_PCP and OFPAT11_SET_VLAN_PCP. */
 struct ofpact_vlan_pcp {
-    struct ofpact ofpact;
-    uint8_t vlan_pcp;           /* VLAN PCP in low 3 bits, 0 in other bits. */
-    bool push_vlan_if_needed;   /* OF 1.0 semantics if true. */
-    bool flow_has_vlan;         /* VLAN present at action validation time? */
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        uint8_t vlan_pcp;           /* VLAN PCP in low 3 bits, other bits 0. */
+        bool push_vlan_if_needed;   /* OF 1.0 semantics if true. */
+        bool flow_has_vlan;         /* VLAN present at action validation? */
+    );
 };
 
 /* OFPACT_PUSH_VLAN.
  *
  * Used for OFPAT11_PUSH_VLAN. */
 struct ofpact_push_vlan {
-    struct ofpact ofpact;
-    ovs_be16 ethertype;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        ovs_be16 ethertype;
+    );
 };
 
 /* OFPACT_SET_ETH_SRC, OFPACT_SET_ETH_DST.
  *
  * Used for OFPAT10_SET_DL_SRC, OFPAT10_SET_DL_DST. */
 struct ofpact_mac {
-    struct ofpact ofpact;
-    struct eth_addr mac;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        struct eth_addr mac;
+    );
 };
 
 /* OFPACT_SET_IPV4_SRC, OFPACT_SET_IPV4_DST.
  *
  * Used for OFPAT10_SET_NW_SRC, OFPAT10_SET_NW_DST. */
 struct ofpact_ipv4 {
-    struct ofpact ofpact;
-    ovs_be32 ipv4;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        ovs_be32 ipv4;
+    );
 };
 
 /* OFPACT_SET_IP_DSCP.
  *
  * Used for OFPAT10_SET_NW_TOS. */
 struct ofpact_dscp {
-    struct ofpact ofpact;
-    uint8_t dscp;               /* DSCP in high 6 bits, rest ignored. */
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        uint8_t dscp;           /* DSCP in high 6 bits, rest ignored. */
+    );
 };
 
 /* OFPACT_SET_IP_ECN.
  *
  * Used for OFPAT11_SET_NW_ECN. */
 struct ofpact_ecn {
-    struct ofpact ofpact;
-    uint8_t ecn;               /* ECN in low 2 bits, rest ignored. */
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        uint8_t ecn;            /* ECN in low 2 bits, rest ignored. */
+    );
 };
 
 /* OFPACT_SET_IP_TTL.
  *
  * Used for OFPAT11_SET_NW_TTL. */
 struct ofpact_ip_ttl {
-    struct ofpact ofpact;
-    uint8_t ttl;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        uint8_t ttl;
+    );
 };
 
 /* OFPACT_SET_L4_SRC_PORT, OFPACT_SET_L4_DST_PORT.
  *
  * Used for OFPAT10_SET_TP_SRC, OFPAT10_SET_TP_DST. */
 struct ofpact_l4_port {
-    struct ofpact ofpact;
-    uint16_t port;              /* TCP, UDP or SCTP port number. */
-    uint8_t  flow_ip_proto;     /* IP proto from corresponding match, or 0 */
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        uint16_t port;          /* TCP, UDP or SCTP port number. */
+        uint8_t  flow_ip_proto; /* IP proto from corresponding match, or 0 */
+    );
 };
 
 /* OFPACT_REG_MOVE.
  *
  * Used for NXAST_REG_MOVE. */
 struct ofpact_reg_move {
-    struct ofpact ofpact;
-    struct mf_subfield src;
-    struct mf_subfield dst;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        struct mf_subfield src;
+        struct mf_subfield dst;
+    );
 };
 
 /* OFPACT_STACK_PUSH, OFPACT_STACK_POP.
  *
  * Used for NXAST_STACK_PUSH and NXAST_STACK_POP. */
 struct ofpact_stack {
-    struct ofpact ofpact;
-    struct mf_subfield subfield;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        struct mf_subfield subfield;
+    );
 };
 
 /* OFPACT_SET_FIELD.
@@ -517,59 +551,73 @@  BUILD_ASSERT_DECL(offsetof(struct ofpact_set_field, value)
  *
  * Used for NXAST_PUSH_MPLS, OFPAT11_PUSH_MPLS. */
 struct ofpact_push_mpls {
-    struct ofpact ofpact;
-    ovs_be16 ethertype;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        ovs_be16 ethertype;
+    );
 };
 
 /* OFPACT_POP_MPLS
  *
  * Used for NXAST_POP_MPLS, OFPAT11_POP_MPLS.. */
 struct ofpact_pop_mpls {
-    struct ofpact ofpact;
-    ovs_be16 ethertype;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        ovs_be16 ethertype;
+    );
 };
 
 /* OFPACT_SET_TUNNEL.
  *
  * Used for NXAST_SET_TUNNEL, NXAST_SET_TUNNEL64. */
 struct ofpact_tunnel {
-    struct ofpact ofpact;
-    uint64_t tun_id;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        uint64_t tun_id;
+    );
 };
 
 /* OFPACT_SET_QUEUE.
  *
  * Used for NXAST_SET_QUEUE. */
 struct ofpact_queue {
-    struct ofpact ofpact;
-    uint32_t queue_id;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        uint32_t queue_id;
+    );
 };
 
 /* OFPACT_FIN_TIMEOUT.
  *
  * Used for NXAST_FIN_TIMEOUT. */
 struct ofpact_fin_timeout {
-    struct ofpact ofpact;
-    uint16_t fin_idle_timeout;
-    uint16_t fin_hard_timeout;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        uint16_t fin_idle_timeout;
+        uint16_t fin_hard_timeout;
+    );
 };
 
 /* OFPACT_WRITE_METADATA.
  *
  * Used for NXAST_WRITE_METADATA. */
 struct ofpact_metadata {
-    struct ofpact ofpact;
-    ovs_be64 metadata;
-    ovs_be64 mask;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        ovs_be64 metadata;
+        ovs_be64 mask;
+    );
 };
 
 /* OFPACT_METER.
  *
  * Used for OFPIT13_METER. */
 struct ofpact_meter {
-    struct ofpact ofpact;
-    uint32_t meter_id;
-    uint32_t provider_meter_id;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        uint32_t meter_id;
+        uint32_t provider_meter_id;
+    );
 };
 
 /* OFPACT_WRITE_ACTIONS, OFPACT_CLONE.
@@ -657,25 +705,27 @@  enum nx_nat_flags {
  *
  * Used for NXAST_NAT. */
 struct ofpact_nat {
-    struct ofpact ofpact;
-    uint8_t range_af; /* AF_UNSPEC, AF_INET, or AF_INET6 */
-    uint16_t flags;  /* NX_NAT_F_* */
-    struct {
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        uint8_t range_af; /* AF_UNSPEC, AF_INET, or AF_INET6 */
+        uint16_t flags;  /* NX_NAT_F_* */
         struct {
-            uint16_t min;
-            uint16_t max;
-        } proto;
-        union {
-            struct {
-                ovs_be32 min;
-                ovs_be32 max;
-            } ipv4;
             struct {
-                struct in6_addr min;
-                struct in6_addr max;
-            } ipv6;
-        } addr;
-    } range;
+                uint16_t min;
+                uint16_t max;
+            } proto;
+            union {
+                struct {
+                    ovs_be32 min;
+                    ovs_be32 max;
+                } ipv4;
+                struct {
+                    struct in6_addr min;
+                    struct in6_addr max;
+                } ipv6;
+            } addr;
+        } range;
+    );
 };
 
 
@@ -683,11 +733,13 @@  struct ofpact_nat {
  *
  * Used for NXAST_RESUBMIT, NXAST_RESUBMIT_TABLE, NXAST_RESUBMIT_TABLE_CT. */
 struct ofpact_resubmit {
-    struct ofpact ofpact;
-    ofp_port_t in_port;
-    uint8_t table_id;
-    bool with_ct_orig;   /* Resubmit with Conntrack original direction tuple
-                          * fields in place of IP header fields. */
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        ofp_port_t in_port;
+        uint8_t table_id;
+        bool with_ct_orig;  /* Resubmit with Conntrack original direction tuple
+                             * fields in place of IP header fields. */
+    );
 };
 
 /* Bits for 'flags' in struct nx_action_learn.
@@ -870,37 +922,43 @@  enum nx_mp_algorithm {
  *
  * Used for NXAST_CONJUNCTION. */
 struct ofpact_conjunction {
-    struct ofpact ofpact;
-    uint8_t clause;
-    uint8_t n_clauses;
-    uint32_t id;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        uint8_t clause;
+        uint8_t n_clauses;
+        uint32_t id;
+    );
 };
 
 /* OFPACT_MULTIPATH.
  *
  * Used for NXAST_MULTIPATH. */
 struct ofpact_multipath {
-    struct ofpact ofpact;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
 
-    /* What fields to hash and how. */
-    enum nx_hash_fields fields;
-    uint16_t basis;             /* Universal hash parameter. */
+        /* What fields to hash and how. */
+        enum nx_hash_fields fields;
+        uint16_t basis;         /* Universal hash parameter. */
 
-    /* Multipath link choice algorithm to apply to hash value. */
-    enum nx_mp_algorithm algorithm;
-    uint16_t max_link;          /* Number of output links, minus 1. */
-    uint32_t arg;               /* Algorithm-specific argument. */
+        /* Multipath link choice algorithm to apply to hash value. */
+        enum nx_mp_algorithm algorithm;
+        uint16_t max_link;      /* Number of output links, minus 1. */
+        uint32_t arg;           /* Algorithm-specific argument. */
 
-    /* Where to store the result. */
-    struct mf_subfield dst;
+        /* Where to store the result. */
+        struct mf_subfield dst;
+    );
 };
 
 /* OFPACT_NOTE.
  *
  * Used for NXAST_NOTE. */
 struct ofpact_note {
-    struct ofpact ofpact;
-    size_t length;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        size_t length;
+    );
     uint8_t data[];
 };
 
@@ -922,23 +980,25 @@  enum nx_action_sample_direction {
  *
  * Used for NXAST_SAMPLE, NXAST_SAMPLE2, and NXAST_SAMPLE3. */
 struct ofpact_sample {
-    struct ofpact ofpact;
-    uint16_t probability;  /* Always positive. */
-    uint32_t collector_set_id;
-    uint32_t obs_domain_id;
-    uint32_t obs_point_id;
-    ofp_port_t sampling_port;
-    enum nx_action_sample_direction direction;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        uint16_t probability;   /* Always positive. */
+        uint32_t collector_set_id;
+        uint32_t obs_domain_id;
+        uint32_t obs_point_id;
+        ofp_port_t sampling_port;
+        enum nx_action_sample_direction direction;
+    );
 };
 
 /* OFPACT_DEC_TTL.
  *
  * Used for OFPAT11_DEC_NW_TTL, NXAST_DEC_TTL and NXAST_DEC_TTL_CNT_IDS. */
 struct ofpact_cnt_ids {
-    struct ofpact ofpact;
-
-    /* Controller ids. */
-    unsigned int n_controllers;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        unsigned int n_controllers;
+    );
     uint16_t cnt_ids[];
 };
 
@@ -946,54 +1006,63 @@  struct ofpact_cnt_ids {
  *
  * Used for OFPAT11_SET_MPLS_LABEL and NXAST_SET_MPLS_LABEL */
 struct ofpact_mpls_label {
-    struct ofpact ofpact;
-
-    ovs_be32 label;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        ovs_be32 label;
+    );
 };
 
 /* OFPACT_SET_MPLS_TC.
  *
  * Used for OFPAT11_SET_MPLS_TC and NXAST_SET_MPLS_TC */
 struct ofpact_mpls_tc {
-    struct ofpact ofpact;
-
-    uint8_t tc;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        uint8_t tc;
+    );
 };
 
 /* OFPACT_SET_MPLS_TTL.
  *
  * Used for OFPAT11_SET_MPLS_TTL and NXAST_SET_MPLS_TTL */
 struct ofpact_mpls_ttl {
-    struct ofpact ofpact;
-
-    uint8_t ttl;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        uint8_t ttl;
+    );
 };
 
 /* OFPACT_GOTO_TABLE
  *
  * Used for OFPIT11_GOTO_TABLE */
 struct ofpact_goto_table {
-    struct ofpact ofpact;
-    uint8_t table_id;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        uint8_t table_id;
+    );
 };
 
 /* OFPACT_GROUP.
  *
  * Used for OFPAT11_GROUP. */
 struct ofpact_group {
-    struct ofpact ofpact;
-    uint32_t group_id;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        uint32_t group_id;
+    );
 };
 
 /* OFPACT_UNROLL_XLATE.
  *
  * Used only internally. */
 struct ofpact_unroll_xlate {
-    struct ofpact ofpact;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
 
-    /* Metadata in xlate context, visible to controller via PACKET_INs. */
-    uint8_t  rule_table_id;       /* 0xFF if none. */
-    ovs_be64 rule_cookie;         /* OVS_BE64_MAX if none. */
+        /* Metadata in xlate context, visible to controller via PACKET_INs. */
+        uint8_t  rule_table_id; /* 0xFF if none. */
+        ovs_be64 rule_cookie;   /* OVS_BE64_MAX if none. */
+    );
 };
 
 /* OFPACT_ENCAP.
@@ -1001,10 +1070,12 @@  struct ofpact_unroll_xlate {
  * Used for NXAST_ENCAP. */
 
 struct ofpact_encap {
-    struct ofpact ofpact;
-    ovs_be32 new_pkt_type;        /* Packet type of the header to add. */
-    uint16_t hdr_size;            /* New header size in bytes. */
-    uint16_t n_props;             /* Number of encap properties. */
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
+        ovs_be32 new_pkt_type;         /* Packet type of the header to add. */
+        uint16_t hdr_size;             /* New header size in bytes. */
+        uint16_t n_props;              /* Number of encap properties. */
+    );
     struct ofpact_ed_prop props[]; /* Properties in internal format. */
 };
 
@@ -1012,15 +1083,17 @@  struct ofpact_encap {
  *
  * Used for NXAST_DECAP. */
 struct ofpact_decap {
-    struct ofpact ofpact;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
 
-    /* New packet type.
-     *
-     * The special value (0,0xFFFE) "Use next proto" is used to request OVS to
-     * automatically set the new packet type based on the decap'ed header's
-     * next protocol.
-     */
-    ovs_be32 new_pkt_type;
+        /* New packet type.
+         *
+         * The special value (0,0xFFFE) "Use next proto" is used to request OVS
+         * to automatically set the new packet type based on the decap'ed
+         * header's next protocol.
+         */
+        ovs_be32 new_pkt_type;
+    );
 };
 
 /* Converting OpenFlow to ofpacts. */
@@ -1150,21 +1223,23 @@  void *ofpact_finish(struct ofpbuf *, struct ofpact *);
  *
  *     Initializes the parts of 'ofpact' that identify it as having type
  *     OFPACT_<ENUM> and length OFPACT_<ENUM>_SIZE and zeros the rest.
- *
- *   <ENUM>_SIZE
- *
- *     The size of the action structure.  For a fixed-length action, this is
- *     sizeof(struct <STRUCT>) rounded up to a multiple of OFPACT_ALIGNTO.  For
- *     a variable-length action, this is the offset to the variable-length
- *     part.
  */
 #define OFPACT(ENUM, STRUCT, MEMBER, NAME)                              \
     BUILD_ASSERT_DECL(offsetof(struct STRUCT, ofpact) == 0);            \
                                                                         \
-    enum { OFPACT_##ENUM##_SIZE                                         \
-           = (offsetof(struct STRUCT, MEMBER) != 0                      \
-              ? offsetof(struct STRUCT, MEMBER)                         \
-              : OFPACT_ALIGN(sizeof(struct STRUCT))) };                 \
+    /* Action structures must be a multiple of OFPACT_ALIGNTO bytes. */ \
+    BUILD_ASSERT_DECL(sizeof(struct STRUCT) % OFPACT_ALIGNTO == 0);     \
+                                                                        \
+    /* Variable-length data must start at a multiple of OFPACT_ALIGNTO  \
+     * bytes. */                                                        \
+    BUILD_ASSERT_DECL(offsetof(struct STRUCT, MEMBER)                   \
+                      % OFPACT_ALIGNTO == 0);                           \
+                                                                        \
+    /* If there is variable-length data, it starts at the end of the    \
+     * structure. */                                                    \
+    BUILD_ASSERT_DECL(!offsetof(struct STRUCT, MEMBER)                  \
+                      || (offsetof(struct STRUCT, MEMBER)               \
+                          == sizeof(struct STRUCT)));                   \
                                                                         \
     static inline struct STRUCT *                                       \
     ofpact_get_##ENUM(const struct ofpact *ofpact)                      \
@@ -1184,14 +1259,14 @@  void *ofpact_finish(struct ofpbuf *, struct ofpact *);
     ofpact_put_##ENUM(struct ofpbuf *ofpacts)                           \
     {                                                                   \
         return (struct STRUCT *) ofpact_put(ofpacts, OFPACT_##ENUM,     \
-                                            OFPACT_##ENUM##_SIZE);      \
+                                            sizeof(struct STRUCT));     \
     }                                                                   \
                                                                         \
     static inline void                                                  \
     ofpact_init_##ENUM(struct STRUCT *ofpact)                           \
     {                                                                   \
         ofpact_init(&ofpact->ofpact, OFPACT_##ENUM,                     \
-                    OFPACT_##ENUM##_SIZE);                              \
+                    sizeof(struct STRUCT));                             \
     }                                                                   \
                                                                         \
     static inline void                                                  \
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index f25fdec64cc0..96e39d6c6c9c 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -840,7 +840,7 @@  decode_NXAST_RAW_CONTROLLER2(const struct ext_action_header *eah,
         }
 
         case NXAC2PT_USERDATA:
-            out->size = start_ofs + OFPACT_CONTROLLER_SIZE;
+            out->size = start_ofs + sizeof(struct ofpact_controller);
             ofpbuf_put(out, payload.msg, ofpbuf_msgsize(&payload));
             oc = ofpbuf_at_assert(out, start_ofs, sizeof *oc);
             oc->userdata_len = ofpbuf_msgsize(&payload);
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index f5576d50524d..7a7790c0bdbb 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -1939,7 +1939,7 @@  connmgr_flushed(struct connmgr *mgr)
         struct ofpbuf ofpacts;
         struct match match;
 
-        ofpbuf_init(&ofpacts, OFPACT_OUTPUT_SIZE);
+        ofpbuf_init(&ofpacts, sizeof(struct ofpact_output));
         ofpact_put_OUTPUT(&ofpacts)->port = OFPP_NORMAL;
 
         match_init_catchall(&match);
diff --git a/ofproto/fail-open.c b/ofproto/fail-open.c
index ded282895a10..5b105ba880a3 100644
--- a/ofproto/fail-open.c
+++ b/ofproto/fail-open.c
@@ -163,7 +163,7 @@  fail_open_add_normal_flow(struct fail_open *fo)
 
     /* Set up a flow that matches every packet and directs them to
      * OFPP_NORMAL. */
-    ofpbuf_init(&ofpacts, OFPACT_OUTPUT_SIZE);
+    ofpbuf_init(&ofpacts, sizeof(struct ofpact_output));
     ofpact_put_OUTPUT(&ofpacts)->port = OFPP_NORMAL;
 
     match_init_catchall(&match);