| 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: <527D0AEF.1060802@redhat.com>
Date: Fri, 08 Nov 2013 17:01:51 +0100
From: Nikolay Aleksandrov <nikolay@...hat.com>
To: Ding Tianhong <dingtianhong@...wei.com>
CC: Jay Vosburgh <fubar@...ibm.com>,
Andy Gospodarek <andy@...yhouse.net>,
"David S. Miller" <davem@...emloft.net>,
Veaceslav Falico <vfalico@...hat.com>,
Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 6/10] bonding: rebuild the lock use for bond_activebackup_arp_mon()
On 11/08/2013 03:08 AM, Ding Tianhong wrote:
> The bond_activebackup_arp_mon() use the bond lock for read to
> protect the slave list, it is no effect, and the RTNL is only
> called for bond_ab_arp_commit() and peer notify, for the performance
> better, use RCU instead of the bond lock, because the bond slave
> list need to called in RCU, add a new bond_first_slave_rcu()
> to get the first slave in RCU protection.
>
> When bond_ab_arp_inspect() and should_notify_peers is true, the
> RTNL will called twice, it is a loss of performance, so make the
> two RTNL together to avoid performance loss.
>
> Suggested-by: Jay Vosburgh <fubar@...ibm.com>
> Suggested-by: Veaceslav Falico <vfalico@...hat.com>
> Signed-off-by: Ding Tianhong <dingtianhong@...wei.com>
> ---
> drivers/net/bonding/bond_main.c | 35 +++++++++++++++++++----------------
> drivers/net/bonding/bonding.h | 7 +++++++
> 2 files changed, 26 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 759dcd0..b48ca55 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2524,7 +2524,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
> struct slave *slave;
> int commit = 0;
>
> - bond_for_each_slave(bond, slave, iter) {
> + bond_for_each_slave_rcu(bond, slave, iter) {
> slave->new_link = BOND_LINK_NOCHANGE;
> last_rx = slave_last_rx(bond, slave);
>
> @@ -2586,7 +2586,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
> * Called to commit link state changes noted by inspection step of
> * active-backup mode ARP monitor.
> *
> - * Called with RTNL and bond->lock for read.
> + * Called with RTNL hold.
> */
> static void bond_ab_arp_commit(struct bonding *bond)
> {
> @@ -2661,7 +2661,7 @@ do_failover:
> /*
> * Send ARP probes for active-backup mode ARP monitor.
> *
> - * Called with bond->lock held for read.
> + * Called with rcu_read_lock hold.
> */
> static void bond_ab_arp_probe(struct bonding *bond)
> {
> @@ -2690,14 +2690,14 @@ static void bond_ab_arp_probe(struct bonding *bond)
> */
>
> if (!bond->current_arp_slave) {
> - bond->current_arp_slave = bond_first_slave(bond);
> + bond->current_arp_slave = bond_first_slave_rcu(bond);
> if (!bond->current_arp_slave)
> return;
> }
>
> bond_set_slave_inactive_flags(bond->current_arp_slave);
>
> - bond_for_each_slave(bond, slave, iter) {
> + bond_for_each_slave_rcu(bond, slave, iter) {
> if (!found && !before && IS_UP(slave->dev))
> before = slave;
>
> @@ -2745,43 +2745,46 @@ void bond_activebackup_arp_mon(struct work_struct *work)
> bool should_notify_peers = false;
> int delta_in_ticks;
>
> - read_lock(&bond->lock);
> -
> delta_in_ticks = msecs_to_jiffies(bond->params.arp_interval);
>
> - if (!bond_has_slaves(bond))
> + rcu_read_lock();
> +
> + if (!bond_has_slaves_rcu(bond)) {
> + rcu_read_unlock();
> goto re_arm;
> + }
>
> should_notify_peers = bond_should_notify_peers(bond);
Again, bond_should_notify_peers() is not RCU-safe.
>
> if (bond_ab_arp_inspect(bond)) {
> - read_unlock(&bond->lock);
> + rcu_read_unlock();
>
> /* Race avoidance with bond_close flush of workqueue */
> if (!rtnl_trylock()) {
> - read_lock(&bond->lock);
> delta_in_ticks = 1;
> should_notify_peers = false;
> goto re_arm;
> }
>
> - read_lock(&bond->lock);
> -
> bond_ab_arp_commit(bond);
>
> - read_unlock(&bond->lock);
> + if (should_notify_peers) {
> + call_netdevice_notifiers(NETDEV_NOTIFY_PEERS,
> + bond->dev);
> + should_notify_peers = false;
> + }
> +
> rtnl_unlock();
> - read_lock(&bond->lock);
> + rcu_read_lock();
> }
>
> bond_ab_arp_probe(bond);
Generally you might be safe in bond_ab_arp_probe() due to the synchronization
done by netdev_rx_handler_unregister(), but this code may run after that (and
after the unlinked slave) but before current_arp_slave is set to NULL and thus
use it. Now I don't see a direct problem with that, only a complication that can
bite us later. I vaguely remember that I re-worked the bond_ab_arp_probe() and
the way current_arp_slave works when doing this transition in my patches.
> + rcu_read_unlock();
>
> re_arm:
> if (bond->params.arp_interval)
> queue_delayed_work(bond->wq, &bond->arp_work, delta_in_ticks);
>
> - read_unlock(&bond->lock);
> -
> if (should_notify_peers) {
> if (!rtnl_trylock())
> return;
> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
> index deb9738..90b745c 100644
> --- a/drivers/net/bonding/bonding.h
> +++ b/drivers/net/bonding/bonding.h
> @@ -97,6 +97,13 @@
> netdev_adjacent_get_private(bond_slave_list(bond)->prev) : \
> NULL)
>
> +#define bond_first_slave_rcu(bond) \
> + ({struct list_head *__ptr = (bond_slave_list(bond)); \
> + struct list_head *__next = ACCESS_ONCE(__ptr->next); \
> + likely(__ptr != __next) ? \
> + netdev_adjacent_get_private_rcu(__next) : NULL; \
> + })
> +
Honestly, I don't like this, it sure can be re-written in a more
straight-forward manner.
> #define bond_is_first_slave(bond, pos) (pos == bond_first_slave(bond))
> #define bond_is_last_slave(bond, pos) (pos == bond_last_slave(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
Powered by blists - more mailing lists