| 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
| ||
|
Message-ID: <CAF2d9jjHbqpMCrS9Zm=rDO9w68TZddLztN_-QF8KBrLhZNMXWw@mail.gmail.com>
Date: Fri, 19 Sep 2014 17:09:21 -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: [PATCH net-next v4 2/2] bonding: Simplify the xmit function for
modes that use xmit_hash
On Fri, Sep 19, 2014 at 4:06 AM, Nikolay Aleksandrov <nikolay@...hat.com> wrote:
>
> On 09/19/2014 12:00 PM, Nikolay Aleksandrov wrote:
> > On 09/18/2014 11:53 PM, Mahesh Bandewar wrote:
> >> Earlier change to use usable slave array for TLB mode had an additional
> >> performance advantage. So extending the same logic to all other modes
> >> that use xmit-hash for slave selection (viz 802.3AD, and XOR modes).
> >> Also consolidating this with the earlier TLB change.
> >>
> >> The main idea is to build the usable slaves array in the control path
> >> and use that array for slave selection during xmit operation.
> >>
> >> Measured performance in a setup with a bond of 4x1G NICs with 200
> >> instances of netperf for the modes involved (3ad, xor, tlb)
> >> cmd: netperf -t TCP_RR -H <TargetHost> -l 60 -s 5
> >>
> >> Mode TPS-Before TPS-After
> >>
> >> 802.3ad : 468,694 493,101
> >> TLB (lb=0): 392,583 392,965
> >> XOR : 475,696 484,517
> >>
> >> Signed-off-by: Mahesh Bandewar <maheshb@...gle.com>
> >> ---
> >> v1:
> >> (a) If bond_update_slave_arr() fails to allocate memory, it will overwrite
> >> the slave that need to be removed.
> >> (b) Freeing of array will assign NULL (to handle bond->down to bond->up
> >> transition gracefully.
> >> (c) Change from pr_debug() to pr_err() if bond_update_slave_arr() returns
> >> failure.
> >> (d) XOR: bond_update_slave_arr() will consider mii-mon, arp-mon cases and
> >> will populate the array even if these parameters are not used.
> >> (e) 3AD: Should handle the ad_agg_selection_logic correctly.
> >> v2:
> >> (a) Removed rcu_read_{un}lock() calls from array manipulation code.
> >> (b) Slave link-events now refresh array for all these modes.
> >> (c) Moved free-array call from bond_close() to bond_uninit().
> >> v3:
> >> (a) Fixed null pointer dereference.
> >> (b) Removed bond->lock lockdep dependency.
> >> v4:
> >> (a) Made to changes to comply with Nikolay's locking changes
> >> (b) Added a work-queue to refresh slave-array when RTNL is not held
> >> (c) Array refresh happens ONLY with RTNL now.
> >> (d) alloc changed from GFP_ATOMIC to GFP_KERNEL
> >>
> <<<snip>>>
> >> @@ -3839,6 +4003,7 @@ static void bond_uninit(struct net_device *bond_dev)
> >> struct bonding *bond = netdev_priv(bond_dev);
> >> struct list_head *iter;
> >> struct slave *slave;
> >> + struct bond_up_slave *arr;
> >>
> >> bond_netpoll_cleanup(bond_dev);
> >>
> >> @@ -3847,6 +4012,12 @@ static void bond_uninit(struct net_device *bond_dev)
> >> __bond_release_one(bond_dev, slave->dev, true);
> >> netdev_info(bond_dev, "Released all slaves\n");
> >>
> Sorry but I just spotted a major problem, bond_3ad_unbind_slave() (called
> from __bond_release_one) calls ad_agg_selection_logic() which can re-arm
> the slave_arr work after it's supposed to be stopped here (i.e. the bond
> device has been closed so all works should've been stopped) so we might
> leak memory and access freed memory after all since it'll keep
> re-scheduling itself until it can acquire rtnl which is after the bond
> device has been destroyed.
>
This should not be a problem. ndo_close (bond_close()) is called
before ndo_uninit(bond_uninit()), so the work-queues get cancelled
there so if rearm tries to schedule some work after queue gets
cancelled, it can't do much and wont harm anything.
Hence there wont be any arrays built once it's free-ed completely and
therefore no memory leak. I addded some instrumentation and tried
following sequence -
# modprobe bonding mode=4
# ip link set bond0 up
# [Add ip]
# [Add default route]
# ifenslave bond0 eth0 eth1 eth2 eth3
....
[Run some backgound traffic. I used netperf.]
# ip link bond0 down
I did not see anything "bad" happening. Did your trial produced
something unpleasant?
> >> + arr = rtnl_dereference(bond->slave_arr);
> >> + if (arr) {
> >> + kfree_rcu(arr, rcu);
> >> + RCU_INIT_POINTER(bond->slave_arr, NULL);
> >> + }
> >> +
> >> list_del(&bond->bond_list);
> >>
> >> bond_debug_unregister(bond);
> >> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
> >> index 98dc0d7ad731..4635b175256a 100644
> >> --- a/drivers/net/bonding/bonding.h
> >> +++ b/drivers/net/bonding/bonding.h
> >> @@ -177,6 +177,12 @@ struct slave {
> >> struct kobject kobj;
> >> };
> >>
> >> +struct bond_up_slave {
> >> + unsigned int count;
> >> + struct rcu_head rcu;
> >> + struct slave *arr[0];
> >> +};
> >> +
> >> /*
> >> * Link pseudo-state only used internally by monitors
> >> */
> >> @@ -191,6 +197,7 @@ struct bonding {
> >> struct slave __rcu *curr_active_slave;
> >> struct slave __rcu *current_arp_slave;
> >> struct slave __rcu *primary_slave;
> >> + struct bond_up_slave __rcu *slave_arr; /* Array of usable slaves */
> >> bool force_primary;
> >> s32 slave_cnt; /* never change this value outside the attach/detach wrappers */
> >> int (*recv_probe)(const struct sk_buff *, struct bonding *,
> >> @@ -220,6 +227,7 @@ struct bonding {
> >> struct delayed_work alb_work;
> >> struct delayed_work ad_work;
> >> struct delayed_work mcast_work;
> >> + struct delayed_work slave_arr_work;
> >> #ifdef CONFIG_DEBUG_FS
> >> /* debugging support via debugfs */
> >> struct dentry *debug_dir;
> >> @@ -531,6 +539,8 @@ const char *bond_slave_link_status(s8 link);
> >> struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev,
> >> struct net_device *end_dev,
> >> int level);
> >> +int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave);
> >> +void bond_slave_arr_work_rearm(struct bonding *bond);
> >>
> >> #ifdef CONFIG_PROC_FS
> >> void bond_create_proc_entry(struct bonding *bond);
> >>
> >
> > --
> > 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
> >
>
--
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