netdev - Re: [PATCH net] bonding: fix active-backup transition after link failure

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF2d9jhg1jb1=AKzYNDC2bu43zx8Ob-exDxk6190LKZnPBDaqQ@mail.gmail.com>
Date:   Wed, 11 Dec 2019 12:10:43 -0800
From:   Mahesh Bandewar (महेश बंडेवार) 
        <maheshb@...gle.com>
To:     Jay Vosburgh <jay.vosburgh@...onical.com>
Cc:     Andy Gospodarek <andy@...yhouse.net>,
        Veaceslav Falico <vfalico@...il.com>,
        David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>,
        Mahesh Bandewar <mahesh@...dewar.net>
Subject: Re: [PATCH net] bonding: fix active-backup transition after link failure

On Mon, Dec 9, 2019 at 10:41 AM Mahesh Bandewar (महेश बंडेवार)
<maheshb@...gle.com> wrote:
>
> On Sat, Dec 7, 2019 at 2:09 PM Jay Vosburgh <jay.vosburgh@...onical.com> wrote:
> >
> > Mahesh Bandewar <maheshb@...gle.com> wrote:
> >
> > >After the recent fix 1899bb325149 ("bonding: fix state transition
> > >issue in link monitoring"), the active-backup mode with miimon
> > >initially come-up fine but after a link-failure, both members
> > >transition into backup state.
> > >
> > >Following steps to reproduce the scenario (eth1 and eth2 are the
> > >slaves of the bond):
> > >
> > >    ip link set eth1 up
> > >    ip link set eth2 down
> > >    sleep 1
> > >    ip link set eth2 up
> > >    ip link set eth1 down
> > >    cat /sys/class/net/eth1/bonding_slave/state
> > >    cat /sys/class/net/eth2/bonding_slave/state
> > >
> > >Fixes: 1899bb325149 ("bonding: fix state transition issue in link monitoring")
> > >CC: Jay Vosburgh <jay.vosburgh@...onical.com>
> > >Signed-off-by: Mahesh Bandewar <maheshb@...gle.com>
> > >---
> > > drivers/net/bonding/bond_main.c | 3 ---
> > > 1 file changed, 3 deletions(-)
> > >
> > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > >index fcb7c2f7f001..ad9906c102b4 100644
> > >--- a/drivers/net/bonding/bond_main.c
> > >+++ b/drivers/net/bonding/bond_main.c
> > >@@ -2272,9 +2272,6 @@ static void bond_miimon_commit(struct bonding *bond)
> > >                       } else if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
> > >                               /* make it immediately active */
> > >                               bond_set_active_slave(slave);
> > >-                      } else if (slave != primary) {
> > >-                              /* prevent it from being the active one */
> > >-                              bond_set_backup_slave(slave);
> >
> >         How does this fix things?  Doesn't bond_select_active_slave() ->
> > bond_change_active_slave() set the backup flag correctly via a call to
> > bond_set_slave_active_flags() when it sets a slave to be the active
> > slave?  If this change resolves the problem, I'm not sure how this ever
> > worked correctly, even prior to 1899bb325149.
> >
> Hi Jay, I used kprobes to figure out the brokenness this patch fixes.
> Prior to your patch this call would not happen but with the patch,
> this extra call will put the master into the backup mode erroneously
> (in fact both members would be in backup state). The mechanics you
> have mentioned works correctly except that in the prior case, the
> switch statement was using new_link which was not same as
> link_new_state. The miimon_inspect will update new_link which is what
> was used in miimon_commit code. The link_new_state was used only to
> mitigate the rtnl-lock issue which would update the "link". Hence in
> the prior code, this path would never get executed.
>
> The steps to reproduce this issue is straightforward and happens 100%
> of the time (I used two mlx interfaces but that shouldn't matter).
>
A friendly ping.

> thanks,
> --mahesh..
> >         -J
> >
> > >                       }
> > >
> > >                       slave_info(bond->dev, slave->dev, "link status definitely up, %u Mbps %s duplex\n",
> > >--
> > >2.24.0.393.g34dc348eaf-goog
> > >
> >
> > ---
> >         -Jay Vosburgh, jay.vosburgh@...onical.com