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 <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
