[PATCH] 8139too.c

[PATCH] 8139too.c

[kumon]

In 8139too.c, skb is used after it is freed.
Look at the argument of RTL_W32_F() in the following patch.

8 % diff -u -10 8139too.c  /tmp/8139too.c 
--- 8139too.c	Sun Nov 25 10:46:36 2001
+++ /tmp/8139too.c	Fri Nov 30 19:50:48 2001
@@ -1635,40 +1635,40 @@
 {
 	struct rtl8139_private *tp = dev->priv;
 	void *ioaddr = tp->mmio_addr;
 	unsigned int entry;
 
 	/* Calculate the next Tx descriptor entry. */
 	entry = tp->cur_tx % NUM_TX_DESC;
 
 	if (likely(skb->len < TX_BUF_SIZE)) {
 		skb_copy_and_csum_dev(skb, tp->tx_buf[entry]);
-		dev_kfree_skb(skb);
 	} else {
 		dev_kfree_skb(skb);
 		tp->stats.tx_dropped++;
 		return 0;
   	}
 
 	/* Note: the chip doesn't have auto-pad! */
 	spin_lock_irq(&tp->lock);
 	RTL_W32_F (TxStatus0 + (entry * sizeof (u32)),
 		   tp->tx_flag | (skb->len >= ETH_ZLEN ? skb->len : ETH_ZLEN));
 
 	dev->trans_start = jiffies;
 
 	tp->cur_tx++;
 	wmb();
 
 	if ((tp->cur_tx - NUM_TX_DESC) == tp->dirty_tx)
 		netif_stop_queue (dev);
 	spin_unlock_irq(&tp->lock);
+	dev_kfree_skb(skb);
 
 	DPRINTK ("%s: Queued Tx packet at %p size %u to slot %d.\n",
 		 dev->name, skb->data, skb->len, entry);
 
 	return 0;
 }
 
 
 static void rtl8139_tx_interrupt (struct net_device *dev,
 				  struct rtl8139_private *tp,


--
Software Laboratory, Fujitsu Labs.
kumon@flab.fujitsu.co.jp

Re: [PATCH] 8139too.c

[Andreas Dilger]

On Nov 30, 2001  20:23 +0900, kumon@flab.fujitsu.co.jp wrote:
> In 8139too.c, skb is used after it is freed.
> Look at the argument of RTL_W32_F() in the following patch.
> 
> 8 % diff -u -10 8139too.c  /tmp/8139too.c 
> --- 8139too.c	Sun Nov 25 10:46:36 2001
> +++ /tmp/8139too.c	Fri Nov 30 19:50:48 2001
      ^^^^^^^^^^^^^^
Please try to keep patches relative to the linux/ directory, so they can
be applied via "patch -p1" as most other kernel patches are.  Also note
that you should CC the driver maintainer if you want to get the patch
into the kernel (in this case Jeff Garzik <jgarzik@mandrakesoft.com>).

> +	dev_kfree_skb(skb);
>  
>  	DPRINTK ("%s: Queued Tx packet at %p size %u to slot %d.\n",
>  		 dev->name, skb->data, skb->len, entry);
                            ^^^^^^^^^^^^^^^^^^^
Your patch still does not fix the problems.  Since we only use "skb->len"
(except for the debug case, where we also use the skb->data pointer),
rather just copy skb->len into a local variable and free skb early, so
it can be used again, rather than holding it over a spin lock.

I don't know if anyone really cares about the skb->data pointer in the
DPRINTK.  If so, we could move it up before we free the skb, but I
thought it incorrect to do so, because it is not queued at that time,
and I didn't want to add a temp variable that only appears in a debug.

Cheers, Andreas
--- linux/drivers/net/8139too.c.orig	Sun Nov 25 10:46:36 2001
+++ linux/drivers/net/8139too.c	Fri Nov 30 19:50:48 2001
@@ -1635,35 +1635,36 @@
 	struct rtl8139_private *tp = dev->priv;
 	void *ioaddr = tp->mmio_addr;
 	unsigned int entry;
+	unsigned int len = skb->len;
 
 	/* Calculate the next Tx descriptor entry. */
 	entry = tp->cur_tx % NUM_TX_DESC;
 
-	if (likely(skb->len < TX_BUF_SIZE)) {
+	if (likely(len < TX_BUF_SIZE)) {
 		skb_copy_and_csum_dev(skb, tp->tx_buf[entry]);
 		dev_kfree_skb(skb);
 	} else {
 		dev_kfree_skb(skb);
 		tp->stats.tx_dropped++;
 		return 0;
-  	}
+	}
 
 	/* Note: the chip doesn't have auto-pad! */
 	spin_lock_irq(&tp->lock);
 	RTL_W32_F (TxStatus0 + (entry * sizeof (u32)),
-		   tp->tx_flag | (skb->len >= ETH_ZLEN ? skb->len : ETH_ZLEN));
+		   tp->tx_flag | max(len, (unsigned int)ETH_ZLEN));
 
 	dev->trans_start = jiffies;
 
 	tp->cur_tx++;
 	wmb();
 
 	if ((tp->cur_tx - NUM_TX_DESC) == tp->dirty_tx)
 		netif_stop_queue (dev);
 	spin_unlock_irq(&tp->lock);
 
-	DPRINTK ("%s: Queued Tx packet at %p size %u to slot %d.\n",
-		 dev->name, skb->data, skb->len, entry);
+	DPRINTK ("%s: Queued Tx packet size %u to slot %d.\n",
+		 dev->name, len, entry);
 
 	return 0;
 }

--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/

Re: [PATCH] 8139too.c

[Jeff Garzik]

Andreas Dilger wrote:
> Please try to keep patches relative to the linux/ directory, so they can
> be applied via "patch -p1" as most other kernel patches are.  Also note
> that you should CC the driver maintainer if you want to get the patch
> into the kernel (in this case Jeff Garzik <jgarzik@mandrakesoft.com>).

Kumon forgot... he resent privately

> --- linux/drivers/net/8139too.c.orig    Sun Nov 25 10:46:36 2001
> +++ linux/drivers/net/8139too.c Fri Nov 30 19:50:48 2001

patch applied

> -                  tp->tx_flag | (skb->len >= ETH_ZLEN ? skb->len : ETH_ZLEN));
> +                  tp->tx_flag | max(len, (unsigned int)ETH_ZLEN));

style:  use max_t if you need to cast.  I'll fix it up.

thanks to you both,

	Jeff


-- 
Jeff Garzik      | Only so many songs can be sung
Building 1024    | with two lips, two lungs, and one tongue.
MandrakeSoft     |         - nomeansno

[patch] 8139too.c

[Andrew Morton]

- Clear current->blocked in the kernel thread.  We shouldn't
  be inheriting this from the process which opens the interface.

- Fixed a few printk warnings which are coming out
  when RTL8139_DEBUG is defined.

- Killed the undefined and unused module parm `debug' (finally!)

- Fixed a potential buffer overrun when setting current->comm[].

  Currently, if the user renames "eth0" to "my-nifty-realtek-nic"
  with SIOCSIFNAME and then tries to open it, the kernel thread
  will scrog its own task_struct.

  The kernel thread is now simply called "[eth0]".


I think it would be better to use boring old waitqueues for this
stuff, rather than signals....




--- linux-2.4.0-test13-pre3/drivers/net/8139too.c	Tue Dec 12 19:24:18 2000
+++ linux-akpm/drivers/net/8139too.c	Tue Dec 19 02:15:23 2000
@@ -74,6 +74,9 @@
 		
 		Tobias Ringström - Rx interrupt status checking suggestion
 
+		Andrew Morton - (v0.9.13): clear blocked signals, avoid
+		buffer overrun setting current->comm.
+
 	Submitting bug reports:
 
 		"rtl8139-diag -mmmaaavvveefN" output
@@ -147,7 +150,7 @@
 #include <asm/io.h>
 
 
-#define RTL8139_VERSION "0.9.12"
+#define RTL8139_VERSION "0.9.13"
 #define MODNAME "8139too"
 #define RTL8139_DRIVER_NAME   MODNAME " Fast Ethernet driver " RTL8139_VERSION
 #define PFX MODNAME ": "
@@ -536,7 +539,6 @@
 MODULE_DESCRIPTION ("RealTek RTL-8139 Fast Ethernet driver");
 MODULE_PARM (multicast_filter_limit, "i");
 MODULE_PARM (max_interrupt_work, "i");
-MODULE_PARM (debug, "i");
 MODULE_PARM (media, "1-" __MODULE_STRING(8) "i");
 
 static int read_eeprom (void *ioaddr, int location, int addr_len);
@@ -1461,7 +1463,7 @@
 	DPRINTK ("%s: Media selection tick, Link partner %4.4x.\n",
 		 dev->name, RTL_R16 (NWayLPAR));
 	DPRINTK ("%s:  Other registers are IntMask %4.4x IntStatus %4.4x"
-		 " RxStatus %4.4x.\n", dev->name,
+		 " RxStatus %4.4lx.\n", dev->name,
 		 RTL_R16 (IntrMask),
 		 RTL_R16 (IntrStatus),
 		 RTL_R32 (RxEarlyStatus));
@@ -1478,7 +1480,13 @@
 	unsigned long timeout;
 
 	daemonize ();
-	sprintf (current->comm, "k8139d-%s", dev->name);
+	spin_lock_irq(&current->sigmask_lock);
+	sigemptyset(&current->blocked);
+	recalc_sigpending(current);
+	spin_unlock_irq(&current->sigmask_lock);
+
+	strncpy (current->comm, dev->name, sizeof(current->comm) - 1);
+	current->comm[sizeof(current->comm) - 1] = '\0';
 
 	while (1) {
 		timeout = next_tick;
@@ -2136,7 +2144,7 @@
 
 	DPRINTK ("ENTER\n");
 
-	DPRINTK ("%s:   rtl8139_set_rx_mode(%4.4x) done -- Rx config %8.8x.\n",
+	DPRINTK ("%s:   rtl8139_set_rx_mode(%4.4x) done -- Rx config %8.8lx.\n",
 			dev->name, dev->flags, RTL_R32 (RxConfig));
 
 	/* Note: do not reorder, GCC is clever about common statements. */
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/