Re: 2.6.23-rc8 network problem. Mem leak? ip1000a?

[David Miller <davem@davemloft.net>]

From: David Miller <davem@davemloft.net>
Date: Mon, 07 Jan 2008 23:07:09 -0800 (PST)

> From: linux@horizon.com
> Date: 8 Jan 2008 01:52:11 -0500
> 
> > @@ -172,6 +172,10 @@ config IP1000
> >  	select MII
> >  	---help---
> >  	  This driver supports IP1000 gigabit Ethernet cards.
> > +	  It works, but suffers from a memory leak.  Signifcant
> > +	  use will consume unswappable kernel memory until the
> > +	  machine runs out of memory and crashes.  Thus, this
> > +	  driver cannot be considered usable at the the present time.
> 
> This is not how we handle and track bugs.
> 
> Such a patch is inappropriate, and I'd like to ask that you just be
> patient until someone has a chance to try and figure out what the
> problem is.  Or even better, you can try to track down the problem
> yourself since you seem to have a specific interest in this problem.

Actually, the bug is amazingly obvious after a quick scan of this
driver.

ipg_nic_rx_free_skb() is called from various places and is given zero
context to work with.  It assumes that the caller wants
"sp->rx_current % IPG_RFCLIST_LENGTH" to be freed.

But that's not right in most cases.  For example, consider the call in
ipg_nic_rx_with_end().  This function is invoked from ipg_nic_rx()
like so:

	unsigned int curr = sp->rx_current;
 ...
	for (i = 0; i < IPG_MAXRFDPROCESS_COUNT; i++, curr++) {
		unsigned int entry = curr % IPG_RFDLIST_LENGTH;
		struct ipg_rx *rxfd = sp->rxd + entry;

		if (!(rxfd->rfs & le64_to_cpu(IPG_RFS_RFDDONE)))
			break;

		switch (ipg_nic_rx_check_frame_type(dev)) {
 ...
		case Frame_WithEnd:
			ipg_nic_rx_with_end(dev, tp, rxfd, entry);
			break;
 ...
		}
	}

	sp->rx_current = curr;

So sp->rx_current does not correspond to the packet being processed
currently, so ipg_nic_rx_free_skb() will only look at and try to free
only the first packet the above loop tries to processe.

WOW!!!!  Amazing!!!

I invested 30 seconds of code reading to figure out the leak.  A much
better investment of time than adding bogus comments to the Kconfig
help text don't you think? :-)