Thx for the review,
Forgot to mention, that I'll add (both in code and in commit log) that this
will not fix the case where the recv_bridge is not the same as
recirc_bridge, and rules matching in_port are installed on recirc_bridge.
(since in that case, the in_port is set to OFPP_NONE)
Thanks,
Alex Wang,
On Mon, Dec 22, 2014 at 11:39 AM, Andy Zhou <az...@nicira.com> wrote:
> Acked-by: Andy Zhou <az...@nicira.com>
>
> On Sun, Dec 21, 2014 at 9:58 AM, Alex Wang <al...@nicira.com> wrote:
> > After commit 0c7812e5e (recirculation: Do not drop packet when
> > there is no match from internal table.), if flow keys are modified
> > before the recirculation action (e.g. set vlan ID), the miss
> > handling of recirc'ed packets may not reach the intended
> > 'ofproto_dpif' which has rules looking up the 'recirc_id's,
> > causing drops.
> >
> > This commit adds an unittest that captures this bug. Moreover,
> > to solve this bug, this commit checks mapping between 'recirc_id'
> > and the corresponding 'ofproto_dpif', and makes sure that the
> > miss handling of recirc'ed packets are done with the correct
> > 'ofproto_dpif'.
> >
> > Signed-off-by: Alex Wang <al...@nicira.com>
> >
> > ---
> > PATCH->V2:
> > - move the recirc_ofproto check to xlate_lookup_ofproto_().
> > - use VLOG_ERR to notify recirc_id leak.
> > - add check for recirc_id_alloc failure.
> > - clarify the unittest.
> > ---
> > ofproto/ofproto-dpif-xlate.c | 48 +++++++++++++++++++---------
> > ofproto/ofproto-dpif.c | 71
> ++++++++++++++++++++++++++++++++++++++++--
> > ofproto/ofproto-dpif.h | 2 ++
> > tests/ofproto-dpif.at | 58 ++++++++++++++++++++++++++++++++++
> > 4 files changed, 162 insertions(+), 17 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 4cef550..cee6d32 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -980,18 +980,41 @@ static struct ofproto_dpif *
> > xlate_lookup_ofproto_(const struct dpif_backer *backer, const struct
> flow *flow,
> > ofp_port_t *ofp_in_port, const struct xport
> **xportp)
> > {
> > + struct ofproto_dpif *recv_ofproto = NULL;
> > + struct ofproto_dpif *recirc_ofproto = NULL;
> > const struct xport *xport;
> > + ofp_port_t in_port = OFPP_NONE;
> >
> > *xportp = xport = xlate_lookup_xport(backer, flow);
> >
> > if (xport) {
> > - if (ofp_in_port) {
> > - *ofp_in_port = xport->ofp_port;
> > + recv_ofproto = xport->xbridge->ofproto;
> > + in_port = xport->ofp_port;
> > + }
> > +
> > + /* When recirc_id is set in 'flow', checks whether the ofproto_dpif
> that
> > + * corresponds to the recirc_id is same as the receiving bridge.
> If they
> > + * are the same, uses the 'recv_ofproto' and keeps the
> 'ofp_in_port' as
> > + * assigned. Otherwise, uses the 'recirc_ofproto' that owns
> recirc_id and
> > + * assigns OFPP_NONE to 'ofp_in_port'. Doing this is in that, for
> the
> > + * latter case, post recirculation flow are not allowed to cross a
> patch
> > + * port, and the rules that match the recirculated packets are
> installed
> > + * only to the 'recirc_ofproto'.
> > + */
> > + if (recv_ofproto && flow->recirc_id) {
> > + recirc_ofproto = ofproto_dpif_recirc_get_ofproto(backer,
> > +
> flow->recirc_id);
> > + if (recv_ofproto != recirc_ofproto) {
> > + *xportp = xport = NULL;
> > + in_port = OFPP_NONE;
> > }
> > - return xport->xbridge->ofproto;
> > }
> >
> > - return NULL;
> > + if (ofp_in_port) {
> > + *ofp_in_port = in_port;
> > + }
> > +
> > + return xport ? recv_ofproto : recirc_ofproto;
> > }
> >
> > /* Given a datapath and flow metadata ('backer', and 'flow'
> respectively)
> > @@ -1012,9 +1035,7 @@ xlate_lookup_ofproto(const struct dpif_backer
> *backer, const struct flow *flow,
> > * pointers until quiescing, for longer term use additional references
> must
> > * be taken.
> > *
> > - * '*ofp_in_port' is set to OFPP_NONE if 'flow''s in_port does not
> exist.
> > - *
> > - * Returns 0 if successful, ENODEV if the parsed flow has no associated
> ofport.
> > + * Returns 0 if successful, ENODEV if the parsed flow has no associated
> ofproto.
> > */
> > int
> > xlate_lookup(const struct dpif_backer *backer, const struct flow *flow,
> > @@ -1027,11 +1048,7 @@ xlate_lookup(const struct dpif_backer *backer,
> const struct flow *flow,
> >
> > ofproto = xlate_lookup_ofproto_(backer, flow, ofp_in_port, &xport);
> >
> > - if (ofp_in_port && !xport) {
> > - *ofp_in_port = OFPP_NONE;
> > - }
> > -
> > - if (!xport) {
> > + if (!ofproto) {
> > return ENODEV;
> > }
> >
> > @@ -1040,16 +1057,17 @@ xlate_lookup(const struct dpif_backer *backer,
> const struct flow *flow,
> > }
> >
> > if (ipfix) {
> > - *ipfix = xport->xbridge->ipfix;
> > + *ipfix = xport ? xport->xbridge->ipfix : NULL;
> > }
> >
> > if (sflow) {
> > - *sflow = xport->xbridge->sflow;
> > + *sflow = xport ? xport->xbridge->sflow : NULL;
> > }
> >
> > if (netflow) {
> > - *netflow = xport->xbridge->netflow;
> > + *netflow = xport ? xport->xbridge->netflow : NULL;
> > }
> > +
> > return 0;
> > }
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 81c2c6d..fd73e37 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -251,6 +251,13 @@ COVERAGE_DEFINE(rev_flow_table);
> > COVERAGE_DEFINE(rev_mac_learning);
> > COVERAGE_DEFINE(rev_mcast_snooping);
> >
> > +/* Stores mapping between 'recirc_id' and 'ofproto-dpif'. */
> > +struct dpif_backer_recirc_node {
> > + struct cmap_node cmap_node;
> > + struct ofproto_dpif *ofproto;
> > + uint32_t recirc_id;
> > +};
> > +
> > /* All datapaths of a given type share a single dpif backer instance. */
> > struct dpif_backer {
> > char *type;
> > @@ -269,6 +276,8 @@ struct dpif_backer {
> >
> > /* Recirculation. */
> > struct recirc_id_pool *rid_pool; /* Recirculation ID pool. */
> > + struct cmap recirc_map; /* Map of 'recirc_id's to
> 'ofproto's. */
> > + struct ovs_mutex recirc_mutex; /* Protects 'recirc_map'. */
> > bool enable_recirc; /* True if the datapath supports
> recirculation */
> >
> > /* True if the datapath supports unique flow identifiers */
> > @@ -844,6 +853,26 @@ dealloc(struct ofproto *ofproto_)
> > free(ofproto);
> > }
> >
> > +/* Called when 'ofproto' is destructed. Clears all the 'recirc_id's
> > + * owned by 'ofproto'. */
> > +static void
> > +dpif_backer_recirc_clear_ofproto(struct dpif_backer *backer,
> > + struct ofproto_dpif *ofproto)
> > +{
> > + struct dpif_backer_recirc_node *node;
> > +
> > + ovs_mutex_lock(&backer->recirc_mutex);
> > + CMAP_FOR_EACH (node, cmap_node, &backer->recirc_map) {
> > + if (node->ofproto == ofproto) {
> > + VLOG_ERR("recirc_id %"PRIu32", not freed when ofproto (%s) "
> > + "is destructed", node->recirc_id, ofproto->up.name
> );
> > + cmap_remove(&backer->recirc_map, &node->cmap_node,
> > + node->recirc_id);
> > + }
> > + }
> > + ovs_mutex_unlock(&backer->recirc_mutex);
> > +}
> > +
> > static void
> > close_dpif_backer(struct dpif_backer *backer)
> > {
> > @@ -860,6 +889,8 @@ close_dpif_backer(struct dpif_backer *backer)
> > hmap_destroy(&backer->odp_to_ofport_map);
> > shash_find_and_delete(&all_dpif_backers, backer->type);
> > recirc_id_pool_destroy(backer->rid_pool);
> > + cmap_destroy(&backer->recirc_map);
> > + ovs_mutex_destroy(&backer->recirc_mutex);
> > free(backer->type);
> > free(backer->dp_version_string);
> > dpif_close(backer->dpif);
> > @@ -975,6 +1006,8 @@ open_dpif_backer(const char *type, struct
> dpif_backer **backerp)
> > backer->masked_set_action = check_masked_set_action(backer);
> > backer->enable_ufid = check_ufid(backer);
> > backer->rid_pool = recirc_id_pool_create();
> > + ovs_mutex_init(&backer->recirc_mutex);
> > + cmap_init(&backer->recirc_map);
> >
> > backer->enable_tnl_push_pop =
> dpif_supports_tnl_push_pop(backer->dpif);
> > atomic_count_init(&backer->tnl_count, 0);
> > @@ -1423,6 +1456,8 @@ destruct(struct ofproto *ofproto_)
> > }
> > guarded_list_destroy(&ofproto->pins);
> >
> > + dpif_backer_recirc_clear_ofproto(ofproto->backer, ofproto);
> > +
> > mbridge_unref(ofproto->mbridge);
> >
> > netflow_unref(ofproto->netflow);
> > @@ -5376,20 +5411,52 @@ odp_port_to_ofp_port(const struct ofproto_dpif
> *ofproto, odp_port_t odp_port)
> > }
> > }
> >
> > +struct ofproto_dpif *
> > +ofproto_dpif_recirc_get_ofproto(const struct dpif_backer *backer,
> > + uint32_t recirc_id)
> > +{
> > + struct dpif_backer_recirc_node *node;
> > +
> > + node = CONTAINER_OF(cmap_find(&backer->recirc_map, recirc_id),
> > + struct dpif_backer_recirc_node, cmap_node);
> > +
> > + return node ? node->ofproto : NULL;
> > +}
> > +
> > uint32_t
> > ofproto_dpif_alloc_recirc_id(struct ofproto_dpif *ofproto)
> > {
> > struct dpif_backer *backer = ofproto->backer;
> > + uint32_t recirc_id = recirc_id_alloc(backer->rid_pool);
> >
> > - return recirc_id_alloc(backer->rid_pool);
> > + if (recirc_id) {
> > + struct dpif_backer_recirc_node *node = xmalloc(sizeof *node);
> > +
> > + node->recirc_id = recirc_id;
> > + node->ofproto = ofproto;
> > +
> > + ovs_mutex_lock(&backer->recirc_mutex);
> > + cmap_insert(&backer->recirc_map, &node->cmap_node,
> node->recirc_id);
> > + ovs_mutex_unlock(&backer->recirc_mutex);
> > + }
> > +
> > + return recirc_id;
> > }
> >
> > void
> > ofproto_dpif_free_recirc_id(struct ofproto_dpif *ofproto, uint32_t
> recirc_id)
> > {
> > struct dpif_backer *backer = ofproto->backer;
> > + struct dpif_backer_recirc_node *node;
> >
> > - recirc_id_free(backer->rid_pool, recirc_id);
> > + node = CONTAINER_OF(cmap_find(&backer->recirc_map, recirc_id),
> > + struct dpif_backer_recirc_node, cmap_node);
> > + if (node) {
> > + ovs_mutex_lock(&backer->recirc_mutex);
> > + cmap_remove(&backer->recirc_map, &node->cmap_node,
> node->recirc_id);
> > + ovs_mutex_unlock(&backer->recirc_mutex);
> > + recirc_id_free(backer->rid_pool, node->recirc_id);
> > + }
> > }
> >
> > int
> > diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> > index 1170c33..13a47cf 100644
> > --- a/ofproto/ofproto-dpif.h
> > +++ b/ofproto/ofproto-dpif.h
> > @@ -219,6 +219,8 @@ struct ofport_dpif *odp_port_to_ofport(const struct
> dpif_backer *, odp_port_t);
> > * work the same way as regular flows.
> > */
> >
> > +struct ofproto_dpif *ofproto_dpif_recirc_get_ofproto(const struct
> dpif_backer *ofproto,
> > + uint32_t
> recirc_id);
> > uint32_t ofproto_dpif_alloc_recirc_id(struct ofproto_dpif *ofproto);
> > void ofproto_dpif_free_recirc_id(struct ofproto_dpif *ofproto, uint32_t
> recirc_id);
> > int ofproto_dpif_add_internal_flow(struct ofproto_dpif *,
> > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> > index 54a5209..55c3e90 100644
> > --- a/tests/ofproto-dpif.at
> > +++ b/tests/ofproto-dpif.at
> > @@ -144,6 +144,64 @@ AT_CHECK([ovs-appctl dpif/dump-flows br1 |grep tcp
> > br1_flows.txt])
> > AT_CHECK([test `grep in_port.4 br1_flows.txt |wc -l` -gt 24])
> > AT_CHECK([test `grep in_port.5 br1_flows.txt |wc -l` -gt 24])
> > AT_CHECK([test `grep in_port.6 br1_flows.txt |wc -l` -gt 24])
> > +
> > +OVS_VSWITCHD_STOP()
> > +AT_CLEANUP
> > +
> > +# Makes sure recirculation does not change the way packet is handled.
> > +AT_SETUP([ofproto-dpif, balance-tcp bonding, different recirc flow ])
> > +OVS_VSWITCHD_START(
> > + [add-bond br0 bond0 p1 p2 bond_mode=balance-tcp lacp=active \
> > + other-config:lacp-time=fast
> other-config:bond-rebalance-interval=0 -- \
> > + set interface p1 type=dummy
> options:pstream=punix:$OVS_RUNDIR/p1.sock ofport_request=1 -- \
> > + set interface p2 type=dummy
> options:pstream=punix:$OVS_RUNDIR/p2.sock ofport_request=2 -- \
> > + add-br br1 -- \
> > + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \
> > + set bridge br1 datapath-type=dummy other-config:datapath-id=1234 \
> > + fail-mode=standalone -- \
> > + add-bond br1 bond1 p3 p4 bond_mode=balance-tcp lacp=active \
> > + other-config:lacp-time=fast
> other-config:bond-rebalance-interval=0 -- \
> > + set interface p3 type=dummy options:stream=unix:$OVS_RUNDIR/p1.sock
> ofport_request=3 -- \
> > + set interface p4 type=dummy options:stream=unix:$OVS_RUNDIR/p2.sock
> ofport_request=4 -- \
> > + add-port br1 br1- -- set interface br1- type=patch options:peer=br1+
> ofport_request=100 -- \
> > + add-br br-int -- \
> > + set bridge br-int other-config:hwaddr=aa:77:aa:77:00:00 -- \
> > + set bridge br-int datapath-type=dummy other-config:datapath-id=1235 \
> > + fail-mode=secure -- \
> > + add-port br-int br1+ -- set interface br1+ type=patch
> options:peer=br1- ofport_request=101 -- \
> > + add-port br-int p5 -- set interface p5 ofport_request=5 type=dummy
> > +])
> > +AT_CHECK([ovs-appctl netdev-dummy/set-admin-state up], 0, [OK
> > +])
> > +
> > +# The dl_vlan flow should not be ever matched,
> > +# since recirculation should not change the flow handling.
> > +AT_DATA([flows.txt], [dnl
> > +table=0 priority=1 in_port=5 actions=mod_vlan_vid:1,output(101)
> > +table=0 priority=2 in_port=5 dl_vlan=1 actions=drop
> > +])
> > +AT_CHECK([ovs-ofctl add-flows br-int flows.txt])
> > +
> > +# Sends a packet to trigger recirculation.
> > +# Should generate recirc_id(0x12d),dp_hash(0xc1261ba2/0xff).
> > +AT_CHECK([ovs-appctl netdev-dummy/receive p5
> "in_port(5),eth(src=50:54:00:00:00:05,dst=50:54:00:00:01:00),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1)"])
> > +
> > +# Collects flow stats.
> > +AT_CHECK([ovs-appctl revalidator/purge], [0])
> > +
> > +# Checks the flow stats in br1, should only be one flow with non-zero
> > +# 'n_packets' from internal table.
> > +AT_CHECK([ovs-appctl bridge/dump-flows br1 | ofctl_strip | grep --
> "n_packets" | grep -- "table_id" | sed -e 's/output:[[0-9]]\+/output/'],
> [0], [dnl
> > +table_id=254, n_packets=1, n_bytes=64,
> priority=20,recirc_id=0x12d,dp_hash=0xa2/0xff,actions=output
> > +])
> > +
> > +# Checks the flow stats in br-int, should be only one match.
> > +AT_CHECK([ovs-ofctl dump-flows br-int | ofctl_strip | sort], [0], [dnl
> > + n_packets=1, n_bytes=60, priority=1,in_port=5
> actions=mod_vlan_vid:1,output:101
> > + priority=2,in_port=5,dl_vlan=1 actions=drop
> > +NXST_FLOW reply:
> > +])
> > +
> > OVS_VSWITCHD_STOP()
> > AT_CLEANUP
> >
> > --
> > 1.7.9.5
> >
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
