| Message ID | 20201013100210.2119857-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] bond: Fix using uninitialized 'lacp_fallback_ab_cfg' for 'bond-primary'. | expand |
Acked-by: Alin Gabriel Serdean aserdean@cloudbasesolutions.com<mailto:aserdean@cloudbasesolutions.com> From: Ilya Maximets<mailto:i.maximets@ovn.org> Sent: Tuesday, October 13, 2020 1:02 PM To: ovs-dev@openvswitch.org<mailto:ovs-dev@openvswitch.org>; Jeff Squyres<mailto:jsquyres@cisco.com> Cc: Flavio Leitner<mailto:fbl@sysclose.org>; Ilya Maximets<mailto:i.maximets@ovn.org> Subject: [ovs-dev] [PATCH] bond: Fix using uninitialized 'lacp_fallback_ab_cfg' for 'bond-primary'. '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 <i.maximets@ovn.org> --- 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"); + } 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 + <ref column="other_config" key="lacp-fallback-ab"/> is + <code>true</code>). </column> <group title="Link Failure Detection"> -- 2.25.4
On Oct 13, 2020, at 6:02 AM, Ilya Maximets <i.maximets@ovn.org> 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 <i.maximets@ovn.org> > --- > 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 <jsquyres@cisco.com>
On 10/13/20 2:58 PM, Jeff Squyres (jsquyres) wrote: > On Oct 13, 2020, at 6:02 AM, Ilya Maximets <i.maximets@ovn.org> 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 <i.maximets@ovn.org> >> --- >> 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 ..." This seems like a US-specific punctuation rule, interesting. But I changed it. > >> + <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 <jsquyres@cisco.com> Thanks, Alin and Jeff! Applied to master and branch-2.14. Best regards, Ilya Maximets.
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"); + } 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 + <ref column="other_config" key="lacp-fallback-ab"/> is + <code>true</code>). </column> <group title="Link Failure Detection">
'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 <i.maximets@ovn.org> --- vswitchd/bridge.c | 9 ++++----- vswitchd/vswitch.xml | 5 ++++- 2 files changed, 8 insertions(+), 6 deletions(-)