net/hsr: Move slave init to hsr_slave.c.

Also try to prevent some possible slave dereference race conditions. This is
finalized in the next patch, which abandons the slave array in favour of
a list_head list and list RCU.

Signed-off-by: Arvid Brodin <arvid.brodin@alten.se>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index 8936b3c2bb843eb86b488c1e7f4a4fc1e8c63f68..1f8869cb70ae12c46f091bd9b24a233ca8cde78c 100644 (file)
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -15,7 +15,6 @@
 #include <linux/netdevice.h>
 #include <linux/skbuff.h>
 #include <linux/etherdevice.h>
-#include <linux/if_arp.h>
 #include <linux/rtnetlink.h>
 #include <linux/pkt_sched.h>
 #include "hsr_device.h"
@@ -108,23 +107,27 @@ void hsr_check_carrier_and_operstate(struct hsr_priv *hsr)
        hsr_check_announce(hsr->dev, old_operstate);
 }
 
-
 int hsr_get_max_mtu(struct hsr_priv *hsr)
 {
-       int mtu_max;
-
-       if (hsr->slave[0] && hsr->slave[1])
-               mtu_max = min(hsr->slave[0]->mtu, hsr->slave[1]->mtu);
-       else if (hsr->slave[0])
-               mtu_max = hsr->slave[0]->mtu;
-       else if (hsr->slave[1])
-               mtu_max = hsr->slave[1]->mtu;
-       else
-               mtu_max = HSR_HLEN;
-
+       unsigned int mtu_max;
+       struct net_device *slave;
+
+       mtu_max = ETH_DATA_LEN;
+       rcu_read_lock();
+       slave = hsr->slave[0];
+       if (slave)
+               mtu_max = min(slave->mtu, mtu_max);
+       slave = hsr->slave[1];
+       if (slave)
+               mtu_max = min(slave->mtu, mtu_max);
+       rcu_read_unlock();
+
+       if (mtu_max < HSR_HLEN)
+               return 0;
        return mtu_max - HSR_HLEN;
 }
 
+
 static int hsr_dev_change_mtu(struct net_device *dev, int new_mtu)
 {
        struct hsr_priv *hsr;
@@ -145,18 +148,20 @@ static int hsr_dev_change_mtu(struct net_device *dev, int new_mtu)
 static int hsr_dev_open(struct net_device *dev)
 {
        struct hsr_priv *hsr;
+       struct net_device *slave;
        int i;
        char *slave_name;
 
        hsr = netdev_priv(dev);
 
        for (i = 0; i < HSR_MAX_SLAVE; i++) {
-               if (hsr->slave[i])
-                       slave_name = hsr->slave[i]->name;
+               slave = hsr->slave[i];
+               if (slave)
+                       slave_name = slave->name;
                else
                        slave_name = "null";
 
-               if (!is_slave_up(hsr->slave[i]))
+               if (!is_slave_up(slave))
                        netdev_warn(dev, "Slave %c (%s) is not up; please bring it up to get a working HSR network\n",
                                    'A' + i, slave_name);
        }
@@ -223,6 +228,8 @@ static int slave_xmit(struct sk_buff *skb, struct hsr_priv *hsr,
        hsr_ethhdr = (struct hsr_ethhdr *) skb->data;
 
        skb->dev = hsr->slave[dev_idx];
+       if (unlikely(!skb->dev))
+               return NET_XMIT_DROP;
 
        hsr_addr_subst_dest(hsr, &hsr_ethhdr->ethhdr, dev_idx);
 
@@ -252,14 +259,8 @@ static int hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
        }
 
        skb2 = pskb_copy(skb, GFP_ATOMIC);
-
-       res1 = NET_XMIT_DROP;
-       if (likely(hsr->slave[HSR_DEV_SLAVE_A]))
-               res1 = slave_xmit(skb, hsr, HSR_DEV_SLAVE_A);
-
-       res2 = NET_XMIT_DROP;
-       if (likely(skb2 && hsr->slave[HSR_DEV_SLAVE_B]))
-               res2 = slave_xmit(skb2, hsr, HSR_DEV_SLAVE_B);
+       res1 = slave_xmit(skb, hsr, HSR_DEV_SLAVE_A);
+       res2 = slave_xmit(skb2, hsr, HSR_DEV_SLAVE_B);
 
        if (likely(res1 == NET_XMIT_SUCCESS || res1 == NET_XMIT_CN ||
                   res2 == NET_XMIT_SUCCESS || res2 == NET_XMIT_CN)) {
@@ -406,28 +407,10 @@ static void hsr_announce(unsigned long data)
 static void restore_slaves(struct net_device *hsr_dev)
 {
        struct hsr_priv *hsr;
-       int i;
-       int res;
 
        hsr = netdev_priv(hsr_dev);
-
-       rtnl_lock();
-
-       for (i = 0; i < HSR_MAX_SLAVE; i++) {
-               if (!hsr->slave[i])
-                       continue;
-               res = dev_set_promiscuity(hsr->slave[i], -1);
-               if (res)
-                       netdev_info(hsr_dev,
-                                   "Cannot restore slave promiscuity (%s, %d)\n",
-                                   hsr->slave[i]->name, res);
-
-               if (hsr->slave[i]->rx_handler == hsr_handle_frame)
-                       netdev_rx_handler_unregister(hsr->slave[i]);
-       }
-
-
-       rtnl_unlock();
+       hsr_del_slave(hsr, 1);
+       hsr_del_slave(hsr, 0);
 }
 
 static void reclaim_hsr_dev(struct rcu_head *rh)
@@ -483,38 +466,6 @@ bool is_hsr_master(struct net_device *dev)
        return (dev->netdev_ops->ndo_start_xmit == hsr_dev_xmit);
 }
 
-static int check_slave_ok(struct net_device *dev)
-{
-       /* Don't allow HSR on non-ethernet like devices */
-       if ((dev->flags & IFF_LOOPBACK) || (dev->type != ARPHRD_ETHER) ||
-           (dev->addr_len != ETH_ALEN)) {
-               netdev_info(dev, "Cannot use loopback or non-ethernet device as HSR slave.\n");
-               return -EINVAL;
-       }
-
-       /* Don't allow enslaving hsr devices */
-       if (is_hsr_master(dev)) {
-               netdev_info(dev, "Cannot create trees of HSR devices.\n");
-               return -EINVAL;
-       }
-
-       if (is_hsr_slave(dev)) {
-               netdev_info(dev, "This device is already a HSR slave.\n");
-               return -EINVAL;
-       }
-
-       if (dev->priv_flags & IFF_802_1Q_VLAN) {
-               netdev_info(dev, "HSR on top of VLAN is not yet supported in this driver.\n");
-               return -EINVAL;
-       }
-
-       /* HSR over bonded devices has not been tested, but I'm not sure it
-        * won't work...
-        */
-
-       return 0;
-}
-
 
 /* Default multicast address for HSR Supervision frames */
 static const unsigned char def_multicast_addr[ETH_ALEN] __aligned(2) = {
@@ -525,15 +476,30 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
                     unsigned char multicast_spec)
 {
        struct hsr_priv *hsr;
-       int i;
        int res;
 
        hsr = netdev_priv(hsr_dev);
        hsr->dev = hsr_dev;
+       hsr->slave[0] = NULL;
+       hsr->slave[1] = NULL;
        INIT_LIST_HEAD(&hsr->node_db);
        INIT_LIST_HEAD(&hsr->self_node_db);
-       for (i = 0; i < HSR_MAX_SLAVE; i++)
-               hsr->slave[i] = slave[i];
+
+       ether_addr_copy(hsr_dev->dev_addr, slave[0]->dev_addr);
+
+       /* Make sure we recognize frames from ourselves in hsr_rcv() */
+       res = hsr_create_self_node(&hsr->self_node_db, hsr_dev->dev_addr,
+                                  slave[1]->dev_addr);
+       if (res < 0)
+               return res;
+
+       hsr_dev->features = slave[0]->features & slave[1]->features;
+       /* Prevent recursive tx locking */
+       hsr_dev->features |= NETIF_F_LLTX;
+       /* VLAN on top of HSR needs testing and probably some work on
+        * hsr_header_create() etc.
+        */
+       hsr_dev->features |= NETIF_F_VLAN_CHALLENGED;
 
        spin_lock_init(&hsr->seqnr_lock);
        /* Overflow soon to find bugs easier: */
@@ -560,65 +526,21 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
  *                     IFF_HSR_MASTER/SLAVE?
  */
 
-       for (i = 0; i < HSR_MAX_SLAVE; i++) {
-               res = check_slave_ok(slave[i]);
-               if (res)
-                       return res;
-       }
-
-       hsr_dev->features = slave[0]->features & slave[1]->features;
-       /* Prevent recursive tx locking */
-       hsr_dev->features |= NETIF_F_LLTX;
-       /* VLAN on top of HSR needs testing and probably some work on
-        * hsr_header_create() etc.
-        */
-       hsr_dev->features |= NETIF_F_VLAN_CHALLENGED;
-
-       /* Set hsr_dev's MAC address to that of mac_slave1 */
-       ether_addr_copy(hsr_dev->dev_addr, hsr->slave[0]->dev_addr);
-
-       /* Set required header length */
-       for (i = 0; i < HSR_MAX_SLAVE; i++) {
-               if (slave[i]->hard_header_len + HSR_HLEN >
-                                               hsr_dev->hard_header_len)
-                       hsr_dev->hard_header_len =
-                                       slave[i]->hard_header_len + HSR_HLEN;
-       }
-
-       /* MTU */
-       for (i = 0; i < HSR_MAX_SLAVE; i++)
-               if (slave[i]->mtu - HSR_HLEN < hsr_dev->mtu)
-                       hsr_dev->mtu = slave[i]->mtu - HSR_HLEN;
-
        /* Make sure the 1st call to netif_carrier_on() gets through */
        netif_carrier_off(hsr_dev);
 
-       /* Promiscuity */
-       for (i = 0; i < HSR_MAX_SLAVE; i++) {
-               res = dev_set_promiscuity(slave[i], 1);
-               if (res) {
-                       netdev_info(hsr_dev, "Cannot set slave promiscuity (%s, %d)\n",
-                                   slave[i]->name, res);
-                       goto fail;
-               }
-       }
-
-       for (i = 0; i < HSR_MAX_SLAVE; i++) {
-               res = netdev_rx_handler_register(slave[i], hsr_handle_frame,
-                                                hsr);
-               if (res)
-                       goto fail;
-       }
-
-       /* Make sure we recognize frames from ourselves in hsr_rcv() */
-       res = hsr_create_self_node(&hsr->self_node_db, hsr_dev->dev_addr,
-                                  hsr->slave[1]->dev_addr);
-       if (res < 0)
-               goto fail;
-
        res = register_netdevice(hsr_dev);
        if (res)
-               goto fail;
+               return res;
+
+       res = hsr_add_slave(hsr, slave[0], 0);
+       if (res)
+               return res;
+       res = hsr_add_slave(hsr, slave[1], 1);
+       if (res) {
+               hsr_del_slave(hsr, 0);
+               return res;
+       }
 
        hsr->prune_timer.expires = jiffies + msecs_to_jiffies(PRUNE_PERIOD);
        add_timer(&hsr->prune_timer);
@@ -626,8 +548,4 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
        register_hsr_master(hsr);
 
        return 0;
-
-fail:
-       restore_slaves(hsr_dev);
-       return res;
 }

diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
index 79e3f7ff66540d35f4b812721ebccb8ceac46f06..3666f94c526fc7955bf8efa994ccd4c795a430ab 100644 (file)
--- a/net/hsr/hsr_framereg.c
+++ b/net/hsr/hsr_framereg.c
@@ -455,6 +455,7 @@ int hsr_get_node_data(struct hsr_priv *hsr,
                      u16 *if2_seq)
 {
        struct hsr_node *node;
+       struct net_device *slave;
        unsigned long tdiff;
 
 
@@ -491,8 +492,9 @@ int hsr_get_node_data(struct hsr_priv *hsr,
        *if1_seq = node->seq_out[HSR_DEV_SLAVE_B];
        *if2_seq = node->seq_out[HSR_DEV_SLAVE_A];
 
-       if ((node->AddrB_if != HSR_DEV_NONE) && hsr->slave[node->AddrB_if])
-               *addr_b_ifindex = hsr->slave[node->AddrB_if]->ifindex;
+       slave = hsr->slave[node->AddrB_if];
+       if ((node->AddrB_if != HSR_DEV_NONE) && slave)
+               *addr_b_ifindex = slave->ifindex;
        else
                *addr_b_ifindex = -1;
 

diff --git a/net/hsr/hsr_main.c b/net/hsr/hsr_main.c
index 431b528c2447247123d2821e9695fc6629b432d9..b5abe26f7b4c45751309a3874d3fd9f8a0f2f324 100644 (file)
--- a/net/hsr/hsr_main.c
+++ b/net/hsr/hsr_main.c
@@ -17,6 +17,7 @@
 #include "hsr_device.h"
 #include "hsr_netlink.h"
 #include "hsr_framereg.h"
+#include "hsr_slave.h"
 
 
 /* List of all registered virtual HSR devices */
@@ -124,22 +125,21 @@ static int hsr_netdev_notify(struct notifier_block *nb, unsigned long event,
                if (dev == hsr->dev)
                        break;
 
-               if (dev == hsr->slave[0])
-                       ether_addr_copy(hsr->dev->dev_addr,
-                                       hsr->slave[0]->dev_addr);
+               if (dev == hsr->slave[0]) {
+                       ether_addr_copy(hsr->dev->dev_addr, dev->dev_addr);
+                       call_netdevice_notifiers(NETDEV_CHANGEADDR, hsr->dev);
+               }
 
                /* Make sure we recognize frames from ourselves in hsr_rcv() */
+               other_slave = hsr->slave[1];
                res = hsr_create_self_node(&hsr->self_node_db,
                                           hsr->dev->dev_addr,
-                                          hsr->slave[1] ?
-                                               hsr->slave[1]->dev_addr :
+                                          other_slave ?
+                                               other_slave->dev_addr :
                                                hsr->dev->dev_addr);
                if (res)
                        netdev_warn(hsr->dev,
                                    "Could not update HSR node address.\n");
-
-               if (dev == hsr->slave[0])
-                       call_netdevice_notifiers(NETDEV_CHANGEADDR, hsr->dev);
                break;
        case NETDEV_CHANGEMTU:
                if (dev == hsr->dev)
@@ -149,10 +149,14 @@ static int hsr_netdev_notify(struct notifier_block *nb, unsigned long event,
                        dev_set_mtu(hsr->dev, mtu_max);
                break;
        case NETDEV_UNREGISTER:
-               if (dev == hsr->slave[0])
+               if (dev == hsr->slave[0]) {
                        hsr->slave[0] = NULL;
-               if (dev == hsr->slave[1])
+                       hsr_del_slave(hsr, 0);
+               }
+               if (dev == hsr->slave[1]) {
                        hsr->slave[1] = NULL;
+                       hsr_del_slave(hsr, 1);
+               }
 
                /* There should really be a way to set a new slave device... */
 

diff --git a/net/hsr/hsr_netlink.c b/net/hsr/hsr_netlink.c
index bea250ec3586f1590b8a71f2614d9ff8cee374bf..a2ce359774f38cc142234dcf62a8ee06b9bcf494 100644 (file)
--- a/net/hsr/hsr_netlink.c
+++ b/net/hsr/hsr_netlink.c
@@ -64,16 +64,28 @@ static int hsr_newlink(struct net *src_net, struct net_device *dev,
 static int hsr_fill_info(struct sk_buff *skb, const struct net_device *dev)
 {
        struct hsr_priv *hsr;
+       struct net_device *slave;
+       int res;
 
        hsr = netdev_priv(dev);
 
-       if (hsr->slave[0])
-               if (nla_put_u32(skb, IFLA_HSR_SLAVE1, hsr->slave[0]->ifindex))
-                       goto nla_put_failure;
+       res = 0;
 
-       if (hsr->slave[1])
-               if (nla_put_u32(skb, IFLA_HSR_SLAVE2, hsr->slave[1]->ifindex))
-                       goto nla_put_failure;
+       rcu_read_lock();
+       slave = hsr->slave[0];
+       if (slave)
+               res = nla_put_u32(skb, IFLA_HSR_SLAVE1, slave->ifindex);
+       rcu_read_unlock();
+       if (res)
+               goto nla_put_failure;
+
+       rcu_read_lock();
+       slave = hsr->slave[1];
+       if (slave)
+               res = nla_put_u32(skb, IFLA_HSR_SLAVE2, slave->ifindex);
+       rcu_read_unlock();
+       if (res)
+               goto nla_put_failure;
 
        if (nla_put(skb, IFLA_HSR_SUPERVISION_ADDR, ETH_ALEN,
                    hsr->sup_multicast_addr) ||
@@ -132,6 +144,7 @@ void hsr_nl_ringerror(struct hsr_priv *hsr, unsigned char addr[ETH_ALEN],
                      enum hsr_dev_idx dev_idx)
 {
        struct sk_buff *skb;
+       struct net_device *slave;
        void *msg_head;
        int res;
        int ifindex;
@@ -148,10 +161,14 @@ void hsr_nl_ringerror(struct hsr_priv *hsr, unsigned char addr[ETH_ALEN],
        if (res < 0)
                goto nla_put_failure;
 
-       if (hsr->slave[dev_idx])
-               ifindex = hsr->slave[dev_idx]->ifindex;
+       rcu_read_lock();
+       slave = hsr->slave[dev_idx];
+       if (slave)
+               ifindex = slave->ifindex;
        else
                ifindex = -1;
+       rcu_read_unlock();
+
        res = nla_put_u32(skb, HSR_A_IFINDEX, ifindex);
        if (res < 0)
                goto nla_put_failure;
@@ -215,7 +232,7 @@ static int hsr_get_node_status(struct sk_buff *skb_in, struct genl_info *info)
 {
        /* For receiving */
        struct nlattr *na;
-       struct net_device *hsr_dev;
+       struct net_device *hsr_dev, *slave;
 
        /* For sending */
        struct sk_buff *skb_out;
@@ -301,9 +318,11 @@ static int hsr_get_node_status(struct sk_buff *skb_in, struct genl_info *info)
        res = nla_put_u16(skb_out, HSR_A_IF1_SEQ, hsr_node_if1_seq);
        if (res < 0)
                goto nla_put_failure;
-       if (hsr->slave[0])
-               res = nla_put_u32(skb_out, HSR_A_IF1_IFINDEX,
-                                               hsr->slave[0]->ifindex);
+       rcu_read_lock();
+       slave = hsr->slave[0];
+       if (slave)
+               res = nla_put_u32(skb_out, HSR_A_IF1_IFINDEX, slave->ifindex);
+       rcu_read_unlock();
        if (res < 0)
                goto nla_put_failure;
 
@@ -313,9 +332,13 @@ static int hsr_get_node_status(struct sk_buff *skb_in, struct genl_info *info)
        res = nla_put_u16(skb_out, HSR_A_IF2_SEQ, hsr_node_if2_seq);
        if (res < 0)
                goto nla_put_failure;
-       if (hsr->slave[1])
-               res = nla_put_u32(skb_out, HSR_A_IF2_IFINDEX,
-                                               hsr->slave[1]->ifindex);
+       rcu_read_lock();
+       slave = hsr->slave[1];
+       if (slave)
+               res = nla_put_u32(skb_out, HSR_A_IF2_IFINDEX, slave->ifindex);
+       rcu_read_unlock();
+       if (res < 0)
+               goto nla_put_failure;
 
        genlmsg_end(skb_out, msg_head);
        genlmsg_unicast(genl_info_net(info), skb_out, info->snd_portid);

diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
index 702814631ee11396b053227196786cf4e427766d..d676090f7900711d93b1784e4fc5b5b5bf2ee7ee 100644 (file)
--- a/net/hsr/hsr_slave.c
+++ b/net/hsr/hsr_slave.c
@@ -11,10 +11,45 @@
 
 #include "hsr_slave.h"
 #include <linux/etherdevice.h>
+#include <linux/if_arp.h>
 #include "hsr_main.h"
+#include "hsr_device.h"
 #include "hsr_framereg.h"
 
 
+static int check_slave_ok(struct net_device *dev)
+{
+       /* Don't allow HSR on non-ethernet like devices */
+       if ((dev->flags & IFF_LOOPBACK) || (dev->type != ARPHRD_ETHER) ||
+           (dev->addr_len != ETH_ALEN)) {
+               netdev_info(dev, "Cannot use loopback or non-ethernet device as HSR slave.\n");
+               return -EINVAL;
+       }
+
+       /* Don't allow enslaving hsr devices */
+       if (is_hsr_master(dev)) {
+               netdev_info(dev, "Cannot create trees of HSR devices.\n");
+               return -EINVAL;
+       }
+
+       if (is_hsr_slave(dev)) {
+               netdev_info(dev, "This device is already a HSR slave.\n");
+               return -EINVAL;
+       }
+
+       if (dev->priv_flags & IFF_802_1Q_VLAN) {
+               netdev_info(dev, "HSR on top of VLAN is not yet supported in this driver.\n");
+               return -EINVAL;
+       }
+
+       /* HSR over bonded devices has not been tested, but I'm not sure it
+        * won't work...
+        */
+
+       return 0;
+}
+
+
 static struct sk_buff *hsr_pull_tag(struct sk_buff *skb)
 {
        struct hsr_tag *hsr_tag;
@@ -241,3 +276,59 @@ forward:
 
        return RX_HANDLER_CONSUMED;
 }
+
+int hsr_add_slave(struct hsr_priv *hsr, struct net_device *dev, int idx)
+{
+       int res;
+
+       dev_hold(dev);
+
+       res = check_slave_ok(dev);
+       if (res)
+               goto fail;
+
+       res = dev_set_promiscuity(dev, 1);
+       if (res)
+               goto fail;
+
+       res = netdev_rx_handler_register(dev, hsr_handle_frame, hsr);
+       if (res)
+               goto fail_rx_handler;
+
+
+       hsr->slave[idx] = dev;
+
+       /* Set required header length */
+       if (dev->hard_header_len + HSR_HLEN > hsr->dev->hard_header_len)
+               hsr->dev->hard_header_len = dev->hard_header_len + HSR_HLEN;
+
+       dev_set_mtu(hsr->dev, hsr_get_max_mtu(hsr));
+
+       return 0;
+
+fail_rx_handler:
+       dev_set_promiscuity(dev, -1);
+
+fail:
+       dev_put(dev);
+       return res;
+}
+
+void hsr_del_slave(struct hsr_priv *hsr, int idx)
+{
+       struct net_device *slave;
+
+       slave = hsr->slave[idx];
+       hsr->slave[idx] = NULL;
+
+       netdev_update_features(hsr->dev);
+       dev_set_mtu(hsr->dev, hsr_get_max_mtu(hsr));
+
+       if (slave) {
+               netdev_rx_handler_unregister(slave);
+               dev_set_promiscuity(slave, -1);
+       }
+
+       synchronize_rcu();
+       dev_put(slave);
+}

diff --git a/net/hsr/hsr_slave.h b/net/hsr/hsr_slave.h
index ae90c8d0fde443835628c6ad60678f179f0a188c..03c15fda39a81d25209fdc5038b774453d42b967 100644 (file)
--- a/net/hsr/hsr_slave.h
+++ b/net/hsr/hsr_slave.h
@@ -14,7 +14,10 @@
 
 #include <linux/skbuff.h>
 #include <linux/netdevice.h>
+#include "hsr_main.h"
 
+int hsr_add_slave(struct hsr_priv *hsr, struct net_device *dev, int idx);
+void hsr_del_slave(struct hsr_priv *hsr, int idx);
 rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb);
 
 #endif /* __HSR_SLAVE_H */