From: Arvid Brodin <arvid.brodin@alten.se>
To: "David S. Miller" <davem@davemloft.net>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: [PATCH net-next 05/10] net/hsr: Move slave init to hsr_slave.c.
Date: Fri, 4 Jul 2014 23:37:27 +0200 [thread overview]
Message-ID: <53B71E97.6030106@alten.se> (raw)
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>
---
net/hsr/hsr_device.c | 192 ++++++++++++++-----------------------------------
net/hsr/hsr_framereg.c | 6 +-
net/hsr/hsr_main.c | 24 ++++---
net/hsr/hsr_netlink.c | 53 ++++++++++----
net/hsr/hsr_slave.c | 91 +++++++++++++++++++++++
net/hsr/hsr_slave.h | 3 +
6 files changed, 205 insertions(+), 164 deletions(-)
diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index 8936b3c..1f8869c 100644
--- 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 79e3f7f..3666f94 100644
--- 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 431b528..b5abe26 100644
--- 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 bea250e..a2ce359 100644
--- 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 7028146..d676090 100644
--- 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 ae90c8d..03c15fd 100644
--- 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 */
--
1.8.3.2
--
Arvid Brodin | Consultant (Linux)
ALTEN | Knarrarnäsgatan 7 | SE-164 40 Kista | Sweden
arvid.brodin@alten.se | www.alten.se/en/
reply other threads:[~2014-07-04 21:45 UTC|newest]
Thread overview: [no followups] expand[flat|nested] mbox.gz Atom feed
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53B71E97.6030106@alten.se \
--to=arvid.brodin@alten.se \
--cc=davem@davemloft.net \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.