On Oct 13, 2020, at 6:02 AM, Ilya Maximets <[email protected]> wrote:
>
> 's->lacp_fallback_ab_cfg' initialized down below in the code, so
> we're using it uninitialized to detect if we need to get 'bond-primary'
> configuration.
>
> Found by valgrind:
>
> Conditional jump or move depends on uninitialised value(s)
> at 0x409114: port_configure_bond (bridge.c:4569)
> by 0x409114: port_configure (bridge.c:1284)
> by 0x40F6E6: bridge_reconfigure (bridge.c:917)
> by 0x411425: bridge_run (bridge.c:3330)
> by 0x406D84: main (ovs-vswitchd.c:127)
> Uninitialised value was created by a stack allocation
> at 0x408C53: port_configure (bridge.c:1190)
>
> Fix that by moving this code to the point where 'lacp_fallback_ab_cfg'
> already initialized. Additionally clarified behavior of 'bond-primary'
> in manpages for the fallback to AB case.
>
> Fixes: b4e50218a0f8 ("bond: Add 'primary' interface concept for active-backup
> mode.")
> Signed-off-by: Ilya Maximets <[email protected]>
> ---
> vswitchd/bridge.c | 9 ++++-----
> vswitchd/vswitch.xml | 5 ++++-
> 2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index a3e7facd3..a332517bc 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -4565,11 +4565,6 @@ port_configure_bond(struct port *port, struct
> bond_settings *s)
> port->name);
> }
>
> - s->primary = NULL;
> - if (s->balance == BM_AB || s->lacp_fallback_ab_cfg) {
> - s->primary = smap_get(&port->cfg->other_config, "bond-primary");
> - }
> -
> miimon_interval = smap_get_int(&port->cfg->other_config,
> "bond-miimon-interval", 0);
> if (miimon_interval <= 0) {
> @@ -4596,6 +4591,10 @@ port_configure_bond(struct port *port, struct
> bond_settings *s)
>
> s->lacp_fallback_ab_cfg = smap_get_bool(&port->cfg->other_config,
> "lacp-fallback-ab", false);
> + s->primary = NULL;
> + if (s->balance == BM_AB || s->lacp_fallback_ab_cfg) {
> + s->primary = smap_get(&port->cfg->other_config, "bond-primary");
> + }
Excellent catch.
> LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
> netdev_set_miimon_interval(iface->netdev, miimon_interval);
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 07da2ee8c..20decb2db 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -2008,7 +2008,10 @@
> If a slave interface with this name exists in the bond and
> is up, it will be made active. Relevant only when <ref
> column="other_config" key="bond_mode"/> is
> - <code>active-backup</code>.
> + <code>active-backup</code> or if <code>balance-tcp</code> falls back
> + to <code>active-backup</code> (e.g. LACP negotiation fails and
Super minor nit: "e.g." should be followed by a comma -- "e.g., LACP
negotiation ..."
> + <ref column="other_config" key="lacp-fallback-ab"/> is
> + <code>true</code>).
> </column>
>
> <group title="Link Failure Detection">
> --
> 2.25.4
Acked-by: Jeff Squyres <[email protected]>
--
Jeff Squyres
[email protected]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
