netdev - Re: [PATCH] bonding: Do not try to send packets over dead link in TLB mode.

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-next>] [day] [month] [year] [list]

Message-ID: <1404925785.3515.30.camel@edumazet-glaptop2.roam.corp.google.com>
Date:	Wed, 09 Jul 2014 19:09:45 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Mahesh Bandewar <maheshb@...gle.com>
Cc:	Jay Vosburgh <fubar@...ibm.com>,
	Veaceslav Falico <vfalico@...hat.com>,
	Andy Gospodarek <andy@...yhouse.net>,
	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Maciej Zenczykowski <maze@...gle.com>
Subject: Re: [PATCH] bonding: Do not try to send packets over dead link in
 TLB mode.

On Wed, 2014-07-09 at 09:34 -0700, Mahesh Bandewar wrote:
> 

Please make sure to not send HTML message on netdev Mahesh, your message
was not delivered to the list.
> 
> On Wed, Jul 9, 2014 at 12:10 AM, Eric Dumazet <eric.dumazet@...il.com>
> wrote:
>         On Tue, 2014-07-08 at 18:09 -0700, Mahesh Bandewar wrote:
>         > In TLB mode if tlb_dynamic_lb is NOT set, slaves from the
>         bond
>         > group are selected based on the hash distribution. This does
>         not
>         > exclude dead links which are part of the bond. Also if there
>         is a
>         > temporary link event which brings down the interface,
>         packets
>         > hashed on that interface would be dropped too.
>         >
>         > This patch fixes these issues and distributes flows across
>         the
>         > UP links only. Also the array construction of links which
>         are
>         > capable of sending packets happen in the control path
>         leaving
>         > only link-selection duing the data-path.
>         
>         
>         s/duing/during/
>         
>         Seems a speed improvement as well for bonding of 8 slaves ;)
>          
>         
>         >
>         > One possible side effect of this is - at a link event; all
>         > flows will be shuffled to get good distribution. But impact
>         of
>         > this should be minimum with the assumption that a member or
>         > members of the bond group are not available is a very
>         temporary
>         > situation.
>         >
>         > Signed-off-by: Mahesh Bandewar <maheshb@...gle.com>
>         > ---
>         >  drivers/net/bonding/bond_alb.c | 52
>         +++++++++++++++++++++++++++++++++++++-----
>         >  drivers/net/bonding/bond_alb.h | 11 +++++++++
>         >  drivers/net/bonding/bonding.h  |  6 +++++
>         >  3 files changed, 63 insertions(+), 6 deletions(-)
>         >
>         > diff --git a/drivers/net/bonding/bond_alb.c
>         b/drivers/net/bonding/bond_alb.c
>         > index 76c0dade233f..1f39d41fde4b 100644
>         > --- a/drivers/net/bonding/bond_alb.c
>         > +++ b/drivers/net/bonding/bond_alb.c
>         > @@ -195,6 +195,9 @@ static int tlb_initialize(struct bonding
>         *bond)
>         >
>         >       _unlock_tx_hashtbl_bh(bond);
>         >
>         > +     /* Initialize the TLB array spin-lock */
>         > +     spin_lock_init(&bond_info->slave_arr_lock);
>         > +
>         >       return 0;
>         >  }
>         >
>         > @@ -209,6 +212,9 @@ static void tlb_deinitialize(struct
>         bonding *bond)
>         >       bond_info->tx_hashtbl = NULL;
>         >
>         >       _unlock_tx_hashtbl_bh(bond);
>         > +
>         > +     if (bond_is_nondyn_tlb(bond) && bond_info->slave_arr)
>         > +             kfree_rcu(bond_info->slave_arr, rcu);
>         
>         
>         You could remove the first condition, as slave_arr being NULL
>         or not is
>         enough to take the decision to call kfree_rcu()
>         
>         I do not know if a the "bond_is_nondyn_tlb(bond)" can change
>         over the
>         time for a given bonding device, so feel uncomfortable with a
>         possible
>         memleak here.
>         
> It can not change while the bond device is up. It checks for the
> bonding mode and the parameter tlb_dynamic_lb and none of them can be
> changed without bringing down the bond. 
> 
> 
> I wanted to add it as a safe-guard against trying to free this array
> in other mode if there are some random bytes present (which is
> unlikely but having an extra check to full-proof is not going to hurt
> was the thinking behind it). 
> 
Well, field is guaranteed to be NULL at bonding init time, otherwise you
would crash at first kfree_rcu()




--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html