[ovs-dev] [PATCH 4/4] bridge: Don't learn from inadmissible flows when revising learning table.

Justin Pettit jpettit at nicira.com
Fri Apr 16 02:12:50 UTC 2010


Thanks for tracking down this nasty bug!  Looks good.

--Justin


On Apr 15, 2010, at 5:11 PM, Ben Pfaff wrote:

> Various kinds of flows are inadmissible and must be dropped.  Most notably,
> OVS drops packets received on a bond whose destinations are ones that OVS
> has already learned on a different port.  As the comment says:
> 
>         /* Drop all packets for which we have learned a different input
>          * port, because we probably sent the packet on one slave and got
>          * it back on the other.  Broadcast ARP replies are an exception
>          * to this rule: the host has moved to another switch. */
> 
> As an important side effect of dropping these packets, OVS does not use
> them for MAC learning when it sets up the corresponding flows.
> 
> However, OVS also periodically scans the datapath flow table and uses
> information about flow activity to update its learning tables.  (Otherwise,
> learning table entries could expire because no new flows were being set up,
> even though active flows existed.)  This process, implemented in
> bridge_account_flow_ofhook_cb(), did not check for admissibility, so
> packets received on a bond could be used for learning even though another
> port had already been learned.
> 
> This commit fixes the problem by making bridge_account_flow_ofhook_cb()
> check for admissibility.
> 
> QA notes: Reproducing this problem requires some care and some luck.  One
> way is to have two VMs with network interfaces on a single bonded network.
> Both bonded interfaces must be up (otherwise packets sent out on one slave
> will never be received on the other).  The problem will also not occur if
> the physical switch that the bond slaves are plugged into has learned the
> MAC address of the VMs involved (because the physical switch will then,
> again, drop the packets without sending them back in on the other slave).
> Finally, there needs to be some luck in timing and perhaps with the OVS
> internal hash function also.
> 
> (One way to reproduce it reliably is to plug a pair of Ethernet ports into
> each other with a cable, without an intermediate switch, and then use that
> pair of ports as a bond.  Then every packet sent out on one will
> immediately be received on the other, triggering the problem fairly often.
> If this doesn't work at first, try changing the Ethernet address used on
> one side or the other.)
> 
> To verify that the problem being observed is the one fixed by this commit,
> turn on bridge debugging with "ovs-appctl vlog/set bridge:file" and look
> for "bridge xapi2: learned that 00:01:02:03:04:06 is on port bond0 in VLAN
> 0" where 00:01:02:03:04:06 is a VM's Ethernet address and bond0 is the
> name of the bond in the ovs-vswitchd log file.
> 
> Testing: I ran the "loopback bond" test above with and without this commit,
> twice each in case I was just lucky.
> 
> CC: Henrik Amren <henrik at nicira.com>
> 
> Bug #2366.
> Bug NIC-64.
> Bug NIC-69.
> ---
> vswitchd/bridge.c |   12 +++++-------
> 1 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index d33944a..2d60525 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -2380,18 +2380,16 @@ bridge_account_flow_ofhook_cb(const flow_t *flow,
>                               void *br_)
> {
>     struct bridge *br = br_;
> -    struct port *in_port;
>     const union odp_action *a;
> +    struct port *in_port;
> +    tag_type tags = 0;
> +    int vlan;
> 
>     /* Feed information from the active flows back into the learning table
>      * to ensure that table is always in sync with what is actually flowing
>      * through the datapath. */
> -    in_port = port_from_dp_ifidx(br, flow->in_port);
> -    if (in_port) {
> -        int vlan = flow_get_vlan(br, flow, in_port, false);
> -         if (vlan >= 0) {
> -            update_learning_table(br, flow, vlan, in_port);
> -        }
> +    if (is_admissible(br, flow, false, &tags, &vlan, &in_port)) {
> +        update_learning_table(br, flow, vlan, in_port);
>     }
> 
>     if (!br->has_bonded_ports) {
> -- 
> 1.6.6.1
> 
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev_openvswitch.org





More information about the dev mailing list