[ovs-git] Open vSwitch: ofproto-dpif: Fix tag caching for learned flows. (master)

Wed Mar 21 16:01:49 UTC 2012

This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "Open vSwitch".

The branch, master has been updated
       via  33780682c5530ed4811763e0a1d7e3562d73f51e (commit)
       via  805e4d07ecd8ce86c6f36bdb83d2e7fee56891c7 (commit)
       via  e39e5b9d9d19f933cae177471dc0ebbc7e0041e7 (commit)
       via  822d941452da65dd067ab1fe19a7fc8f181558b5 (commit)
       via  9ed20b8a36555a39e3cda9900c6adc0e2f5ae2c2 (commit)
      from  5341d04613a359b949279ce3f54f1a5bd7731999 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
commit 33780682c5530ed4811763e0a1d7e3562d73f51e
Diffs: http://openvswitch.org/cgi-bin/gitweb.cgi?p=openvswitch;a=commitdiff;h=33780682c5530ed4811763e0a1d7e3562d73f51e
Author: Ben Pfaff <blp at nicira.com>

ofproto-dpif: Fix tag caching for learned flows.

This code in xlate_table_action() is supposed to tag flows in tables that
have special forms so that changes do not require revalidating every flow.
When rule->tag is nonzero, its value can be used, because we know in this
case that rule->cr.wc is the same as table->other_table->wc and that thus
rule->tag caches the return value of the rule_calculate_tag() expression.
When rule->tag is zero (a "catchall" rule) we need to calculate the tag
manually because we have no way to cache it in that case.

I discovered this bug by running an "hping3" between a couple of VMs plus
the following commands on OVS in the middle:

    ovs-ofctl del-flows br0
    ovs-ofctl add-flow br0 "table=0 actions=learn(table=1, \
              idle_timeout=600, NXM_OF_VLAN_TCI[0..11], \
              NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[], \
              output:NXM_OF_IN_PORT[], fin_idle_timeout=10), resubmit(,1)"
    ovs-ofctl add-flow br0 "table=1 priority=0 actions=flood"

Without this patch, flows don't get properly invalidated upon initial MAC
learning, so one sees warnings like the following:

    in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),
    eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,
    ttl=64,frag=no),tcp(src=13966,dst=0): inconsistency in subfacet
    (actions were: 3,0,1) (correct actions: 1)

This patch fixes the problem and thus avoids these warnings.

Signed-off-by: Ben Pfaff <blp at nicira.com>

commit 805e4d07ecd8ce86c6f36bdb83d2e7fee56891c7
Diffs: http://openvswitch.org/cgi-bin/gitweb.cgi?p=openvswitch;a=commitdiff;h=805e4d07ecd8ce86c6f36bdb83d2e7fee56891c7
Author: Ben Pfaff <blp at nicira.com>

ofproto-dpif: Avoid segfault deleting facets that execute LEARN actions.

"ovs-ofctl del-flows <bridge>" can result in the following call path:

  delete_flows_loose() in ofproto.c
    -> collect_rules_loose() -- uses 'ofproto_node' inside 'struct rule'
    -> rule_destruct() in ofproto-dpif.c
      -> facet_revalidate()
        -> facet_remove()
          -> facet_flush_stats()
            -> facet_account()
              -> xlate_actions()
                -> xlate_learn_action()
                  -> ofproto_flow_mod() back in ofproto.c
                    -> modify_flow_strict()
                      -> collect_rules_strict() -- also uses 'ofproto_node'

which goes "boom" when we fall back up the call chain because the nested
use of ofproto_node steps on the outer use of ofproto_node.

This commit fixes the problem by refusing to translate "learn" actions
within facet_flush_stats(), breaking the doubled use.

Another possible approach would be to switch to another way to keep track
of rules in the flow_mod implementations, so that there'd be no fighting
over 'ofproto_node'.  But then "ovs-ofctl del-flows" might still leave some
flows around (ones created by "learn" actions as flows are accounted as
facets get deleted), which would be surprising behavior.  And it seems in
general a bad idea to allow recursive flow_mods; the consequences have not
been carefully thought through.

Before this commit, one can reproduce the problem by running an "hping3"
between a couple of VMs plus the following commands on OVS in the middle.
Sometimes you have to run them a few times:

    ovs-ofctl del-flows br0
    ovs-ofctl add-flow br0 "table=0 actions=learn(table=1, \
              idle_timeout=600, NXM_OF_VLAN_TCI[0..11], \
              NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[], \
              output:NXM_OF_IN_PORT[], fin_idle_timeout=10), resubmit(,1)"
    ovs-ofctl add-flow br0 "table=1 priority=0 actions=flood"

This commit has a side effect that leftover unaccounted packets no longer
update the timeouts in MAC learning actions in some cases, when the facets
that cause updates are deleted.  At most one second of updates should  be
lost.

Bug #10184.
Reported-by: Michael Mao <mmao at nicira.com>
Signed-off-by: Ben Pfaff <blp at nicira.com>

commit e39e5b9d9d19f933cae177471dc0ebbc7e0041e7
Diffs: http://openvswitch.org/cgi-bin/gitweb.cgi?p=openvswitch;a=commitdiff;h=e39e5b9d9d19f933cae177471dc0ebbc7e0041e7
Author: Ben Pfaff <blp at nicira.com>

hmap: New function hmap_contains().

This is useful in a situation where one knows that an hmap_node is in some
hmap, but it's not certain which one, and one needs to know whether it is
in a particular one.  This is not a very common case; I don't see any
potential users in the current tree, although an upcoming commit will add
one.

Signed-off-by: Ben Pfaff <blp at nicira.com>

commit 822d941452da65dd067ab1fe19a7fc8f181558b5
Diffs: http://openvswitch.org/cgi-bin/gitweb.cgi?p=openvswitch;a=commitdiff;h=822d941452da65dd067ab1fe19a7fc8f181558b5
Author: Ben Pfaff <blp at nicira.com>

ofproto-dpif: Fix return type of rule_calculate_tag().

tag_type is currently uint32_t but using uint32_t directly is conceptually
wrong.

Signed-off-by: Ben Pfaff <blp at nicira.com>

commit 9ed20b8a36555a39e3cda9900c6adc0e2f5ae2c2
Diffs: http://openvswitch.org/cgi-bin/gitweb.cgi?p=openvswitch;a=commitdiff;h=9ed20b8a36555a39e3cda9900c6adc0e2f5ae2c2
Author: Ben Pfaff <blp at nicira.com>

learn: Initialize cookie_mask in constructed flow_mod.

Otherwise the "learn" action may not correctly set the cookie in flows that
it creates.

Found by valgrind.

Signed-off-by: Ben Pfaff <blp at nicira.com>

-----------------------------------------------------------------------

Summary of changes:
 lib/hmap.c             |   17 ++++++++++++++++-
 lib/hmap.h             |    4 +++-
 lib/learn.c            |    1 +
 ofproto/ofproto-dpif.c |   40 ++++++++++++++++++++++++----------------
 4 files changed, 44 insertions(+), 18 deletions(-)

hooks/post-receive
-- 
Open vSwitch