From e965f8049460569bab12fe7bb5381bb2279712e2 Mon Sep 17 00:00:00 2001 From: Eric Dumazet Date: Tue, 15 Jul 2014 06:56:53 -0700 Subject: [PATCH 1/4] bonding: get rid of bond_option_active_slave_get() Only keep bond_option_active_slave_get_rcu() helper. bond_fill_info() uses a new bond_option_active_slave_get_ifindex() helper. Signed-off-by: Eric Dumazet Acked-by: Veaceslav Falico Reviewed-by: Nikolay Aleksandrov Signed-off-by: David S. Miller --- drivers/net/bonding/bond_netlink.c | 21 ++++++++++++++++----- drivers/net/bonding/bond_options.c | 5 ----- drivers/net/bonding/bonding.h | 1 - 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c index 5ab3c1847e67..4d97e23eb497 100644 --- a/drivers/net/bonding/bond_netlink.c +++ b/drivers/net/bonding/bond_netlink.c @@ -398,20 +398,31 @@ static size_t bond_get_size(const struct net_device *bond_dev) 0; } +static int bond_option_active_slave_get_ifindex(struct bonding *bond) +{ + const struct net_device *slave; + int ifindex; + + rcu_read_lock(); + slave = bond_option_active_slave_get_rcu(bond); + ifindex = slave ? slave->ifindex : 0; + rcu_read_unlock(); + return ifindex; +} + static int bond_fill_info(struct sk_buff *skb, const struct net_device *bond_dev) { struct bonding *bond = netdev_priv(bond_dev); - struct net_device *slave_dev = bond_option_active_slave_get(bond); - struct nlattr *targets; unsigned int packets_per_slave; - int i, targets_added; + int ifindex, i, targets_added; + struct nlattr *targets; if (nla_put_u8(skb, IFLA_BOND_MODE, BOND_MODE(bond))) goto nla_put_failure; - if (slave_dev && - nla_put_u32(skb, IFLA_BOND_ACTIVE_SLAVE, slave_dev->ifindex)) + ifindex = bond_option_active_slave_get_ifindex(bond); + if (ifindex && nla_put_u32(skb, IFLA_BOND_ACTIVE_SLAVE, ifindex)) goto nla_put_failure; if (nla_put_u32(skb, IFLA_BOND_MIIMON, bond->params.miimon)) diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c index 540e0167bf24..b26271fd7b09 100644 --- a/drivers/net/bonding/bond_options.c +++ b/drivers/net/bonding/bond_options.c @@ -704,11 +704,6 @@ struct net_device *bond_option_active_slave_get_rcu(struct bonding *bond) return __bond_option_active_slave_get(bond, slave); } -struct net_device *bond_option_active_slave_get(struct bonding *bond) -{ - return __bond_option_active_slave_get(bond, bond->curr_active_slave); -} - static int bond_option_active_slave_set(struct bonding *bond, const struct bond_opt_value *newval) { diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h index 0b4d9cde0b05..713e2a99c661 100644 --- a/drivers/net/bonding/bonding.h +++ b/drivers/net/bonding/bonding.h @@ -514,7 +514,6 @@ unsigned int bond_get_num_tx_queues(void); int bond_netlink_init(void); void bond_netlink_fini(void); struct net_device *bond_option_active_slave_get_rcu(struct bonding *bond); -struct net_device *bond_option_active_slave_get(struct bonding *bond); const char *bond_slave_link_status(s8 link); bool bond_verify_device_path(struct net_device *start_dev, struct net_device *end_dev, From c2646b593eb127adc50e108649e4d34144e14c6c Mon Sep 17 00:00:00 2001 From: Eric Dumazet Date: Tue, 15 Jul 2014 06:56:54 -0700 Subject: [PATCH 2/4] bonding: use rcu_access_pointer() in bonding_show_mii_status() curr_active_slave is rcu protected, and bonding_show_mii_status() only wants to check if pointer is NULL or not. Signed-off-by: Eric Dumazet Acked-by: Veaceslav Falico Reviewed-by: Nikolay Aleksandrov Signed-off-by: David S. Miller --- drivers/net/bonding/bond_sysfs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c index daed52f68ce1..98db8edd9c75 100644 --- a/drivers/net/bonding/bond_sysfs.c +++ b/drivers/net/bonding/bond_sysfs.c @@ -492,8 +492,9 @@ static ssize_t bonding_show_mii_status(struct device *d, char *buf) { struct bonding *bond = to_bond(d); + bool active = !!rcu_access_pointer(bond->curr_active_slave); - return sprintf(buf, "%s\n", bond->curr_active_slave ? "up" : "down"); + return sprintf(buf, "%s\n", active ? "up" : "down"); } static DEVICE_ATTR(mii_status, S_IRUGO, bonding_show_mii_status, NULL); From 4740d6382790077f22c606d03804f5d9f15b90d7 Mon Sep 17 00:00:00 2001 From: Eric Dumazet Date: Tue, 15 Jul 2014 06:56:55 -0700 Subject: [PATCH 3/4] bonding: add proper __rcu annotation for curr_active_slave RCU was added to bonding in linux-3.12 but lacked proper sparse annotations. Using __rcu annotation actually helps to spot all accesses to bond->curr_active_slave are correctly protected, with LOCKDEP support. Signed-off-by: Eric Dumazet Acked-by: Veaceslav Falico Reviewed-by: Nikolay Aleksandrov Signed-off-by: David S. Miller --- drivers/net/bonding/bond_alb.c | 47 +++++++++++++++----------- drivers/net/bonding/bond_main.c | 53 ++++++++++++++++-------------- drivers/net/bonding/bond_options.c | 2 +- drivers/net/bonding/bonding.h | 6 +++- 4 files changed, 63 insertions(+), 45 deletions(-) diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c index 76c0dade233f..de5bd03925b4 100644 --- a/drivers/net/bonding/bond_alb.c +++ b/drivers/net/bonding/bond_alb.c @@ -448,11 +448,13 @@ static struct slave *__rlb_next_rx_slave(struct bonding *bond) */ static void rlb_teach_disabled_mac_on_primary(struct bonding *bond, u8 addr[]) { - if (!bond->curr_active_slave) + struct slave *curr_active = bond_deref_active_protected(bond); + + if (!curr_active) return; if (!bond->alb_info.primary_is_promisc) { - if (!dev_set_promiscuity(bond->curr_active_slave->dev, 1)) + if (!dev_set_promiscuity(curr_active->dev, 1)) bond->alb_info.primary_is_promisc = 1; else bond->alb_info.primary_is_promisc = 0; @@ -460,7 +462,7 @@ static void rlb_teach_disabled_mac_on_primary(struct bonding *bond, u8 addr[]) bond->alb_info.rlb_promisc_timeout_counter = 0; - alb_send_learning_packets(bond->curr_active_slave, addr, true); + alb_send_learning_packets(curr_active, addr, true); } /* slave being removed should not be active at this point @@ -509,7 +511,7 @@ static void rlb_clear_slave(struct bonding *bond, struct slave *slave) write_lock_bh(&bond->curr_slave_lock); - if (slave != bond->curr_active_slave) + if (slave != bond_deref_active_protected(bond)) rlb_teach_disabled_mac_on_primary(bond, slave->dev->dev_addr); write_unlock_bh(&bond->curr_slave_lock); @@ -684,7 +686,7 @@ static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bon * move the old client to primary (curr_active_slave) so * that the new client can be assigned to this entry. */ - if (bond->curr_active_slave && + if (curr_active_slave && client_info->slave != curr_active_slave) { client_info->slave = curr_active_slave; rlb_update_client(client_info); @@ -1221,7 +1223,7 @@ static void alb_change_hw_addr_on_detach(struct bonding *bond, struct slave *sla */ static int alb_handle_addr_collision_on_attach(struct bonding *bond, struct slave *slave) { - struct slave *has_bond_addr = bond->curr_active_slave; + struct slave *has_bond_addr = rcu_access_pointer(bond->curr_active_slave); struct slave *tmp_slave1, *free_mac_slave = NULL; struct list_head *iter; @@ -1575,7 +1577,7 @@ void bond_alb_monitor(struct work_struct *work) * use mac of the slave device. * In RLB mode, we always use strict matches. */ - strict_match = (slave != bond->curr_active_slave || + strict_match = (slave != rcu_access_pointer(bond->curr_active_slave) || bond_info->rlb_enabled); alb_send_learning_packets(slave, slave->dev->dev_addr, strict_match); @@ -1593,7 +1595,7 @@ void bond_alb_monitor(struct work_struct *work) bond_for_each_slave_rcu(bond, slave, iter) { tlb_clear_slave(bond, slave, 1); - if (slave == bond->curr_active_slave) { + if (slave == rcu_access_pointer(bond->curr_active_slave)) { SLAVE_TLB_INFO(slave).load = bond_info->unbalanced_load / BOND_TLB_REBALANCE_INTERVAL; @@ -1625,7 +1627,8 @@ void bond_alb_monitor(struct work_struct *work) * because a slave was disabled then * it can now leave promiscuous mode. */ - dev_set_promiscuity(bond->curr_active_slave->dev, -1); + dev_set_promiscuity(rtnl_dereference(bond->curr_active_slave)->dev, + -1); bond_info->primary_is_promisc = 0; rtnl_unlock(); @@ -1742,17 +1745,21 @@ void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave __acquires(&bond->curr_slave_lock) { struct slave *swap_slave; + struct slave *curr_active; - if (bond->curr_active_slave == new_slave) + curr_active = rcu_dereference_protected(bond->curr_active_slave, + !new_slave || + lockdep_is_held(&bond->curr_slave_lock)); + if (curr_active == new_slave) return; - if (bond->curr_active_slave && bond->alb_info.primary_is_promisc) { - dev_set_promiscuity(bond->curr_active_slave->dev, -1); + if (curr_active && bond->alb_info.primary_is_promisc) { + dev_set_promiscuity(curr_active->dev, -1); bond->alb_info.primary_is_promisc = 0; bond->alb_info.rlb_promisc_timeout_counter = 0; } - swap_slave = bond->curr_active_slave; + swap_slave = curr_active; rcu_assign_pointer(bond->curr_active_slave, new_slave); if (!new_slave || !bond_has_slaves(bond)) @@ -1818,6 +1825,7 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr) { struct bonding *bond = netdev_priv(bond_dev); struct sockaddr *sa = addr; + struct slave *curr_active; struct slave *swap_slave; int res; @@ -1834,23 +1842,24 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr) * Otherwise we'll need to pass the new address to it and handle * duplications. */ - if (!bond->curr_active_slave) + curr_active = rtnl_dereference(bond->curr_active_slave); + if (!curr_active) return 0; swap_slave = bond_slave_has_mac(bond, bond_dev->dev_addr); if (swap_slave) { - alb_swap_mac_addr(swap_slave, bond->curr_active_slave); - alb_fasten_mac_swap(bond, swap_slave, bond->curr_active_slave); + alb_swap_mac_addr(swap_slave, curr_active); + alb_fasten_mac_swap(bond, swap_slave, curr_active); } else { - alb_set_slave_mac_addr(bond->curr_active_slave, bond_dev->dev_addr); + alb_set_slave_mac_addr(curr_active, bond_dev->dev_addr); read_lock(&bond->lock); - alb_send_learning_packets(bond->curr_active_slave, + alb_send_learning_packets(curr_active, bond_dev->dev_addr, false); if (bond->alb_info.rlb_enabled) { /* inform clients mac address has changed */ - rlb_req_update_slave_clients(bond, bond->curr_active_slave); + rlb_req_update_slave_clients(bond, curr_active); } read_unlock(&bond->lock); } diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 46dcb7b6216f..27ce838d45d6 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -498,11 +498,11 @@ static int bond_set_promiscuity(struct bonding *bond, int inc) int err = 0; if (bond_uses_primary(bond)) { + struct slave *curr_active = bond_deref_active_protected(bond); + /* write lock already acquired */ - if (bond->curr_active_slave) { - err = dev_set_promiscuity(bond->curr_active_slave->dev, - inc); - } + if (curr_active) + err = dev_set_promiscuity(curr_active->dev, inc); } else { struct slave *slave; @@ -524,11 +524,11 @@ static int bond_set_allmulti(struct bonding *bond, int inc) int err = 0; if (bond_uses_primary(bond)) { + struct slave *curr_active = bond_deref_active_protected(bond); + /* write lock already acquired */ - if (bond->curr_active_slave) { - err = dev_set_allmulti(bond->curr_active_slave->dev, - inc); - } + if (curr_active) + err = dev_set_allmulti(curr_active->dev, inc); } else { struct slave *slave; @@ -713,7 +713,7 @@ out: static bool bond_should_change_active(struct bonding *bond) { struct slave *prim = bond->primary_slave; - struct slave *curr = bond->curr_active_slave; + struct slave *curr = bond_deref_active_protected(bond); if (!prim || !curr || curr->link != BOND_LINK_UP) return true; @@ -792,7 +792,11 @@ static bool bond_should_notify_peers(struct bonding *bond) */ void bond_change_active_slave(struct bonding *bond, struct slave *new_active) { - struct slave *old_active = bond->curr_active_slave; + struct slave *old_active; + + old_active = rcu_dereference_protected(bond->curr_active_slave, + !new_active || + lockdep_is_held(&bond->curr_slave_lock)); if (old_active == new_active) return; @@ -900,7 +904,7 @@ void bond_select_active_slave(struct bonding *bond) int rv; best_slave = bond_find_best_slave(bond); - if (best_slave != bond->curr_active_slave) { + if (best_slave != bond_deref_active_protected(bond)) { bond_change_active_slave(bond, best_slave); rv = bond_set_carrier(bond); if (!rv) @@ -1531,7 +1535,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) * anyway (it holds no special properties of the bond device), * so we can change it without calling change_active_interface() */ - if (!bond->curr_active_slave && new_slave->link == BOND_LINK_UP) + if (!rcu_access_pointer(bond->curr_active_slave) && + new_slave->link == BOND_LINK_UP) rcu_assign_pointer(bond->curr_active_slave, new_slave); break; @@ -1602,7 +1607,7 @@ err_detach: vlan_vids_del_by_dev(slave_dev, bond_dev); if (bond->primary_slave == new_slave) bond->primary_slave = NULL; - if (bond->curr_active_slave == new_slave) { + if (rcu_access_pointer(bond->curr_active_slave) == new_slave) { block_netpoll_tx(); write_lock_bh(&bond->curr_slave_lock); bond_change_active_slave(bond, NULL); @@ -1704,7 +1709,7 @@ static int __bond_release_one(struct net_device *bond_dev, bond_is_active_slave(slave) ? "active" : "backup", slave_dev->name); - oldcurrent = bond->curr_active_slave; + oldcurrent = rcu_access_pointer(bond->curr_active_slave); bond->current_arp_slave = NULL; @@ -1878,7 +1883,7 @@ static int bond_slave_info_query(struct net_device *bond_dev, struct ifslave *in /*-------------------------------- Monitoring -------------------------------*/ - +/* called with rcu_read_lock() */ static int bond_miimon_inspect(struct bonding *bond) { int link_state, commit = 0; @@ -1886,7 +1891,7 @@ static int bond_miimon_inspect(struct bonding *bond) struct slave *slave; bool ignore_updelay; - ignore_updelay = !bond->curr_active_slave ? true : false; + ignore_updelay = !rcu_dereference(bond->curr_active_slave); bond_for_each_slave_rcu(bond, slave, iter) { slave->new_link = BOND_LINK_NOCHANGE; @@ -2046,7 +2051,7 @@ static void bond_miimon_commit(struct bonding *bond) bond_alb_handle_link_change(bond, slave, BOND_LINK_DOWN); - if (slave == bond->curr_active_slave) + if (slave == rcu_access_pointer(bond->curr_active_slave)) goto do_failover; continue; @@ -2416,7 +2421,7 @@ static void bond_loadbalance_arp_mon(struct work_struct *work) rcu_read_lock(); - oldcurrent = ACCESS_ONCE(bond->curr_active_slave); + oldcurrent = rcu_dereference(bond->curr_active_slave); /* see if any of the previous devices are up now (i.e. they have * xmt and rcv traffic). the curr_active_slave does not come into * the picture unless it is null. also, slave->last_link_up is not @@ -2607,8 +2612,8 @@ static void bond_ab_arp_commit(struct bonding *bond) case BOND_LINK_UP: trans_start = dev_trans_start(slave->dev); - if (bond->curr_active_slave != slave || - (!bond->curr_active_slave && + if (rtnl_dereference(bond->curr_active_slave) != slave || + (!rtnl_dereference(bond->curr_active_slave) && bond_time_in_interval(bond, trans_start, 1))) { slave->link = BOND_LINK_UP; if (bond->current_arp_slave) { @@ -2621,7 +2626,7 @@ static void bond_ab_arp_commit(struct bonding *bond) pr_info("%s: link status definitely up for interface %s\n", bond->dev->name, slave->dev->name); - if (!bond->curr_active_slave || + if (!rtnl_dereference(bond->curr_active_slave) || (slave == bond->primary_slave)) goto do_failover; @@ -2640,7 +2645,7 @@ static void bond_ab_arp_commit(struct bonding *bond) pr_info("%s: link status definitely down for interface %s, disabling it\n", bond->dev->name, slave->dev->name); - if (slave == bond->curr_active_slave) { + if (slave == rtnl_dereference(bond->curr_active_slave)) { bond->current_arp_slave = NULL; goto do_failover; } @@ -3097,8 +3102,8 @@ static int bond_open(struct net_device *bond_dev) if (bond_has_slaves(bond)) { read_lock(&bond->curr_slave_lock); bond_for_each_slave(bond, slave, iter) { - if (bond_uses_primary(bond) - && (slave != bond->curr_active_slave)) { + if (bond_uses_primary(bond) && + slave != rcu_access_pointer(bond->curr_active_slave)) { bond_set_slave_inactive_flags(slave, BOND_SLAVE_NOTIFY_NOW); } else { diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c index b26271fd7b09..f908e65e86c1 100644 --- a/drivers/net/bonding/bond_options.c +++ b/drivers/net/bonding/bond_options.c @@ -743,7 +743,7 @@ static int bond_option_active_slave_set(struct bonding *bond, RCU_INIT_POINTER(bond->curr_active_slave, NULL); bond_select_active_slave(bond); } else { - struct slave *old_active = bond->curr_active_slave; + struct slave *old_active = bond_deref_active_protected(bond); struct slave *new_active = bond_slave_get_rtnl(slave_dev); BUG_ON(!new_active); diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h index 713e2a99c661..d03d2ae4d3af 100644 --- a/drivers/net/bonding/bonding.h +++ b/drivers/net/bonding/bonding.h @@ -194,7 +194,7 @@ struct slave { */ struct bonding { struct net_device *dev; /* first - useful for panic debug */ - struct slave *curr_active_slave; + struct slave __rcu *curr_active_slave; struct slave *current_arp_slave; struct slave *primary_slave; bool force_primary; @@ -232,6 +232,10 @@ struct bonding { #define bond_slave_get_rtnl(dev) \ ((struct slave *) rtnl_dereference(dev->rx_handler_data)) +#define bond_deref_active_protected(bond) \ + rcu_dereference_protected(bond->curr_active_slave, \ + lockdep_is_held(&bond->curr_slave_lock)) + struct bond_vlan_tag { __be16 vlan_proto; unsigned short vlan_id; From 8574171833b24fda5101e1aa892a38c0d91d083e Mon Sep 17 00:00:00 2001 From: Eric Dumazet Date: Tue, 15 Jul 2014 06:56:56 -0700 Subject: [PATCH 4/4] bonding: add proper __rcu annotation for current_arp_slave Using __rcu annotation actually helps to spot all accesses to bond->current_arp_slave are correctly protected, with LOCKDEP support. Signed-off-by: Eric Dumazet Reviewed-by: Nikolay Aleksandrov Signed-off-by: David S. Miller --- drivers/net/bonding/bond_main.c | 15 +++++++++------ drivers/net/bonding/bonding.h | 2 +- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 27ce838d45d6..6d3b8db882a1 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1711,7 +1711,7 @@ static int __bond_release_one(struct net_device *bond_dev, oldcurrent = rcu_access_pointer(bond->curr_active_slave); - bond->current_arp_slave = NULL; + RCU_INIT_POINTER(bond->current_arp_slave, NULL); if (!all && (!bond->params.fail_over_mac || BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP)) { @@ -2569,7 +2569,7 @@ static int bond_ab_arp_inspect(struct bonding *bond) * before being taken out */ if (!bond_is_active_slave(slave) && - !bond->current_arp_slave && + !rcu_access_pointer(bond->current_arp_slave) && !bond_time_in_interval(bond, last_rx, 3)) { slave->new_link = BOND_LINK_DOWN; commit++; @@ -2615,12 +2615,15 @@ static void bond_ab_arp_commit(struct bonding *bond) if (rtnl_dereference(bond->curr_active_slave) != slave || (!rtnl_dereference(bond->curr_active_slave) && bond_time_in_interval(bond, trans_start, 1))) { + struct slave *current_arp_slave; + + current_arp_slave = rtnl_dereference(bond->current_arp_slave); slave->link = BOND_LINK_UP; - if (bond->current_arp_slave) { + if (current_arp_slave) { bond_set_slave_inactive_flags( - bond->current_arp_slave, + current_arp_slave, BOND_SLAVE_NOTIFY_NOW); - bond->current_arp_slave = NULL; + RCU_INIT_POINTER(bond->current_arp_slave, NULL); } pr_info("%s: link status definitely up for interface %s\n", @@ -2646,7 +2649,7 @@ static void bond_ab_arp_commit(struct bonding *bond) bond->dev->name, slave->dev->name); if (slave == rtnl_dereference(bond->curr_active_slave)) { - bond->current_arp_slave = NULL; + RCU_INIT_POINTER(bond->current_arp_slave, NULL); goto do_failover; } diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h index d03d2ae4d3af..b2e548e9d738 100644 --- a/drivers/net/bonding/bonding.h +++ b/drivers/net/bonding/bonding.h @@ -195,7 +195,7 @@ struct slave { struct bonding { struct net_device *dev; /* first - useful for panic debug */ struct slave __rcu *curr_active_slave; - struct slave *current_arp_slave; + struct slave __rcu *current_arp_slave; struct slave *primary_slave; bool force_primary; s32 slave_cnt; /* never change this value outside the attach/detach wrappers */