From bdebeece558fbeebb87c17b11a8468d88875037d Mon Sep 17 00:00:00 2001 From: Ethan Jackson Date: Mon, 28 Nov 2011 13:54:08 -0800 Subject: [PATCH] lacp: Require successful LACP negotiations when configured. In the original Open vSwitch LACP implementation, when no slaves found a LACP partner, the LACP module would attach all of them. This allowed the LACP bond to fall back to a standard bond when partnered with a non-LACP switch. In practice, this has caused confusion with marginal benefit, so this feature is removed with this patch. Signed-off-by: Ethan Jackson --- NEWS | 2 + lib/bond.c | 91 ++++++++++++++++++++++++------------------ lib/bond.h | 3 +- lib/lacp.c | 30 +++++++------- lib/lacp.h | 8 +++- ofproto/ofproto-dpif.c | 2 +- tests/lacp.at | 2 +- vswitchd/vswitch.xml | 8 ++-- 8 files changed, 85 insertions(+), 61 deletions(-) diff --git a/NEWS b/NEWS index 2249eae3..d7332f89 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,8 @@ post-v1.5.0 ------------------------ - bonding + - LACP bonds no longer fall back to balance-slb when negotiations fail. + Instead they drop traffic. - The default bond_mode changed from SLB to active-backup, to protect unsuspecting users from the significant risks of SLB bonds (which are documented in vswitchd/INTERNALS). diff --git a/lib/bond.c b/lib/bond.c index a2105ca3..50a1d5d0 100644 --- a/lib/bond.c +++ b/lib/bond.c @@ -26,6 +26,7 @@ #include "dynamic-string.h" #include "flow.h" #include "hmap.h" +#include "lacp.h" #include "list.h" #include "netdev.h" #include "odp-util.h" @@ -92,7 +93,7 @@ struct bond { struct bond_slave *active_slave; tag_type no_slaves_tag; /* Tag for flows when all slaves disabled. */ int updelay, downdelay; /* Delay before slave goes up/down, in ms. */ - bool lacp_negotiated; /* LACP negotiations were successful. */ + enum lacp_status lacp_status; /* Status of LACP negotiations. */ bool bond_revalidate; /* True if flows need revalidation. */ uint32_t basis; /* Basis for flow hash function. */ @@ -122,7 +123,6 @@ static void bond_enable_slave(struct bond_slave *, bool enable, struct tag_set *); static void bond_link_status_update(struct bond_slave *, struct tag_set *); static void bond_choose_active_slave(struct bond *, struct tag_set *); -static bool bond_is_tcp_hash(const struct bond *); static unsigned int bond_hash_src(const uint8_t mac[ETH_ADDR_LEN], uint16_t vlan, uint32_t basis); static unsigned int bond_hash_tcp(const struct flow *, uint16_t vlan, @@ -402,13 +402,12 @@ bond_slave_set_may_enable(struct bond *bond, void *slave_, bool may_enable) * * The caller should check bond_should_send_learning_packets() afterward. */ void -bond_run(struct bond *bond, struct tag_set *tags, bool lacp_negotiated) +bond_run(struct bond *bond, struct tag_set *tags, enum lacp_status lacp_status) { struct bond_slave *slave; - bool is_tcp_hash = bond_is_tcp_hash(bond); - if (bond->lacp_negotiated != lacp_negotiated) { - bond->lacp_negotiated = lacp_negotiated; + if (bond->lacp_status != lacp_status) { + bond->lacp_status = lacp_status; bond->bond_revalidate = true; } @@ -427,10 +426,6 @@ bond_run(struct bond *bond, struct tag_set *tags, bool lacp_negotiated) bond->next_fake_iface_update = time_msec() + 1000; } - if (is_tcp_hash != bond_is_tcp_hash(bond)) { - bond->bond_revalidate = true; - } - if (bond->bond_revalidate) { bond->bond_revalidate = false; @@ -488,8 +483,9 @@ bond_wait(struct bond *bond) static bool may_send_learning_packets(const struct bond *bond) { - return !bond->lacp_negotiated && bond->balance != BM_AB && - bond->active_slave; + return bond->lacp_status == LACP_DISABLED + && bond->balance != BM_AB + && bond->active_slave; } /* Returns true if 'bond' needs the client to send out packets to assist with @@ -566,9 +562,14 @@ bond_check_admissibility(struct bond *bond, const void *slave_, * assume the remote switch is aware of the bond and will "do the right * thing". However, as a precaution we drop packets on disabled slaves * because no correctly implemented partner switch should be sending - * packets to them. */ - if (bond->lacp_negotiated) { - return slave->enabled ? BV_ACCEPT : BV_DROP; + * packets to them. + * + * If LACP is configured, but LACP negotiations have been unsuccessful, we + * drop all incoming traffic. */ + switch (bond->lacp_status) { + case LACP_NEGOTIATED: return slave->enabled ? BV_ACCEPT : BV_DROP; + case LACP_CONFIGURED: return BV_DROP; + case LACP_DISABLED: break; } /* Drop all multicast packets on inactive slaves. */ @@ -595,10 +596,11 @@ bond_check_admissibility(struct bond *bond, const void *slave_, return BV_ACCEPT; case BM_TCP: - /* TCP balancing has degraded to SLB (otherwise the - * bond->lacp_negotiated check above would have processed this). - * - * Fall through. */ + /* TCP balanced bonds require successful LACP negotiated. Based on the + * above check, LACP is off on this bond. Therfore, we drop all + * incoming traffic. */ + return BV_DROP; + case BM_SLB: /* 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 @@ -950,11 +952,6 @@ bond_print_details(struct ds *ds, const struct bond *bond) ds_put_format(ds, "bond_mode: %s\n", bond_mode_to_string(bond->balance)); - if (bond->balance != BM_AB) { - ds_put_format(ds, "bond-hash-algorithm: %s\n", - bond_is_tcp_hash(bond) ? "balance-tcp" : "balance-slb"); - } - ds_put_format(ds, "bond-hash-basis: %"PRIu32"\n", bond->basis); ds_put_format(ds, "updelay: %d ms\n", bond->updelay); @@ -965,8 +962,21 @@ bond_print_details(struct ds *ds, const struct bond *bond) bond->next_rebalance - time_msec()); } - ds_put_format(ds, "lacp_negotiated: %s\n", - bond->lacp_negotiated ? "true" : "false"); + ds_put_cstr(ds, "lacp_status: "); + switch (bond->lacp_status) { + case LACP_NEGOTIATED: + ds_put_cstr(ds, "negotiated\n"); + break; + case LACP_CONFIGURED: + ds_put_cstr(ds, "configured\n"); + break; + case LACP_DISABLED: + ds_put_cstr(ds, "off\n"); + break; + default: + ds_put_cstr(ds, "\n"); + break; + } HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) { shash_add(&slave_shash, slave->name, slave); @@ -1305,7 +1315,7 @@ bond_link_status_update(struct bond_slave *slave, struct tag_set *tags) VLOG_INFO_RL(&rl, "interface %s: will not be %s", slave->name, up ? "disabled" : "enabled"); } else { - int delay = (bond->lacp_negotiated ? 0 + int delay = (bond->lacp_status != LACP_DISABLED ? 0 : up ? bond->updelay : bond->downdelay); slave->delay_expires = time_msec() + delay; if (delay) { @@ -1324,13 +1334,6 @@ bond_link_status_update(struct bond_slave *slave, struct tag_set *tags) } } -static bool -bond_is_tcp_hash(const struct bond *bond) -{ - return (bond->balance == BM_TCP && bond->lacp_negotiated) - || bond->balance == BM_STABLE; -} - static unsigned int bond_hash_src(const uint8_t mac[ETH_ADDR_LEN], uint16_t vlan, uint32_t basis) { @@ -1352,9 +1355,9 @@ bond_hash_tcp(const struct flow *flow, uint16_t vlan, uint32_t basis) static unsigned int bond_hash(const struct bond *bond, const struct flow *flow, uint16_t vlan) { - assert(bond->balance != BM_AB); + assert(bond->balance == BM_TCP || bond->balance == BM_SLB); - return (bond_is_tcp_hash(bond) + return (bond->balance == BM_TCP ? bond_hash_tcp(flow, vlan, bond->basis) : bond_hash_src(flow->dl_src, vlan, bond->basis)); } @@ -1381,7 +1384,7 @@ choose_stb_slave(const struct bond *bond, const struct flow *flow, best = NULL; best_hash = 0; - flow_hash = bond_hash(bond, flow, vlan); + flow_hash = bond_hash_tcp(flow, vlan, bond->basis); HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) { if (slave->enabled) { uint32_t hash; @@ -1403,14 +1406,26 @@ choose_output_slave(const struct bond *bond, const struct flow *flow, { struct bond_entry *e; + if (bond->lacp_status == LACP_CONFIGURED) { + /* LACP has been configured on this bond but negotiations were + * unsuccussful. Drop all traffic. */ + return NULL; + } + switch (bond->balance) { case BM_AB: return bond->active_slave; case BM_STABLE: return choose_stb_slave(bond, flow, vlan); - case BM_SLB: + case BM_TCP: + if (bond->lacp_status != LACP_NEGOTIATED) { + /* Must have LACP negotiations for TCP balanced bonds. */ + return NULL; + } + /* Fall Through. */ + case BM_SLB: e = lookup_bond_entry(bond, flow, vlan); if (!e->slave || !e->slave->enabled) { e->slave = CONTAINER_OF(hmap_random_node(&bond->slaves), diff --git a/lib/bond.h b/lib/bond.h index 19580298..9eb1b8fb 100644 --- a/lib/bond.h +++ b/lib/bond.h @@ -26,6 +26,7 @@ struct flow; struct netdev; struct ofpbuf; +enum lacp_status; /* How flows are balanced among bond slaves. */ enum bond_mode { @@ -68,7 +69,7 @@ void bond_slave_register(struct bond *, void *slave_, void bond_slave_set_netdev(struct bond *, void *slave_, struct netdev *); void bond_slave_unregister(struct bond *, const void *slave); -void bond_run(struct bond *, struct tag_set *, bool lacp_negotiated); +void bond_run(struct bond *, struct tag_set *, enum lacp_status); void bond_wait(struct bond *); void bond_slave_set_may_enable(struct bond *, void *slave_, bool may_enable); diff --git a/lib/lacp.c b/lib/lacp.c index 244b86e6..c51fac69 100644 --- a/lib/lacp.c +++ b/lib/lacp.c @@ -301,12 +301,17 @@ lacp_process_packet(struct lacp *lacp, const void *slave_, } } -/* Returns true if 'lacp' has successfully negotiated with its partner. False - * if 'lacp' is NULL. */ -bool -lacp_negotiated(const struct lacp *lacp) +/* Returns the lacp_status of the given 'lacp' object (which may be NULL). */ +enum lacp_status +lacp_status(const struct lacp *lacp) { - return lacp ? lacp->negotiated : false; + if (!lacp) { + return LACP_DISABLED; + } else if (lacp->negotiated) { + return LACP_NEGOTIATED; + } else { + return LACP_CONFIGURED; + } } /* Registers 'slave_' as subordinate to 'lacp'. This should be called at least @@ -384,12 +389,8 @@ lacp_slave_may_enable(const struct lacp *lacp, const void *slave_) struct slave *slave = slave_lookup(lacp, slave_); /* The slave may be enabled if it's attached to an aggregator and its - * partner is synchronized. The only exception is defaulted slaves. - * They are not required to have synchronized partners because they - * have no partners at all. They will only be attached if negotiations - * failed on all slaves in the bond. */ - return slave->attached && (slave->partner.state & LACP_STATE_SYNC - || slave->status == LACP_DEFAULTED); + * partner is synchronized.*/ + return slave->attached && (slave->partner.state & LACP_STATE_SYNC); } else { return true; } @@ -504,14 +505,13 @@ lacp_update_attached(struct lacp *lacp) HMAP_FOR_EACH (slave, node, &lacp->slaves) { struct lacp_info pri; - slave->attached = true; + slave->attached = false; /* XXX: In the future allow users to configure the expected system ID. * For now just special case loopback. */ if (eth_addr_equals(slave->partner.sys_id, slave->lacp->sys_id)) { VLOG_WARN_RL(&rl, "slave %s: Loopback detected. Slave is " "connected to its own bond", slave->name); - slave->attached = false; continue; } @@ -519,6 +519,7 @@ lacp_update_attached(struct lacp *lacp) continue; } + slave->attached = true; slave_get_priority(slave, &pri); if (!lead || memcmp(&pri, &lead_pri, sizeof pri) < 0) { @@ -531,8 +532,7 @@ lacp_update_attached(struct lacp *lacp) if (lead) { HMAP_FOR_EACH (slave, node, &lacp->slaves) { - if (slave->status == LACP_DEFAULTED - || lead->partner.key != slave->partner.key + if (lead->partner.key != slave->partner.key || !eth_addr_equals(lead->partner.sys_id, slave->partner.sys_id)) { slave->attached = false; diff --git a/lib/lacp.h b/lib/lacp.h index 1d717d60..293fc454 100644 --- a/lib/lacp.h +++ b/lib/lacp.h @@ -29,6 +29,12 @@ enum lacp_time { LACP_TIME_CUSTOM /* Nonstandard custom mode. */ }; +enum lacp_status { + LACP_NEGOTIATED, /* Successful LACP negotations. */ + LACP_CONFIGURED, /* LACP is enabled but not negotiated. */ + LACP_DISABLED /* LACP is not enabled. */ +}; + struct lacp_settings { char *name; /* Name (for debugging). */ uint8_t id[ETH_ADDR_LEN]; /* System ID. Must be nonzero. */ @@ -48,7 +54,7 @@ bool lacp_is_active(const struct lacp *); void lacp_process_packet(struct lacp *, const void *slave, const struct ofpbuf *packet); -bool lacp_negotiated(const struct lacp *); +enum lacp_status lacp_status(const struct lacp *); struct lacp_slave_settings { char *name; /* Name (for debugging). */ diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 381ffaf8..7ded85d3 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -1879,7 +1879,7 @@ bundle_run(struct ofbundle *bundle) } bond_run(bundle->bond, &bundle->ofproto->revalidate_set, - lacp_negotiated(bundle->lacp)); + lacp_status(bundle->lacp)); if (bond_should_send_learning_packets(bundle->bond)) { bundle_send_learning_packets(bundle); } diff --git a/tests/lacp.at b/tests/lacp.at index c10e05fc..543aa257 100644 --- a/tests/lacp.at +++ b/tests/lacp.at @@ -103,7 +103,7 @@ bond_mode: active-backup bond-hash-basis: 0 updelay: 0 ms downdelay: 0 ms -lacp_negotiated: true +lacp_status: negotiated slave p1: disabled may_enable: false diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index f3226e92..8c506e4f 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -757,8 +757,7 @@

The following modes require the upstream switch to support 802.3ad with - successful LACP negotiation. If LACP negotiation fails then - balance-slb style flow hashing is used as a fallback: + successful LACP negotiation:

@@ -861,8 +860,9 @@ connected to. active ports are allowed to initiate LACP negotiations. passive ports are allowed to participate in LACP negotiations initiated by a remote switch, but not allowed to - initiate such negotiations themselves. Defaults to off - if unset. + initiate such negotiations themselves. If LACP is enabled on a port + whose partner switch does not support LACP, the bond will be + disabled. Defaults to off if unset. -- 2.30.2