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  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]
Date:	Sat, 12 Jul 2014 10:17:22 -0700
From:	Mahesh Bandewar <maheshb@...gle.com>
To:	Nikolay Aleksandrov <nikolay@...hat.com>
Cc:	Jay Vosburgh <j.vosburgh@...il.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: [PATCHv2] bonding: Do not try to send packets over dead link in
 TLB mode.

On Sat, Jul 12, 2014 at 1:44 AM, Nikolay Aleksandrov <nikolay@...hat.com> wrote:
> On 07/11/2014 11:15 PM, Mahesh Bandewar wrote:
>> In my tests with added instrumentation (similar to __might_sleep()) in
>> the update_arr function; the calls were never in the automic context
>> so did not change the GFP flag to GFP_ATOMIC.
>>
>> On Fri, Jul 11, 2014 at 2:10 PM, Mahesh Bandewar <maheshb@...gle.com> 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 during the data-path.
>>>
>>> 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 | 44 +++++++++++++++++++++++++++++++++++++-----
>>>  drivers/net/bonding/bond_alb.h |  8 ++++++++
>>>  drivers/net/bonding/bonding.h  |  6 ++++++
>>>  3 files changed, 53 insertions(+), 5 deletions(-)
>>>
>>> [v1]
>>>   * Initial patch
>>> [v2]
>>>   * Removed spinlock protection
>>>   * Removed explicit rcu_read_(un)lock() calls since Xmit fn run in RCU
>>>   * Updated condition during bond-dismantle to check for only arrays' existance.
>>>
>>> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>>> index 76c0dade233f..7fb2066432b1 100644
>>> --- a/drivers/net/bonding/bond_alb.c
>>> +++ b/drivers/net/bonding/bond_alb.c
>>> @@ -209,6 +209,9 @@ static void tlb_deinitialize(struct bonding *bond)
>>>         bond_info->tx_hashtbl = NULL;
>>>
>>>         _unlock_tx_hashtbl_bh(bond);
>>> +
>>> +       if (bond_info->slave_arr)
>>> +               kfree_rcu(bond_info->slave_arr, rcu);
>>>  }
>>>
>>>  static long long compute_gap(struct slave *slave)
>>> @@ -1406,9 +1409,35 @@ out:
>>>         return NETDEV_TX_OK;
>>>  }
>>>
>>> +static int bond_tlb_update_slave_arr(struct bonding *bond)
>>> +{
>>> +       struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>>> +       struct slave *tx_slave;
>>> +       struct list_head *iter;
>>> +       struct tlb_up_slave *new_arr, *old_arr;
>>> +
>>> +       new_arr = kzalloc(offsetof(struct tlb_up_slave, arr[bond->slave_cnt]),
>>> +                         GFP_KERNEL);
>>> +       if (!new_arr)
>>> +               return -ENOMEM;
>>> +
>>> +       bond_for_each_slave(bond, tx_slave, iter) {
>>> +               if (bond_slave_can_tx(tx_slave))
>>> +                       new_arr->arr[new_arr->count++] = tx_slave;
>>> +       }
>>> +
>>> +       old_arr = bond_info->slave_arr;
>>> +       rcu_assign_pointer(bond_info->slave_arr, new_arr);
>>> +       if (old_arr)
>>> +               kfree_rcu(old_arr, rcu);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>>  int bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>>>  {
>>>         struct bonding *bond = netdev_priv(bond_dev);
>>> +       struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>>>         struct ethhdr *eth_data;
>>>         struct slave *tx_slave = NULL;
>>>         u32 hash_index;
>>> @@ -1429,12 +1458,12 @@ int bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>>>                                                               hash_index & 0xFF,
>>>                                                               skb->len);
>>>                         } else {
>>> -                               struct list_head *iter;
>>> -                               int idx = hash_index % bond->slave_cnt;
>>> +                               struct tlb_up_slave *slaves;
>>>
>>> -                               bond_for_each_slave_rcu(bond, tx_slave, iter)
>>> -                                       if (--idx < 0)
>>> -                                               break;
>>> +                               slaves = rcu_dereference(bond_info->slave_arr);
>>> +                               if (slaves && slaves->count)
>>> +                                       tx_slave = slaves->arr[hash_index %
>>> +                                                              slaves->count];
>>>                         }
>>>                         break;
>>>                 }
>>> @@ -1721,6 +1750,11 @@ void bond_alb_handle_link_change(struct bonding *bond, struct slave *slave, char
>>>                          */
>>>                 }
>>>         }
>>> +
>>> +       if (bond_is_nondyn_tlb(bond)) {
>>> +               if (bond_tlb_update_slave_arr(bond))
>>> +                       pr_err("Failed to build slave-array for TLB mode.\n");
>>> +       }
>>>  }
>>>
>>>  /**
>>> diff --git a/drivers/net/bonding/bond_alb.h b/drivers/net/bonding/bond_alb.h
>>> index 5fc76c01636c..aaeac61d03cf 100644
>>> --- a/drivers/net/bonding/bond_alb.h
>>> +++ b/drivers/net/bonding/bond_alb.h
>>> @@ -139,12 +139,20 @@ struct tlb_slave_info {
>>>                          */
>>>  };
>>>
>>> +struct tlb_up_slave {
>>> +       unsigned int    count;
>>> +       struct rcu_head rcu;
>>> +       struct slave    *arr[0];
>>> +};
>>> +
>>>  struct alb_bond_info {
>>>         struct tlb_client_info  *tx_hashtbl; /* Dynamically allocated */
>>>         spinlock_t              tx_hashtbl_lock;
>>>         u32                     unbalanced_load;
>>>         int                     tx_rebalance_counter;
>>>         int                     lp_counter;
>>> +       /* -------- non-dynamic tlb mode only ---------*/
>>> +       struct tlb_up_slave __rcu *slave_arr;     /* Up slaves */
>>>         /* -------- rlb parameters -------- */
>>>         int rlb_enabled;
>>>         struct rlb_client_info  *rx_hashtbl;    /* Receive hash table */
>>> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>>> index 0b4d9cde0b05..ad9b3dce07d8 100644
>>> --- a/drivers/net/bonding/bonding.h
>>> +++ b/drivers/net/bonding/bonding.h
>>> @@ -265,6 +265,12 @@ static inline bool bond_is_lb(const struct bonding *bond)
>>>                BOND_MODE(bond) == BOND_MODE_ALB;
>>>  }
>>>
>>> +static inline bool bond_is_nondyn_tlb(const struct bonding *bond)
>>> +{
>>> +       return (BOND_MODE(bond) == BOND_MODE_TLB)  &&
>>> +              (bond->params.tlb_dynamic_lb == 0);
>>> +}
>>> +
>>>  static inline bool bond_mode_uses_arp(int mode)
>>>  {
>>>         return mode != BOND_MODE_8023AD && mode != BOND_MODE_TLB &&
>>> --
>>> 2.0.0.526.g5318336
>>>
>> --
>> 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
>>
>
> Did you have CONFIG_DEBUG_ATOMIC_SLEEP in your kernel config ?
>
Yes I do have that enabled (along with few other)!

CONFIG_DEBUG_SPINLOCK=y
CONFIG_DEBUG_MUTEXES=y
# CONFIG_DEBUG_WW_MUTEX_SLOWPATH is not set
CONFIG_DEBUG_LOCK_ALLOC=y
CONFIG_PROVE_LOCKING=y
CONFIG_LOCKDEP=y
# CONFIG_LOCK_STAT is not set
# CONFIG_DEBUG_LOCKDEP is not set
CONFIG_DEBUG_ATOMIC_SLEEP=y
# CONFIG_DEBUG_LOCKING_API_SELFTESTS is not set
# CONFIG_LOCK_TORTURE_TEST is not set
CONFIG_TRACE_IRQFLAGS=y
CONFIG_STACKTRACE=y
# CONFIG_DEBUG_KOBJECT is not set
CONFIG_DEBUG_BUGVERBOSE=y
CONFIG_DEBUG_LIST=y

Is there something else that is required that I missed?

> [  556.669144] BUG: sleeping function called from invalid context at
> mm/slub.c:965
> [  556.670262] in_atomic(): 1, irqs_disabled(): 0, pid: 3316, name: bash
> [  556.670939] CPU: 1 PID: 3316 Comm: bash Tainted: G           OE
> 3.16.0-rc2+ #3
> [  556.670941] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
> [  556.670943]  0000000000000000 ffff88005b60fc70 ffffffff816af012
> ffff8800490cb900
> [  556.670957]  ffff88005b60fc80 ffffffff8109c30a ffff88005b60fcc0
> ffffffff811b2283
> [  556.670961]  ffffffffa01f45ae ffff8800490cb900 0000000000000000
> ffff880049d9ac00
> [  556.670965] Call Trace:
> [  556.670975]  [<ffffffff816af012>] dump_stack+0x4d/0x66
> [  556.670981]  [<ffffffff8109c30a>] __might_sleep+0xca/0x100
> [  556.670987]  [<ffffffff811b2283>] __kmalloc+0x63/0x270
> [  556.670996]  [<ffffffffa01f45ae>] ?
> bond_alb_handle_link_change+0x7e/0x180 [bonding]
> [  556.671010]  [<ffffffffa01f45ae>] bond_alb_handle_link_change+0x7e/0x180
> [bonding]
> [  556.671068]  [<ffffffffa01eac62>] bond_change_active_slave+0x3b2/0x620
> [bonding]
> [  556.671076]  [<ffffffffa01eaf94>] bond_select_active_slave+0xc4/0x1a0
> [bonding]
> [  556.671082]  [<ffffffffa01ec00d>] bond_enslave+0xf9d/0xfc0 [bonding]
> [  556.671089]  [<ffffffffa01f70df>] bond_option_slaves_set+0xff/0x130
> [bonding]
> [  556.671095]  [<ffffffffa01f83ff>] __bond_opt_set+0xdf/0x330 [bonding]
> [  556.671100]  [<ffffffffa01f8697>] bond_opt_tryset_rtnl+0x47/0x80 [bonding]
> [  556.671106]  [<ffffffffa01f52e5>] bonding_sysfs_store_option+0x35/0x60
> [bonding]
> [  556.671113]  [<ffffffff8141e6f8>] dev_attr_store+0x18/0x30
> [  556.671118]  [<ffffffff812435ad>] sysfs_kf_write+0x3d/0x50
> [  556.671121]  [<ffffffff812430a4>] kernfs_fop_write+0xe4/0x160
> [  556.671126]  [<ffffffff811cc367>] vfs_write+0xb7/0x1f0
> [  556.671129]  [<ffffffff811ccf19>] SyS_write+0x49/0xb0
> [  556.671134]  [<ffffffff81106f36>] ? __audit_syscall_exit+0x1f6/0x2a0
> [  556.671139]  [<ffffffff816b69d2>] system_call_fastpath+0x16/0x1b
>
> Cheers,
>  Nik
--
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

Powered by blists - more mailing lists