Re: dev->destructor

[Stephen Hemminger <shemminger@osdl.org>]

[-- Attachment #1: Type: text/plain, Size: 2385 bytes --]

As an experiment, tried acquiring module ref count every time network
device is ref counted.  The result is discovering that there are cases
in the Ethernet module init path where there is a call to dev_hold()
without a previous explicit ref count.

kernel BUG at include/linux/module.h:284!
invalid operand: 0000 [#1]
CPU:    0
EIP:    0060:[<c028fd02>]    Not tainted
EFLAGS: 00010246
EIP is at linkwatch_fire_event+0x170/0x1a3
eax: 00000000   ebx: c047fad0   ecx: 00000020   edx: f88c8100
esi: f88c7100   edi: f6fa7000   ebp: f6f15de4   esp: f6f15dc8
ds: 007b   es: 007b   ss: 0068
Process modprobe (pid: 408, threadinfo=f6f14000 task=f78f46a0)
Stack: f88c7100 c03f7008 00000246 f6f14000 f6fa7000 00000000 fffc829b f6f15df8 
       f88c0ab9 f6fa7000 f6fa71e0 033002a8 f6f15e24 f88c00e0 f6fa71e0 00007148 
       c03e2e80 f6fa7320 c011eb46 ffffffef f6f15e3e f6fa71e0 fffc829b f6f15e50 
Call Trace:
 [<f88c7100>] +0x0/0x1180 [e100]
 [<f88c0ab9>] e100_update_link_state+0x97/0xa2 [e100]
 [<f88c00e0>] e100_find_speed_duplex+0x20/0x26a [e100]
 [<c011eb46>] sys_sched_yield+0xc0/0xfe
 [<f88c07de>] e100_auto_neg+0x114/0x11c [e100]
 [<c01fa4f8>] __delay+0x14/0x18
 [<f88c081d>] e100_phy_set_speed_duplex+0x37/0xa4 [e100]
 [<f88c0997>] e100_phy_init+0x69/0x78 [e100]
 [<f88ba1dc>] e100_hw_init+0x14/0x11e [e100]
 [<f88bc432>] e100_rd_pwa_no+0x32/0x40 [e100]
 [<f88ba058>] e100_init+0xf6/0x126 [e100]
 [<f88b9273>] e100_found1+0x1a9/0x42e [e100]
 [<f88c5b25>] e100_driver_version+0x0/0xb [e100]
 [<f88c5e40>] e100_driver+0x0/0xa0 [e100]
 [<c01fe884>] pci_device_probe+0x5a/0x68
 [<f88c5b60>] e100_id_table+0x0/0x2e0 [e100]
 [<f88c5e68>] e100_driver+0x28/0xa0 [e100]
 [<c0241853>] bus_match+0x43/0x6e
 [<f88c5e68>] e100_driver+0x28/0xa0 [e100]
 [<f88c5e68>] e100_driver+0x28/0xa0 [e100]
 [<c0241956>] driver_attach+0x5c/0x60
 [<f88c5e68>] e100_driver+0x28/0xa0 [e100]
 [<f88c5e68>] e100_driver+0x28/0xa0 [e100]
 [<c0241c2a>] bus_add_driver+0xb2/0xc8
 [<f88c5e68>] e100_driver+0x28/0xa0 [e100]
 [<f88c7100>] +0x0/0x1180 [e100]
 [<c01fe99a>] pci_register_driver+0x46/0x56
 [<f88c5e68>] e100_driver+0x28/0xa0 [e100]
 [<f880c015>] +0x15/0x3e [e100]
 [<f88c5e40>] e100_driver+0x0/0xa0 [e100]
 [<c013b034>] sys_init_module+0x1b0/0x292
all_call+0x7/0xb

Code: 0f 0b 1c 01 97 5a 32 c0 e9 d9 fe ff ff c7 04 24 0c 00 00 00 
 ./ifup: line 91:   408 Segmentation fault      modprobe $1 >/dev/null 2>&1


[-- Attachment #2: netdev-module.diff --]
[-- Type: application/octet-stream, Size: 3843 bytes --]

diff -urNp -X dontdiff linux-2.5/include/asm-i386/asm_offsets.h linux-2.5-dev/include/asm-i386/asm_offsets.h
--- linux-2.5/include/asm-i386/asm_offsets.h	1969-12-31 16:00:00.000000000 -0800
+++ linux-2.5-dev/include/asm-i386/asm_offsets.h	2003-05-05 10:11:50.000000000 -0700
@@ -0,0 +1,22 @@
+#ifndef __ASM_OFFSETS_H__
+#define __ASM_OFFSETS_H__
+/*
+ * DO NOT MODIFY.
+ *
+ * This file was generated by arch/i386/Makefile
+ *
+ */
+
+#define SIGCONTEXT_eax 44 /* offsetof (struct sigcontext, eax) */
+#define SIGCONTEXT_ebx 32 /* offsetof (struct sigcontext, ebx) */
+#define SIGCONTEXT_ecx 40 /* offsetof (struct sigcontext, ecx) */
+#define SIGCONTEXT_edx 36 /* offsetof (struct sigcontext, edx) */
+#define SIGCONTEXT_esi 20 /* offsetof (struct sigcontext, esi) */
+#define SIGCONTEXT_edi 16 /* offsetof (struct sigcontext, edi) */
+#define SIGCONTEXT_ebp 24 /* offsetof (struct sigcontext, ebp) */
+#define SIGCONTEXT_esp 28 /* offsetof (struct sigcontext, esp) */
+#define SIGCONTEXT_eip 56 /* offsetof (struct sigcontext, eip) */
+
+#define RT_SIGFRAME_sigcontext 164 /* offsetof (struct rt_sigframe, uc.uc_mcontext) */
+
+#endif
diff -urNp -X dontdiff linux-2.5/include/linux/netdevice.h linux-2.5-dev/include/linux/netdevice.h
--- linux-2.5/include/linux/netdevice.h	2003-04-14 13:32:21.000000000 -0700
+++ linux-2.5-dev/include/linux/netdevice.h	2003-05-05 10:06:19.000000000 -0700
@@ -29,6 +29,7 @@
 #include <linux/if_ether.h>
 #include <linux/if_packet.h>
 #include <linux/kobject.h>
+#include <linux/module.h>
 
 #include <asm/atomic.h>
 #include <asm/cache.h>
@@ -629,12 +630,32 @@ extern int netdev_finish_unregister(stru
 
 static inline void dev_put(struct net_device *dev)
 {
+	module_put(dev->owner);
 	if (atomic_dec_and_test(&dev->refcnt))
 		netdev_finish_unregister(dev);
 }
 
-#define __dev_put(dev) atomic_dec(&(dev)->refcnt)
-#define dev_hold(dev) atomic_inc(&(dev)->refcnt)
+static inline void __dev_put(struct net_device *dev)
+{
+	module_put(dev->owner);
+	atomic_dec(&dev->refcnt);
+}
+
+static inline void dev_hold(struct net_device *dev)
+{
+	__module_get(dev->owner);
+	atomic_inc(&dev->refcnt);
+}
+
+static inline int dev_try_hold(struct net_device *dev)
+{
+	int ret = 0;
+	if (try_module_get(dev->owner)){
+		atomic_inc(&dev->refcnt);
+		ret = 1;
+	}
+	return ret;
+}
 
 /* Carrier loss detection, dial on demand. The functions netif_carrier_on
  * and _off may be called from IRQ context, but it is caller
diff -urNp -X dontdiff linux-2.5/net/core/dev.c linux-2.5-dev/net/core/dev.c
--- linux-2.5/net/core/dev.c	2003-05-05 09:41:03.000000000 -0700
+++ linux-2.5-dev/net/core/dev.c	2003-05-05 10:06:20.000000000 -0700
@@ -1,3 +1,4 @@
+#define NET_REFCNT_DEBUG	1
 /*
  * 	NET3	Protocol independent device support routines.
  *
@@ -440,8 +441,8 @@ struct net_device *dev_get_by_name(const
 
 	read_lock(&dev_base_lock);
 	dev = __dev_get_by_name(name);
-	if (dev)
-		dev_hold(dev);
+	if (dev && !dev_try_hold(dev))
+		dev = NULL;
 	read_unlock(&dev_base_lock);
 	return dev;
 }
@@ -513,8 +514,8 @@ struct net_device *dev_get_by_index(int 
 
 	read_lock(&dev_base_lock);
 	dev = __dev_get_by_index(ifindex);
-	if (dev)
-		dev_hold(dev);
+	if (dev && !dev_try_hold(dev))
+		dev = NULL;
 	read_unlock(&dev_base_lock);
 	return dev;
 }
@@ -563,8 +564,8 @@ struct net_device * dev_get_by_flags(uns
 
 	read_lock(&dev_base_lock);
 	dev = __dev_get_by_flags(if_flags, mask);
-	if (dev)
-		dev_hold(dev);
+	if (dev && !dev_try_hold(dev))
+	    dev = NULL;
 	read_unlock(&dev_base_lock);
 	return dev;
 }
@@ -2609,10 +2610,11 @@ int register_netdevice(struct net_device
 	set_bit(__LINK_STATE_PRESENT, &dev->state);
 
 	dev->next = NULL;
+	atomic_inc(&dev->refcnt);
 	dev_init_scheduler(dev);
 	write_lock_bh(&dev_base_lock);
 	*dp = dev;
-	dev_hold(dev);
+
 	dev->deadbeaf = 0;
 	write_unlock_bh(&dev_base_lock);