On Thu, Sep 08, 2016 at 11:51:10PM -0500, Chandra Sekhar Vejendla wrote:
>
>
>
> Chandra Sekhar Vejendla/San Jose/IBM@IBMUS wrote on 09/08/2016 11:31:54 PM:
>
> > From: Chandra Sekhar Vejendla/San Jose/IBM@IBMUS
> > To: dev@openvswitch.org
> > Cc: Chandra Sekhar Vejendla/San Jose/IBM@IBMUS, Ryan
> Moats/Omaha/IBM@IBMUS
> > Date: 09/08/2016 11:32 PM
> > Subject: [PATCH v2] ovn-controller: Fix match crieria for dynamic
> > mac binding flows
> >
> > match struct is not initialized before adding flows for each entry in
> > mac_bindings table. This results in incorrect match criteria.
> >
> > Signed-off-by: Chandra Sekhar Vejendla <csvej...@us.ibm.com>
> > Signed-off-by: Ryan Moats <rmo...@us.ibm.com>
> > Co-authored-by: Ryan Moats <rmo...@us.ibm.com>
> > ---
> > ovn/controller/lflow.c | 31 ++++++++++++++-----------------
> > 1 file changed, 14 insertions(+), 17 deletions(-)
> >
> > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> > index efac5b3..3ea7fc7 100644
> > --- a/ovn/controller/lflow.c
> > +++ b/ovn/controller/lflow.c
> > @@ -343,10 +343,13 @@ put_load(const uint8_t *data, size_t len,
> > static void
> > consider_neighbor_flow(const struct lport_index *lports,
> > const struct sbrec_mac_binding *b,
> > - struct ofpbuf *ofpacts_p,
> > - struct match *match_p,
> > struct hmap *flow_table)
> > {
> > + struct ofpbuf ofpacts;
> > + struct match match;
> > + match_init_catchall(&match);
> > + ofpbuf_init(&ofpacts, 0);
> > +
> > const struct sbrec_port_binding *pb
> > = lport_lookup_by_name(lports, b->logical_port);
> > if (!pb) {
> > @@ -368,7 +371,7 @@ consider_neighbor_flow(const struct lport_index
> *lports,
> > VLOG_WARN_RL(&rl, "bad 'ip' %s", b->ip);
> > return;
> > }
> > - match_set_reg(match_p, 0, ntohl(ip));
> > + match_set_reg(&match, 0, ntohl(ip));
> > } else {
> > struct in6_addr ip6;
> > if (!ipv6_parse(b->ip, &ip6)) {
> > @@ -378,16 +381,17 @@ consider_neighbor_flow(const struct lport_index
> *lports,
> > }
> > ovs_be128 value;
> > memcpy(&value, &ip6, sizeof(value));
> > - match_set_xxreg(match_p, 0, ntoh128(value));
> > + match_set_xxreg(&match, 0, ntoh128(value));
> > }
> >
> > - match_set_metadata(match_p, htonll(pb->datapath->tunnel_key));
> > - match_set_reg(match_p, MFF_LOG_OUTPORT - MFF_REG0, pb->tunnel_key);
> > + match_set_metadata(&match, htonll(pb->datapath->tunnel_key));
> > + match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0, pb->tunnel_key);
> >
> > - ofpbuf_clear(ofpacts_p);
> > - put_load(mac.ea, sizeof mac.ea, MFF_ETH_DST, 0, 48, ofpacts_p);
> > + ofpbuf_clear(&ofpacts);
> > + put_load(mac.ea, sizeof mac.ea, MFF_ETH_DST, 0, 48, &ofpacts);
> >
> > - ofctrl_add_flow(flow_table, OFTABLE_MAC_BINDING, 100, match_p,
> > ofpacts_p);
> > + ofctrl_add_flow(flow_table, OFTABLE_MAC_BINDING, 100, &match,
> &ofpacts);
> > + ofpbuf_uninit(&ofpacts);
> > }
> >
> > /* Adds an OpenFlow flow to flow tables for each MAC binding in the OVN
> > @@ -398,17 +402,10 @@ add_neighbor_flows(struct controller_ctx *ctx,
> > const struct lport_index *lports,
> > struct hmap *flow_table)
> > {
> > - struct ofpbuf ofpacts;
> > - struct match match;
> > - match_init_catchall(&match);
> > - ofpbuf_init(&ofpacts, 0);
> > -
> > const struct sbrec_mac_binding *b;
> > SBREC_MAC_BINDING_FOR_EACH (b, ctx->ovnsb_idl) {
> > - consider_neighbor_flow(lports, b, &ofpacts, &match, flow_table);
> > + consider_neighbor_flow(lports, b, flow_table);
> > }
> > -
> > - ofpbuf_uninit(&ofpacts);
> > }
> >
> > /* Translates logical flows in the Logical_Flow table in the OVN_SB
> database
> > --
> > 1.9.1
> >
>
> I haven't been able to come up with a valid test case that would fail
> without
> this patch. For the unit test, I tried to add multiple mac_bindings with
> ovn-sbctl command and then verify that the flows generated should have only
> reg0 and reg15 in the match criteria. When i tested this in a real setup,
> without the patch, reg1, reg2 and reg3 will sometimes show up in the match
> criteria along with reg0 and reg15. This is not 100% reproducible, though.
I think it's clear enough what the patch fixes.
I reworked the code a little bit and applied it to master and branch-2.6
as follows:
--8<--------------------------cut here-------------------------->8--
From: Chandra S Vejendla <csvej...@us.ibm.com>
Date: Thu, 8 Sep 2016 23:31:54 -0500
Subject: [PATCH] ovn-controller: Fix match crieria for dynamic mac binding
flows
match struct is not initialized before adding flows for each entry in
mac_bindings table. The matches for IPv4 and IPv6 entries don't have
exactly the same form (IPv4 uses reg0, IPv6 uses xxreg0), so reusing
a match structure can cause problems.
Signed-off-by: Chandra Sekhar Vejendla <csvej...@us.ibm.com>
Signed-off-by: Ryan Moats <rmo...@us.ibm.com>
Co-authored-by: Ryan Moats <rmo...@us.ibm.com>
Signed-off-by: Ben Pfaff <b...@ovn.org>
---
ovn/controller/lflow.c | 30 +++++++++++-------------------
1 file changed, 11 insertions(+), 19 deletions(-)
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index efac5b3..0414626 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -343,8 +343,6 @@ put_load(const uint8_t *data, size_t len,
static void
consider_neighbor_flow(const struct lport_index *lports,
const struct sbrec_mac_binding *b,
- struct ofpbuf *ofpacts_p,
- struct match *match_p,
struct hmap *flow_table)
{
const struct sbrec_port_binding *pb
@@ -360,7 +358,7 @@ consider_neighbor_flow(const struct lport_index *lports,
return;
}
-
+ struct match match = MATCH_CATCHALL_INITIALIZER;
if (strchr(b->ip, '.')) {
ovs_be32 ip;
if (!ip_parse(b->ip, &ip)) {
@@ -368,7 +366,7 @@ consider_neighbor_flow(const struct lport_index *lports,
VLOG_WARN_RL(&rl, "bad 'ip' %s", b->ip);
return;
}
- match_set_reg(match_p, 0, ntohl(ip));
+ match_set_reg(&match, 0, ntohl(ip));
} else {
struct in6_addr ip6;
if (!ipv6_parse(b->ip, &ip6)) {
@@ -378,16 +376,17 @@ consider_neighbor_flow(const struct lport_index *lports,
}
ovs_be128 value;
memcpy(&value, &ip6, sizeof(value));
- match_set_xxreg(match_p, 0, ntoh128(value));
+ match_set_xxreg(&match, 0, ntoh128(value));
}
- match_set_metadata(match_p, htonll(pb->datapath->tunnel_key));
- match_set_reg(match_p, MFF_LOG_OUTPORT - MFF_REG0, pb->tunnel_key);
-
- ofpbuf_clear(ofpacts_p);
- put_load(mac.ea, sizeof mac.ea, MFF_ETH_DST, 0, 48, ofpacts_p);
+ match_set_metadata(&match, htonll(pb->datapath->tunnel_key));
+ match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0, pb->tunnel_key);
- ofctrl_add_flow(flow_table, OFTABLE_MAC_BINDING, 100, match_p, ofpacts_p);
+ uint64_t stub[1024 / 8];
+ struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub);
+ put_load(mac.ea, sizeof mac.ea, MFF_ETH_DST, 0, 48, &ofpacts);
+ ofctrl_add_flow(flow_table, OFTABLE_MAC_BINDING, 100, &match, &ofpacts);
+ ofpbuf_uninit(&ofpacts);
}
/* Adds an OpenFlow flow to flow tables for each MAC binding in the OVN
@@ -398,17 +397,10 @@ add_neighbor_flows(struct controller_ctx *ctx,
const struct lport_index *lports,
struct hmap *flow_table)
{
- struct ofpbuf ofpacts;
- struct match match;
- match_init_catchall(&match);
- ofpbuf_init(&ofpacts, 0);
-
const struct sbrec_mac_binding *b;
SBREC_MAC_BINDING_FOR_EACH (b, ctx->ovnsb_idl) {
- consider_neighbor_flow(lports, b, &ofpacts, &match, flow_table);
+ consider_neighbor_flow(lports, b, flow_table);
}
-
- ofpbuf_uninit(&ofpacts);
}
/* Translates logical flows in the Logical_Flow table in the OVN_SB database
--
2.1.3
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
