Re: [ovs-dev] [PATCH] learn: Fix iteration over learning specs.
Thanks, applied to master and branch-2.6.
On Fri, Sep 02, 2016 at 03:49:59PM -0700, Jarno Rajahalme wrote:
> Good catch (== thanks for finding an embarrassing bug to mine :-)
>
> Acked-by: Jarno Rajahalme
>
> > On Sep 2, 2016, at 1:26 PM, Ben Pfaff 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
> > ---
> > 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->mask
Re: [ovs-dev] [PATCH] learn: Fix iteration over learning specs.
Good catch (== thanks for finding an embarrassing bug to mine :-)
Acked-by: Jarno Rajahalme
> On Sep 2, 2016, at 1:26 PM, Ben Pfaff 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
> ---
> 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 *l
