[ovs-dev] [PATCH] learn: Fix iteration over learning specs.

[Ben Pfaff]

struct ofpact_learn_spec is variable-length.  The 'n_specs' member of
struct ofpact_learn counted the number of specs, but the iteration loops
over struct ofpact_learn_spec only iterated as far as the *minimum* length
of 'n_specs' specs.

This fixes the problem, which exhibited as consistent failures for test 431
(learning action - TCPv6 port learning), seemingly only on i386 since it
shows up for my personal development machine but appears to not happen for
anyone else.

Fixes: dfe191d5faa6 ("ofp-actions: Waste less memory in learn actions.")
Signed-off-by: Ben Pfaff <blp at ovn.org>
---
 include/openvswitch/ofp-actions.h | 33 +++++++++++++++++++++++----------
 lib/learn.c                       | 13 ++++---------
 lib/ofp-actions.c                 |  4 +---
 3 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h
index bf7d62f..6759201 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -744,21 +744,34 @@ ofpact_learn_spec_next(const struct ofpact_learn_spec *spec)
  *
  * Used for NXAST_LEARN. */
 struct ofpact_learn {
-    struct ofpact ofpact;
+    OFPACT_PADDED_MEMBERS(
+        struct ofpact ofpact;
 
-    uint16_t idle_timeout;      /* Idle time before discarding (seconds). */
-    uint16_t hard_timeout;      /* Max time before discarding (seconds). */
-    uint16_t priority;          /* Priority level of flow entry. */
-    uint8_t table_id;           /* Table to insert flow entry. */
-    enum nx_learn_flags flags;  /* NX_LEARN_F_*. */
-    ovs_be64 cookie;            /* Cookie for new flow. */
-    uint16_t fin_idle_timeout;  /* Idle timeout after FIN, if nonzero. */
-    uint16_t fin_hard_timeout;  /* Hard timeout after FIN, if nonzero. */
+        uint16_t idle_timeout;     /* Idle time before discarding (seconds). */
+        uint16_t hard_timeout;     /* Max time before discarding (seconds). */
+        uint16_t priority;         /* Priority level of flow entry. */
+        uint8_t table_id;          /* Table to insert flow entry. */
+        enum nx_learn_flags flags; /* NX_LEARN_F_*. */
+        ovs_be64 cookie;           /* Cookie for new flow. */
+        uint16_t fin_idle_timeout; /* Idle timeout after FIN, if nonzero. */
+        uint16_t fin_hard_timeout; /* Hard timeout after FIN, if nonzero. */
+    );
 
-    unsigned int n_specs;
     struct ofpact_learn_spec specs[];
 };
 
+static inline const struct ofpact_learn_spec *
+ofpact_learn_spec_end(const struct ofpact_learn *learn)
+{
+    return ALIGNED_CAST(const struct ofpact_learn_spec *,
+                        ofpact_next(&learn->ofpact));
+}
+
+#define OFPACT_LEARN_SPEC_FOR_EACH(SPEC, LEARN) \
+    for ((SPEC) = (LEARN)->specs;               \
+         (SPEC) < ofpact_learn_spec_end(LEARN); \
+         (SPEC) = ofpact_learn_spec_next(SPEC))
+
 /* Multipath link choice algorithm to apply.
  *
  * In the descriptions below, 'n_links' is max_link + 1. */
diff --git a/lib/learn.c b/lib/learn.c
index bb57f7d..9cab759 100644
--- a/lib/learn.c
+++ b/lib/learn.c
@@ -41,8 +41,7 @@ learn_check(const struct ofpact_learn *learn, const struct flow *flow)
     struct match match;
 
     match_init_catchall(&match);
-    for (spec = learn->specs; spec < &learn->specs[learn->n_specs];
-         spec = ofpact_learn_spec_next(spec)) {
+    OFPACT_LEARN_SPEC_FOR_EACH (spec, learn) {
         enum ofperr error;
 
         /* Check the source. */
@@ -123,8 +122,7 @@ learn_execute(const struct ofpact_learn *learn, const struct flow *flow,
         oft->fin_hard_timeout = learn->fin_hard_timeout;
     }
 
-    for (spec = learn->specs; spec < &learn->specs[learn->n_specs];
-         spec = ofpact_learn_spec_next(spec)) {
+    OFPACT_LEARN_SPEC_FOR_EACH (spec, learn) {
         struct ofpact_set_field *sf;
         union mf_subvalue value;
 
@@ -179,8 +177,7 @@ learn_mask(const struct ofpact_learn *learn, struct flow_wildcards *wc)
     union mf_subvalue value;
 
     memset(&value, 0xff, sizeof value);
-    for (spec = learn->specs; spec < &learn->specs[learn->n_specs];
-         spec = ofpact_learn_spec_next(spec)) {
+    OFPACT_LEARN_SPEC_FOR_EACH (spec, learn) {
         if (spec->src_type == NX_LEARN_SRC_FIELD) {
             mf_write_subfield_flow(&spec->src, &value, &wc->masks);
         }
@@ -386,7 +383,6 @@ learn_parse__(char *orig, char *arg, struct ofpbuf *ofpacts)
                 return error;
             }
             learn = ofpacts->header;
-            learn->n_specs++;
         }
     }
     ofpact_finish_LEARN(ofpacts, &learn);
@@ -459,8 +455,7 @@ learn_format(const struct ofpact_learn *learn, struct ds *s)
                       colors.param, colors.end, ntohll(learn->cookie));
     }
 
-    for (spec = learn->specs; spec < &learn->specs[learn->n_specs];
-         spec = ofpact_learn_spec_next(spec)) {
+    OFPACT_LEARN_SPEC_FOR_EACH (spec, learn) {
         unsigned int n_bytes = DIV_ROUND_UP(spec->n_bits, 8);
         ds_put_char(s, ',');
 
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index f27ff21..22c7b16 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -4321,7 +4321,6 @@ decode_NXAST_RAW_LEARN(const struct nx_action_learn *nal,
 
         spec = ofpbuf_put_zeros(ofpacts, sizeof *spec);
         learn = ofpacts->header;
-        learn->n_specs++;
 
         spec->src_type = header & NX_LEARN_SRC_MASK;
         spec->dst_type = header & NX_LEARN_DST_MASK;
@@ -4421,8 +4420,7 @@ encode_LEARN(const struct ofpact_learn *learn,
     nal->flags = htons(learn->flags);
     nal->table_id = learn->table_id;
 
-    for (spec = learn->specs; spec < &learn->specs[learn->n_specs];
-         spec = ofpact_learn_spec_next(spec)) {
+    OFPACT_LEARN_SPEC_FOR_EACH (spec, learn) {
         put_u16(out, spec->n_bits | spec->dst_type | spec->src_type);
 
         if (spec->src_type == NX_LEARN_SRC_FIELD) {
-- 
2.1.3