> Sure, the br_call_ioctl_atomic() wasn't really protecting
> the right thing, but that doesn't mean that you are allowed
> to remove this and totally ignore the problem altogether.
>
> What really needs to happen is to move the br_ioctl.c:ioctl_mutex
> next to br_ioctl_hook in net/socket.c, renaming it to br_ioctl_mutex
> and take this semaphore around any reference there to br_ioctl_hook.
>
> Then you take this semaphore when NULL'ing out br_ioctl_hook.
>
> Don't forget to export the br_ioctl_mutex symbol to modules.
>
> The rest of your patch looks fine.
The race you found is between module unload and ioctl through br_ioctl_hook.
Here is a patch that takes a different approach to solving this by using
try_module_get. It closes both the ioctl and receive hook races with module
unload. The overhead of try_module_get and semaphore operations are probably
about the same since both are based on atomic increment/decrement.
P.s: VLAN has the same race and does no locking.
# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
# ChangeSet 1.1144 -> 1.1145
# net/bridge/br.c 1.10 -> 1.11
# net/socket.c 1.44 -> 1.45
# net/netsyms.c 1.52 -> 1.53
# include/linux/if_bridge.h 1.2 -> 1.3
# net/core/dev.c 1.57 -> 1.58
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/03/21 shemminger@dev4-009.pdx.osdl.net 1.1145
# Get referene to module when using ioctl hook.
# --------------------------------------------
#
diff -Nru a/include/linux/if_bridge.h b/include/linux/if_bridge.h
--- a/include/linux/if_bridge.h Fri Mar 21 11:49:29 2003
+++ b/include/linux/if_bridge.h Fri Mar 21 11:49:29 2003
@@ -101,9 +101,14 @@
struct net_bridge;
struct net_bridge_port;
-extern int (*br_ioctl_hook)(unsigned long arg);
-extern int (*br_handle_frame_hook)(struct sk_buff *skb);
-extern int (*br_should_route_hook)(struct sk_buff **pskb);
+struct net_bridge_hook {
+ struct module *owner;
+ int (*ioctl)(unsigned long arg);
+ int (*handle_frame)(struct sk_buff *skb);
+};
+
+extern int (*br_should_route_hook) (struct sk_buff **);
+extern struct net_bridge_hook br_hook;
#endif
diff -Nru a/net/bridge/br.c b/net/bridge/br.c
--- a/net/bridge/br.c Fri Mar 21 11:49:29 2003
+++ b/net/bridge/br.c Fri Mar 21 11:49:29 2003
@@ -49,8 +49,10 @@
if (br_netfilter_init())
return 1;
#endif
- br_handle_frame_hook = br_handle_frame;
- br_ioctl_hook = br_ioctl_deviceless_stub;
+ br_hook.owner = THIS_MODULE;
+ br_hook.ioctl = br_ioctl_deviceless_stub;
+ br_hook.handle_frame = br_handle_frame;
+
#if defined(CONFIG_ATM_LANE) || defined(CONFIG_ATM_LANE_MODULE)
br_fdb_get_hook = br_fdb_get;
br_fdb_put_hook = br_fdb_put;
@@ -66,13 +68,14 @@
br_netfilter_fini();
#endif
unregister_netdevice_notifier(&br_device_notifier);
- br_ioctl_hook = NULL;
- br_write_lock_bh(BR_NETPROTO_LOCK);
- br_handle_frame_hook = NULL;
- br_write_unlock_bh(BR_NETPROTO_LOCK);
+ /* protected by module refcount */
+ br_hook.owner = NULL;
+ br_hook.ioctl = NULL;
+ br_hook.handle_frame = NULL;
#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 -Nru a/net/core/dev.c b/net/core/dev.c
--- a/net/core/dev.c Fri Mar 21 11:49:29 2003
+++ b/net/core/dev.c Fri Mar 21 11:49:29 2003
@@ -1389,10 +1389,6 @@
}
}
-#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)
{
@@ -1466,12 +1462,15 @@
#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 && try_module_get(br_hook.owner)) {
int ret;
ret = handle_bridge(skb, pt_prev);
- if (br_handle_frame_hook(skb) == 0)
+ if (br_hook.handle_frame(skb) == 0) {
+ module_put(br_hook.owner);
return ret;
+ }
+ module_put(br_hook.owner);
pt_prev = NULL;
}
#endif
diff -Nru a/net/netsyms.c b/net/netsyms.c
--- a/net/netsyms.c Fri Mar 21 11:49:29 2003
+++ b/net/netsyms.c Fri Mar 21 11:49:29 2003
@@ -235,9 +235,8 @@
EXPORT_SYMBOL(scm_detach_fds);
#if defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE)
-EXPORT_SYMBOL(br_handle_frame_hook);
+EXPORT_SYMBOL(br_hook);
#endif
-EXPORT_SYMBOL(br_ioctl_hook);
#ifdef CONFIG_NET_DIVERT
EXPORT_SYMBOL(alloc_divert_blk);
diff -Nru a/net/socket.c b/net/socket.c
--- a/net/socket.c Fri Mar 21 11:49:29 2003
+++ b/net/socket.c Fri Mar 21 11:49:29 2003
@@ -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,7 @@
file, vector, count, tot_len);
}
-int (*br_ioctl_hook)(unsigned long arg);
+struct net_bridge_hook br_hook;
int (*vlan_ioctl_hook)(unsigned long arg);
#ifdef CONFIG_DLCI
@@ -759,11 +760,14 @@
case SIOCSIFBR:
err = -ENOPKG;
#ifdef CONFIG_KMOD
- if (!br_ioctl_hook)
+ if (!br_hook.owner)
request_module("bridge");
#endif
- if (br_ioctl_hook)
- err = br_ioctl_hook(arg);
+ if (try_module_get(br_hook.owner)) {
+ if (br_hook.ioctl)
+ err = br_hook.ioctl(arg);
+ module_put(br_hook.owner);
+ }
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
