Good catch (== thanks for finding an embarrassing bug to mine :-)
Acked-by: Jarno Rajahalme <ja...@ovn.org>
> On Sep 2, 2016, at 1:26 PM, Ben Pfaff <b...@ovn.org> wrote:
>
> 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 <b...@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
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
