Re: [PATCH 2.5.70] e100 initialize fields prior to register_netdevice

Re: [PATCH 2.5.70] e100 initialize fields prior to register_netdevice

[Feldman, Scott]

On Thu, 29 May 2003, Jeff Garzik wrote:

> 
> Stephen Hemminger wrote:
[cut]
> > This patch moves the initialization earlier before registration.  It
> > probably closes other races as well, where some program could access 
> > the e100 driver before the pointers were setup.
> 
> 
> Agreed.  I consider this a rather serious bug, that could cause crashes
> in multiple corner cases; sysfs being only one of those cases.
> 
> I'm going to go ahead and apply this to 2.5.
> 
> Stephen, could I get you to send me a similar patch for 2.4?
> (it doesn't apply cleanly)  It's very much a bug in 2.4 also.
> 
>         Jeff

Wait!  More work is required, otherwise VLANs, checksum offloading and SG
support are broken.  Here's a patch that applies on top of Stephen's to
address this.  USE_IPCB needs to be defined before filling out
netdev->features.

Again, not an issue for 2.4 e100 driver.

-scott

--- linux-2.5.70/drivers/net/e100/e100_main.c.orig	2003-05-29 16:40:00.000000000 -0700
+++ linux-2.5.70/drivers/net/e100/e100_main.c	2003-05-29 16:40:27.000000000 -0700
@@ -614,26 +614,6 @@
 		goto err_dealloc;
 	}
 
-	dev->vlan_rx_register = e100_vlan_rx_register;
-	dev->vlan_rx_add_vid = e100_vlan_rx_add_vid;
-	dev->vlan_rx_kill_vid = e100_vlan_rx_kill_vid;
-	dev->irq = pcid->irq;
-	dev->open = &e100_open;
-	dev->hard_start_xmit = &e100_xmit_frame;
-	dev->stop = &e100_close;
-	dev->change_mtu = &e100_change_mtu;
-	dev->get_stats = &e100_get_stats;
-	dev->set_multicast_list = &e100_set_multi;
-	dev->set_mac_address = &e100_set_mac;
-	dev->do_ioctl = &e100_ioctl;
-	if (bdp->flags & USE_IPCB)
-		dev->features = NETIF_F_SG | NETIF_F_HW_CSUM |
-				NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
-
-	if ((rc = register_netdev(dev)) != 0) {
-		goto err_pci;
-	}
-
 	if (((bdp->pdev->device > 0x1030)
 	       && (bdp->pdev->device < 0x103F))
 	    || ((bdp->pdev->device >= 0x1050)
@@ -657,6 +637,27 @@
 	} else {
 		bdp->rfd_size = 16;
 	}
+
+	dev->vlan_rx_register = e100_vlan_rx_register;
+	dev->vlan_rx_add_vid = e100_vlan_rx_add_vid;
+	dev->vlan_rx_kill_vid = e100_vlan_rx_kill_vid;
+	dev->irq = pcid->irq;
+	dev->open = &e100_open;
+	dev->hard_start_xmit = &e100_xmit_frame;
+	dev->stop = &e100_close;
+	dev->change_mtu = &e100_change_mtu;
+	dev->get_stats = &e100_get_stats;
+	dev->set_multicast_list = &e100_set_multi;
+	dev->set_mac_address = &e100_set_mac;
+	dev->do_ioctl = &e100_ioctl;
+	if (bdp->flags & USE_IPCB)
+		dev->features = NETIF_F_SG | NETIF_F_HW_CSUM |
+				NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
+
+	if ((rc = register_netdev(dev)) != 0) {
+		goto err_pci;
+	}
+
 	e100_check_options(e100nics, bdp);
 
 	if (!e100_init(bdp)) {

Re: [PATCH 2.5.70] e100 initialize fields prior to register_netdevice

[Jeff Garzik]

Your patch doesn't apply at all, even with 'patch -l'.

I backed out Stephen's patch, and will be looking for a fix from you ;-)

	Jeff

RE: [PATCH 2.5.70] e100 initialize fields prior to register_netdevice

[Feldman, Scott]

> Agreed.  I consider this a rather serious bug, that could 
> cause crashes in multiple corner cases; sysfs being only 
> one of those cases.
> 
> I'm going to go ahead and apply this to 2.5.
> 
> Stephen, could I get you to send me a similar patch for 2.4? 
> (it doesn't apply cleanly)  It's very much a bug in 2.4 also.

2.4 e100 should be fine, as this breakage was just introduced in last
patchset for 2.5 (only).

Thanks Stephen.

-scott

[PATCH 2.5.70] e100 initialize fields prior to register_netdevice

[Stephen Hemminger]

The e100 driver sets many of the fields in the netdevice structure after
calling the register function.  This confuses the sysfs setup, because
it uses the presence of the get_stats function pointer to decide whether
to setup a statistics directory or not.  The earlier patch to net-sysfs
keeps this from causing a crash.

This patch moves the initialization earlier before registration.  It probably
closes other races as well, where some program could access the e100 driver
before the pointers were setup.

--- linux-2.5/drivers/net/e100/e100_main.c	2003-05-27 11:04:34.000000000 -0700
+++ linux-2.5-sysfs/drivers/net/e100/e100_main.c	2003-05-29 08:56:47.000000000 -0700
@@ -614,6 +614,22 @@
 		goto err_dealloc;
 	}
 
+	dev->vlan_rx_register = e100_vlan_rx_register;
+	dev->vlan_rx_add_vid = e100_vlan_rx_add_vid;
+	dev->vlan_rx_kill_vid = e100_vlan_rx_kill_vid;
+	dev->irq = pcid->irq;
+	dev->open = &e100_open;
+	dev->hard_start_xmit = &e100_xmit_frame;
+	dev->stop = &e100_close;
+	dev->change_mtu = &e100_change_mtu;
+	dev->get_stats = &e100_get_stats;
+	dev->set_multicast_list = &e100_set_multi;
+	dev->set_mac_address = &e100_set_mac;
+	dev->do_ioctl = &e100_ioctl;
+	if (bdp->flags & USE_IPCB)
+		dev->features = NETIF_F_SG | NETIF_F_HW_CSUM |
+				NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
+
 	if ((rc = register_netdev(dev)) != 0) {
 		goto err_pci;
 	}
@@ -660,23 +676,6 @@
                 goto err_unregister_netdev;
 	}
 	
-	dev->vlan_rx_register = e100_vlan_rx_register;
-	dev->vlan_rx_add_vid = e100_vlan_rx_add_vid;
-	dev->vlan_rx_kill_vid = e100_vlan_rx_kill_vid;
-	dev->irq = pcid->irq;
-	dev->open = &e100_open;
-	dev->hard_start_xmit = &e100_xmit_frame;
-	dev->stop = &e100_close;
-	dev->change_mtu = &e100_change_mtu;
-	dev->get_stats = &e100_get_stats;
-	dev->set_multicast_list = &e100_set_multi;
-	dev->set_mac_address = &e100_set_mac;
-	dev->do_ioctl = &e100_ioctl;
-
-	if (bdp->flags & USE_IPCB)
-	dev->features = NETIF_F_SG | NETIF_F_HW_CSUM |
-			NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
-		
 	e100nics++;
 
 	e100_get_speed_duplex_caps(bdp);