This patch fixes a number of issues with the Ethernet bridging code in
2.5.
Fixes:
1. Locking deadlock between the add/delete interface path and the
add/delete bridge path. Problem was that one path got the rtnl_lock
then the ioctl_mutex, and the other used the opposite order.
The fix for this was to take out the code path based locking (using
ioctl_mutex) and use the existing br->lock rwlock and add another
spinlock for the list of bridges.
2. Race exists with module unloading and br_ioctl_hook. Fixed
this by adding semaphore in socket.c with interface for adding hook.
3. Notify code has a boolean test wrong so it would never notify
interface going down.
3. Ioctl and notify code was too lax about getting br->lock when
necessary. Either no locking was done or reader lock was acquired when
writer lock was necessary.
4. Add C99 initializer for notifier event
Open issues:
* Module reference counting is wrong since the try_module_get
is being done in the module (where it already must have a reference).
* KMOD auto module load is currently busted (probably for all modules).
Other notes:
* The frame_handle_hook does not need to be tested in the receive path
because if net device has handle to bridge port, then ref count is
set and bridge module could not be unloaded.
P.s: On vacation next week, so don't expect immediate response
--------------------------------------------------------------
diff -urN -X dontdiff linux-2.5.65/include/linux/if_bridge.h linux-2.5-ebr/include/linux/if_bridge.h
--- linux-2.5.65/include/linux/if_bridge.h 2003-03-19 13:59:16.000000000 -0800
+++ linux-2.5-ebr/include/linux/if_bridge.h 2003-03-21 14:49:17.000000000 -0800
@@ -101,7 +101,7 @@
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);
diff -urN -X dontdiff linux-2.5.65/net/bridge/br.c linux-2.5-ebr/net/bridge/br.c
--- linux-2.5.65/net/bridge/br.c 2003-03-19 13:59:18.000000000 -0800
+++ linux-2.5-ebr/net/bridge/br.c 2003-03-21 14:48:23.000000000 -0800
@@ -49,8 +49,9 @@
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 @@
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
diff -urN -X dontdiff linux-2.5.65/net/bridge/br_if.c linux-2.5-ebr/net/bridge/br_if.c
--- linux-2.5.65/net/bridge/br_if.c 2003-03-19 13:59:18.000000000 -0800
+++ linux-2.5-ebr/net/bridge/br_if.c 2003-03-21 09:14:24.000000000 -0800
@@ -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 @@
return 0;
}
+/* called with bridge_lock */
static struct net_bridge **__find_br(char *name)
{
struct net_bridge **b;
@@ -188,8 +190,10 @@
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 @@
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 @@
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 @@
indices[i] = br->dev.ifindex;
br = br->next;
}
+ spin_unlock(&bridge_lock);
return i;
}
@@ -289,9 +300,11 @@
{
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);
}
diff -urN -X dontdiff linux-2.5.65/net/bridge/br_ioctl.c linux-2.5-ebr/net/bridge/br_ioctl.c
--- linux-2.5.65/net/bridge/br_ioctl.c 2003-03-19 13:59:18.000000000 -0800
+++ linux-2.5-ebr/net/bridge/br_ioctl.c 2003-03-21 10:25:44.000000000 -0800
@@ -53,6 +53,7 @@
{
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 @@
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 @@
}
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 @@
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 @@
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 @@
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 @@
{
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 @@
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 @@
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 @@
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);
-}
diff -urN -X dontdiff linux-2.5.65/net/bridge/br_notify.c linux-2.5-ebr/net/bridge/br_notify.c
--- linux-2.5.65/net/bridge/br_notify.c 2003-03-19 13:59:18.000000000 -0800
+++ linux-2.5-ebr/net/bridge/br_notify.c 2003-03-20 10:06:12.000000000 -0800
@@ -21,15 +21,14 @@
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 @@
if (p == NULL)
return NOTIFY_DONE;
- switch (event)
+ 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 @@
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;
}
diff -urN -X dontdiff linux-2.5.65/net/core/dev.c linux-2.5-ebr/net/core/dev.c
--- linux-2.5.65/net/core/dev.c 2003-03-19 13:59:18.000000000 -0800
+++ linux-2.5-ebr/net/core/dev.c 2003-03-21 14:50:32.000000000 -0800
@@ -1389,9 +1389,8 @@
}
}
-#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)
@@ -1410,6 +1409,7 @@
return ret;
}
+#endif
#ifdef CONFIG_NET_DIVERT
static inline int handle_diverter(struct sk_buff *skb)
@@ -1466,12 +1466,13 @@
#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)
+ if (br_handle_frame_hook(skb) == 0)
return ret;
+
pt_prev = NULL;
}
#endif
diff -urN -X dontdiff linux-2.5.65/net/netsyms.c linux-2.5-ebr/net/netsyms.c
--- linux-2.5.65/net/netsyms.c 2003-03-19 13:59:18.000000000 -0800
+++ linux-2.5-ebr/net/netsyms.c 2003-03-21 14:46:38.000000000 -0800
@@ -236,8 +236,8 @@
#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);
diff -urN -X dontdiff linux-2.5.65/net/socket.c linux-2.5-ebr/net/socket.c
--- linux-2.5.65/net/socket.c 2003-03-19 13:59:18.000000000 -0800
+++ linux-2.5-ebr/net/socket.c 2003-03-21 14:46:44.000000000 -0800
@@ -70,6 +70,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>
@@ -711,7 +712,17 @@
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
@@ -758,12 +769,16 @@
case SIOCGIFBR:
case SIOCSIFBR:
err = -ENOPKG;
+
#ifdef CONFIG_KMOD
if (!br_ioctl_hook)
request_module("bridge");
#endif
- if (br_ioctl_hook)
+
+ down(&br_ioctl_mutex);
+ if (br_ioctl_hook)
err = br_ioctl_hook(arg);
+ up(&br_ioctl_mutex);
break;
case SIOCGIFVLAN:
case SIOCSIFVLAN:
-
: send the line "unsubscribe linux-net" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
