[BRIDGE]: Fix several locking bugs, plus cleanups.

include/linux/if_bridge.h

@@ -101,7 +101,7 @@ struct __fdb_entry
 struct net_bridge;
 struct net_bridge_port;
 
-extern int (*br_ioctl_hook)(unsigned long arg);
+extern void brioctl_set(int (*ioctl_hook)(unsigned long));
 extern int (*br_handle_frame_hook)(struct sk_buff *skb);
 extern int (*br_should_route_hook)(struct sk_buff **pskb);
 

net/bridge/br.c

@@ -49,8 +49,9 @@ static int __init br_init(void)
 	if (br_netfilter_init())
 		return 1;
 #endif
+	brioctl_set(br_ioctl_deviceless_stub);
 	br_handle_frame_hook = br_handle_frame;
-	br_ioctl_hook = br_ioctl_deviceless_stub;
+
 #if defined(CONFIG_ATM_LANE) || defined(CONFIG_ATM_LANE_MODULE)
 	br_fdb_get_hook = br_fdb_get;
 	br_fdb_put_hook = br_fdb_put;
@@ -60,24 +61,18 @@ static int __init br_init(void)
 	return 0;
 }
 
-static void __br_clear_ioctl_hook(void)
-{
-	br_ioctl_hook = NULL;
-}
-
 static void __exit br_deinit(void)
 {
 #ifdef CONFIG_NETFILTER
 	br_netfilter_fini();
 #endif
 	unregister_netdevice_notifier(&br_device_notifier);
-	br_call_ioctl_atomic(__br_clear_ioctl_hook);
 
-	br_write_lock_bh(BR_NETPROTO_LOCK);
+	brioctl_set(NULL);
 	br_handle_frame_hook = NULL;
-	br_write_unlock_bh(BR_NETPROTO_LOCK);
 
 #if defined(CONFIG_ATM_LANE) || defined(CONFIG_ATM_LANE_MODULE)
+	/* FIX ME. move into hook structure with ref count */
 	br_fdb_get_hook = NULL;
 	br_fdb_put_hook = NULL;
 #endif

net/bridge/br_if.c

@@ -23,6 +23,7 @@
 #include "br_private.h"
 
 static struct net_bridge *bridge_list;
+static spinlock_t bridge_lock = SPIN_LOCK_UNLOCKED;
 
 static int br_initial_port_cost(struct net_device *dev)
 {
@@ -69,6 +70,7 @@ static int __br_del_if(struct net_bridge *br, struct net_device *dev)
 	return 0;
 }
 
+/* called with bridge_lock */
 static struct net_bridge **__find_br(char *name)
 {
 	struct net_bridge **b;
@@ -188,8 +190,10 @@ int br_add_bridge(char *name)
 		return -EEXIST;
 	}
 
+	spin_lock(&bridge_lock);
 	br->next = bridge_list;
 	bridge_list = br;
+	spin_unlock(&bridge_lock);
 
 	br_inc_use_count();
 	register_netdev(&br->dev);
@@ -202,17 +206,22 @@ int br_del_bridge(char *name)
 	struct net_bridge **b;
 	struct net_bridge *br;
 
-	if ((b = __find_br(name)) == NULL)
+	spin_lock(&bridge_lock);
+	if ((b = __find_br(name)) == NULL) {
+		spin_unlock(&bridge_lock);
 		return -ENXIO;
+	}
 
 	br = *b;
-
-	if (br->dev.flags & IFF_UP)
+	if (br->dev.flags & IFF_UP) {
+		spin_unlock(&bridge_lock);
 		return -EBUSY;
-
-	del_ifs(br);
+	}
 
 	*b = br->next;
+	spin_unlock(&bridge_lock);
+
+	del_ifs(br);
 
 	unregister_netdev(&br->dev);
 	kfree(br);
@@ -272,6 +281,7 @@ int br_get_bridge_ifindices(int *indices, int num)
 	struct net_bridge *br;
 	int i;
 
+	spin_lock(&bridge_lock);
 	br = bridge_list;
 	for (i=0;i<num;i++) {
 		if (br == NULL)
@@ -280,6 +290,7 @@ int br_get_bridge_ifindices(int *indices, int num)
 		indices[i] = br->dev.ifindex;
 		br = br->next;
 	}
+	spin_unlock(&bridge_lock);
 
 	return i;
 }
@@ -289,9 +300,11 @@ void br_get_port_ifindices(struct net_bridge *br, int *ifindices)
 {
 	struct net_bridge_port *p;
 
+	read_lock(&br->lock);
 	p = br->port_list;
 	while (p != NULL) {
 		ifindices[p->port_no] = p->dev->ifindex;
 		p = p->next;
 	}
+	read_unlock(&br->lock);
 }

net/bridge/br_ioctl.c

@@ -53,6 +53,7 @@ static int br_ioctl_device(struct net_bridge *br,
 	{
 		struct __bridge_info b;
 
+	        read_lock(&br->lock);
 		memset(&b, 0, sizeof(struct __bridge_info));
 		memcpy(&b.designated_root, &br->designated_root, 8);
 		memcpy(&b.bridge_id, &br->bridge_id, 8);
@@ -73,6 +74,7 @@ static int br_ioctl_device(struct net_bridge *br,
 		b.tcn_timer_value = br_timer_get_residue(&br->tcn_timer);
 		b.topology_change_timer_value = br_timer_get_residue(&br->topology_change_timer);
 		b.gc_timer_value = br_timer_get_residue(&br->gc_timer);
+	        read_unlock(&br->lock);
 
 		if (copy_to_user((void *)arg0, &b, sizeof(b)))
 			return -EFAULT;
@@ -96,21 +98,27 @@ static int br_ioctl_device(struct net_bridge *br,
 	}
 
 	case BRCTL_SET_BRIDGE_FORWARD_DELAY:
+		write_lock(&br->lock);
 		br->bridge_forward_delay = arg0;
 		if (br_is_root_bridge(br))
 			br->forward_delay = arg0;
+		write_unlock(&br->lock);
 		return 0;
 
 	case BRCTL_SET_BRIDGE_HELLO_TIME:
+		write_lock(&br->lock);
 		br->bridge_hello_time = arg0;
 		if (br_is_root_bridge(br))
 			br->hello_time = arg0;
+		write_unlock(&br->lock);
 		return 0;
 
 	case BRCTL_SET_BRIDGE_MAX_AGE:
+		write_lock(&br->lock);
 		br->bridge_max_age = arg0;
 		if (br_is_root_bridge(br))
 			br->max_age = arg0;
+		write_unlock(&br->lock);
 		return 0;
 
 	case BRCTL_SET_AGEING_TIME:
@@ -126,6 +134,7 @@ static int br_ioctl_device(struct net_bridge *br,
 		struct __port_info p;
 		struct net_bridge_port *pt;
 
+		read_lock(&br->lock);
 		if ((pt = br_get_port(br, arg1)) == NULL)
 			return -EINVAL;
 
@@ -143,6 +152,8 @@ static int br_ioctl_device(struct net_bridge *br,
 		p.forward_delay_timer_value = br_timer_get_residue(&pt->forward_delay_timer);
 		p.hold_timer_value = br_timer_get_residue(&pt->hold_timer);
 
+		read_unlock(&br->lock);
+
 		if (copy_to_user((void *)arg0, &p, sizeof(p)))
 			return -EFAULT;
 
@@ -154,16 +165,20 @@ static int br_ioctl_device(struct net_bridge *br,
 		return 0;
 
 	case BRCTL_SET_BRIDGE_PRIORITY:
+		write_lock(&br->lock);
 		br_stp_set_bridge_priority(br, arg0);
+		write_unlock(&br->lock);
 		return 0;
 
 	case BRCTL_SET_PORT_PRIORITY:
 	{
 		struct net_bridge_port *p;
 
+		write_lock(&br->lock);
 		if ((p = br_get_port(br, arg0)) == NULL)
 			return -EINVAL;
 		br_stp_set_port_priority(p, arg1);
+		write_unlock(&br->lock);
 		return 0;
 	}
 
@@ -171,9 +186,11 @@ static int br_ioctl_device(struct net_bridge *br,
 	{
 		struct net_bridge_port *p;
 
+		write_lock(&br->lock);
 		if ((p = br_get_port(br, arg0)) == NULL)
 			return -EINVAL;
 		br_stp_set_path_cost(p, arg1);
+		write_unlock(&br->lock);
 		return 0;
 	}
 
@@ -230,11 +247,9 @@ static int br_ioctl_deviceless(unsigned int cmd,
 	return -EOPNOTSUPP;
 }
 
-static DECLARE_MUTEX(ioctl_mutex);
 
 int br_ioctl_deviceless_stub(unsigned long arg)
 {
-	int err;
 	unsigned long i[3];
 
 	if (!capable(CAP_NET_ADMIN))
@@ -243,11 +258,7 @@ int br_ioctl_deviceless_stub(unsigned long arg)
 	if (copy_from_user(i, (void *)arg, 3*sizeof(unsigned long)))
 		return -EFAULT;
 
-	down(&ioctl_mutex);
-	err = br_ioctl_deviceless(i[0], i[1], i[2]);
-	up(&ioctl_mutex);
-
-	return err;
+	return br_ioctl_deviceless(i[0], i[1], i[2]);
 }
 
 int br_ioctl(struct net_bridge *br, unsigned int cmd, unsigned long arg0, unsigned long arg1, unsigned long arg2)
@@ -257,18 +268,9 @@ int br_ioctl(struct net_bridge *br, unsigned int cmd, unsigned long arg0, unsign
 	if (!capable(CAP_NET_ADMIN))
 		return -EPERM;
 
-	down(&ioctl_mutex);
 	err = br_ioctl_deviceless(cmd, arg0, arg1);
 	if (err == -EOPNOTSUPP)
 		err = br_ioctl_device(br, cmd, arg0, arg1, arg2);
-	up(&ioctl_mutex);
 
 	return err;
 }
-
-void br_call_ioctl_atomic(void (*fn)(void))
-{
-	down(&ioctl_mutex);
-	fn();
-	up(&ioctl_mutex);
-}

net/bridge/br_notify.c

@@ -21,15 +21,14 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
 
 struct notifier_block br_device_notifier =
 {
-	br_device_event,
-	NULL,
-	0
+	.notifier_call = br_device_event
 };
 
 static int br_device_event(struct notifier_block *unused, unsigned long event, void *ptr)
 {
 	struct net_device *dev;
 	struct net_bridge_port *p;
+	struct net_bridge *br;
 
 	dev = ptr;
 	p = dev->br_port;
@@ -37,13 +36,15 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
 	if (p == NULL)
 		return NOTIFY_DONE;
 
+	br = p->br;
+
 	switch (event) 
 	{
 	case NETDEV_CHANGEADDR:
-		read_lock(&p->br->lock);
+		write_lock_bh(&br->lock);
 		br_fdb_changeaddr(p, dev->dev_addr);
-		br_stp_recalculate_bridge_id(p->br);
-		read_unlock(&p->br->lock);
+		br_stp_recalculate_bridge_id(br);
+		write_unlock_bh(&br->lock);
 		break;
 
 	case NETDEV_GOING_DOWN:
@@ -51,23 +52,23 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
 		break;
 
 	case NETDEV_DOWN:
-		if (p->br->dev.flags & IFF_UP) {
-			read_lock(&p->br->lock);
-			br_stp_disable_port(dev->br_port);
-			read_unlock(&p->br->lock);
+		if (br->dev.flags & IFF_UP) {
+			write_lock_bh(&br->lock);
+			br_stp_disable_port(p);
+			write_unlock_bh(&br->lock);
 		}
 		break;
 
 	case NETDEV_UP:
-		if (p->br->dev.flags & IFF_UP) {
-			read_lock(&p->br->lock);
-			br_stp_enable_port(dev->br_port);
-			read_unlock(&p->br->lock);
+		if (!(br->dev.flags & IFF_UP)) {
+			write_lock_bh(&br->lock);
+			br_stp_enable_port(p);
+			write_unlock_bh(&br->lock);
 		}
 		break;
 
 	case NETDEV_UNREGISTER:
-		br_del_if(dev->br_port->br, dev);
+		br_del_if(br, dev);
 		break;
 	}
 

net/core/dev.c

@@ -1433,9 +1433,8 @@ static void net_tx_action(struct softirq_action *h)
 	}
 }
 
-#if defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE)
+#if defined(CONFIG_BRIDGE) || defined (CONFIG_BRIDGE_MODULE)
 int (*br_handle_frame_hook)(struct sk_buff *skb) = NULL;
-#endif
 
 static __inline__ int handle_bridge(struct sk_buff *skb,
 				     struct packet_type *pt_prev)
@@ -1454,6 +1453,7 @@ static __inline__ int handle_bridge(struct sk_buff *skb,
 	return ret;
 }
 
+#endif
 
 #ifdef CONFIG_NET_DIVERT
 static inline int handle_diverter(struct sk_buff *skb)
@@ -1510,12 +1510,13 @@ int netif_receive_skb(struct sk_buff *skb)
 #endif /* CONFIG_NET_DIVERT */
 
 #if defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE)
-	if (skb->dev->br_port && br_handle_frame_hook) {
+	if (skb->dev->br_port) {
 		int ret;
 
 		ret = handle_bridge(skb, pt_prev);
 		if (br_handle_frame_hook(skb) == 0) 
 			return ret;
+
 		pt_prev = NULL;
 	}
 #endif

net/netsyms.c

@@ -234,8 +234,8 @@ EXPORT_SYMBOL(scm_detach_fds);
 
 #if defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE)
 EXPORT_SYMBOL(br_handle_frame_hook);
+EXPORT_SYMBOL(brioctl_set);
 #endif
-EXPORT_SYMBOL(br_ioctl_hook);
 
 #ifdef CONFIG_NET_DIVERT
 EXPORT_SYMBOL(alloc_divert_blk);

net/socket.c

@@ -71,6 +71,7 @@
 #include <linux/wanrouter.h>
 #include <linux/netlink.h>
 #include <linux/rtnetlink.h>
+#include <linux/if_bridge.h>
 #include <linux/init.h>
 #include <linux/poll.h>
 #include <linux/cache.h>
@@ -712,7 +713,18 @@ static ssize_t sock_writev(struct file *file, const struct iovec *vector,
 				 file, vector, count, tot_len);
 }
 
-int (*br_ioctl_hook)(unsigned long arg);
+
+static DECLARE_MUTEX(br_ioctl_mutex);
+static int (*br_ioctl_hook)(unsigned long arg) = NULL;
+
+void brioctl_set(int (*hook)(unsigned long))
+{
+	down(&br_ioctl_mutex);
+	br_ioctl_hook = hook;
+	up(&br_ioctl_mutex);
+}
+
+
 int (*vlan_ioctl_hook)(unsigned long arg);
 
 #ifdef CONFIG_DLCI
@@ -759,12 +771,16 @@ static int sock_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
 		case SIOCGIFBR:
 		case SIOCSIFBR:
 			err = -ENOPKG;
+			
 #ifdef CONFIG_KMOD
 			if (!br_ioctl_hook)
 				request_module("bridge");
 #endif
+
+			down(&br_ioctl_mutex);
 			if (br_ioctl_hook) 
 				err = br_ioctl_hook(arg);
+			up(&br_ioctl_mutex);
 			break;
 		case SIOCGIFVLAN:
 		case SIOCSIFVLAN: