[ovs-dev] [PATCH] bond: Fix using uninitialized 'lacp_fallback_ab_cfg' for 'bond-primary'.

[Ilya Maximets]

On 10/13/20 2:58 PM, Jeff Squyres (jsquyres) wrote:
> On Oct 13, 2020, at 6:02 AM, Ilya Maximets <i.maximets at 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 at 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 at cisco.com>

Thanks, Alin and Jeff!

Applied to master and branch-2.14.

Best regards, Ilya Maximets.