[CHECKER] Resource leaks in driver shutdown functions

[CHECKER] Resource leaks in driver shutdown functions

[Ken Ashcraft]

When device drivers have constructor (ctor) functions that allocate
resources, those resources must be freed in the corresponding destructor
(dtor) functions.  What I've done is pair ctor/dtor functions where I
could in the drivers (i.e. init/shutdown, open/close, attach/detach, etc.)
and then found functions like request_irq/free_irq that should be paired
within those ctor/dtor pairs.  Any dtor that doesn't free a resource
allocated in the ctor is marked as an error.  The resource allocation line
numbers are given in the error messages.

The resource allocation/freeing functions in question below are:
irq_request/free_irq
netif_start_queue/netif_stop_queue
kmalloc/kfree
ei_open/ei_close
netif_device_attach/netif_device_detach
parport_register_device/parport_unregister_device

If these functions do not always need to be paired, I would appreciate an
explanation.

This is an experimental checker, so be warned that these reports are not
necessarily valid.

If you are CC'd on this email, it is because I think you are the
maintainer for some of the code below.  Search for your email address
below to find it.

Thanks for your feedback,
Ken Ashcraft

#  Total 				  = 14
# BUGs	|	File Name
1	|	/drivers/acpi/thermal.c
1	|	/drivers/usb/serial/ir-usb.c
1	|	/drivers/net/wireless/wl3501_cs.c
1	|	/drivers/net/sis900.c
1	|	/drivers/net/ns83820.c
1	|	/drivers/net/smc-ultra32.c
1	|	/drivers/net/tokenring/lanstreamer.c
1	|	/drivers/net/hamradio/hdlcdrv.c
1	|	/drivers/net/plip.c
1	|	/drivers/net/smc-mca.c
1	|	/drivers/net/smc-ultra.c
1	|	/drivers/bluetooth/hci_vhci.c
1	|	/drivers/net/ibmlana.c
1	|	/drivers/net/rcpci45.c
---------------------------------------------------------
[BUG] Pete Popov and Brian Moyle, ported by Alan Cox
/home/kash/linux/2.6.3/linux-2.6.3/drivers/net/rcpci45.c:731:RCclose:ERROR:INTERFACE_A_B:
Not calling free_irq. See RCopen:339 [COUNTER=request_irq-free_irq]
[fit=1] [fit_fn=2] [fn_ex=0] [fn_counter=1] [ex=97] [counter=2] [z =
4.47241827790349] [fn-z = -2]
	} else
		printk (KERN_WARNING "%s: unexpected timer irq\n", dev->name);
}

static int

Error --->
RCclose (struct net_device *dev)
{
	PDPA pDpa = dev->priv;

---------------------------------------------------------
[BUG] linux-ns83820@kvack.org
/home/kash/linux/2.6.3/linux-2.6.3/drivers/net/ns83820.c:1385:ns83820_stop:ERROR:INTERFACE_A_B:
Not calling netif_stop_queue. See ns83820_open:1507
[COUNTER=netif_start_queue-netif_stop_queue] [fit=2] [fit_fn=3] [fn_ex=0]
[fn_counter=1] [ex=99] [counter=5] [z = 3.87329366897913] [fn-z = -2]
		schedule();
	} while (readl(dev->base + CR) & which);
	Dprintk("okay!\n");
}


Error --->
static int ns83820_stop(struct net_device *ndev)
{
	struct ns83820 *dev = PRIV(ndev);

---------------------------------------------------------
[BUG] alfred@ccac.rwth-aachen.de, aarnold@elsa.de
/home/kash/linux/2.6.3/linux-2.6.3/drivers/net/ibmlana.c:799:ibmlana_close:ERROR:INTERFACE_A_B:
Not calling netif_stop_queue. See ibmlana_open:793
[COUNTER=netif_start_queue-netif_stop_queue] [fit=2] [fit_fn=4] [fn_ex=0]
[fn_counter=1] [ex=99] [counter=5] [z = 3.87329366897913] [fn-z = -2]
	return 0;
}

/* close driver.  Shut down board and free allocated resources */


Error --->
static int ibmlana_close(struct net_device *dev)
{
	/* turn off board */

---------------------------------------------------------
[BUG] sailer@ife.ee.ethz.ch
/home/kash/linux/2.6.3/linux-2.6.3/drivers/net/hamradio/hdlcdrv.c:582:hdlcdrv_close:ERROR:INTERFACE_A_B:
Not calling netif_stop_queue. See hdlcdrv_open:573
[COUNTER=netif_start_queue-netif_stop_queue] [fit=2] [fit_fn=5] [fn_ex=0]
[fn_counter=1] [ex=99] [counter=5] [z = 3.87329366897913] [fn-z = -2]
/* --------------------------------------------------------------------- */
/*
 * The inverse routine to hdlcdrv_open().
 */


Error --->
static int hdlcdrv_close(struct net_device *dev)
{
	struct hdlcdrv_state *s;
	int i = 0;
---------------------------------------------------------
[BUG]
/home/kash/linux/2.6.3/linux-2.6.3/drivers/usb/serial/ir-usb.c:329:ir_close:ERROR:INTERFACE_A_B:
Not calling kfree. See ir_open:293 [COUNTER=kmalloc-kfree] [fit=3]
[fit_fn=2] [fn_ex=0] [fn_counter=1] [ex=105] [counter=13] [z =
2.43952273743055] [fn-z = -2]
		dev_err(&port->dev, "%s - failed submitting read urb, error %d\n",
__FUNCTION__, result);

	return result;
}


Error --->
static void ir_close (struct usb_serial_port *port, struct file * filp)
{
	struct usb_serial *serial;

---------------------------------------------------------
[BUG] maxk@qualcomm.com looks like a leak of private_data
/home/kash/linux/2.6.3/linux-2.6.3/drivers/bluetooth/hci_vhci.c:300:hci_vhci_chr_close:ERROR:INTERFACE_A_B:
Not calling kfree. See hci_vhci_chr_open:269 [COUNTER=kmalloc-kfree]
[fit=3] [fit_fn=1] [fn_ex=0] [fn_counter=1] [ex=105] [counter=13] [z =
2.43952273743055] [fn-z = -2]

	file->private_data = hci_vhci;
	return 0;
}


Error --->
static int hci_vhci_chr_close(struct inode *inode, struct file *file)
{
	struct hci_vhci_struct *hci_vhci = (struct hci_vhci_struct *)
file->private_data;

---------------------------------------------------------
[BUG] streamer_open has a bunch of memory leaks on error paths too
/home/kash/linux/2.6.3/linux-2.6.3/drivers/net/tokenring/lanstreamer.c:1202:streamer_close:ERROR:INTERFACE_A_B:
Not calling kfree. See streamer_open:774 [COUNTER=kmalloc-kfree] [fit=3]
[fit_fn=10] [fn_ex=0] [fn_counter=1] [ex=105] [counter=13] [z =
2.43952273743055] [fn-z = -2]
		return 1;
	}
}



Error --->
static int streamer_close(struct net_device *dev)
{
	struct streamer_private *streamer_priv =
	    (struct streamer_private *) dev->priv;
---------------------------------------------------------
[BUG] andrew.grover@intel.com, paul.s.diefenbaugh@intel.com
/home/kash/linux/2.6.3/linux-2.6.3/drivers/acpi/thermal.c:1290:acpi_thermal_remove:ERROR:INTERFACE_A_B:
Not calling kfree. See acpi_thermal_add:1243 [COUNTER=kmalloc-kfree]
[fit=3] [fit_fn=7] [fn_ex=0] [fn_counter=1] [ex=105] [counter=13] [z =
2.43952273743055] [fn-z = -2]
	return_VALUE(result);
}


static int

Error --->
acpi_thermal_remove (
	struct acpi_device	*device,
	int			type)
{
---------------------------------------------------------
[BUG] webvenza@libero.it
/home/kash/linux/2.6.3/linux-2.6.3/drivers/net/sis900.c:2191:sis900_suspend:ERROR:INTERFACE_A_B:
Not calling netif_device_detach. See sis900_resume:2229
[COUNTER=netif_device_attach-netif_device_detach] [fit=4] [fit_fn=2]
[fn_ex=0] [fn_counter=1] [ex=18] [counter=2] [z = 1.11803398874989] [fn-z
= -2]
	pci_set_drvdata(pci_dev, NULL);
}

#ifdef CONFIG_PM


Error --->
static int sis900_suspend(struct pci_dev *pci_dev, u32 state)
{
	struct net_device *net_dev = pci_get_drvdata(pci_dev);
	struct sis900_private *sis_priv = net_dev->priv;
---------------------------------------------------------
[BUG] acme@conectiva.com.br, niemeyer@conectiva.com
/home/kash/linux/2.6.3/linux-2.6.3/drivers/net/wireless/wl3501_cs.c:1286:wl3501_close:ERROR:INTERFACE_A_B:
Not calling netif_device_detach. See wl3501_open:1423
[COUNTER=netif_device_attach-netif_device_detach] [fit=4] [fit_fn=1]
[fn_ex=0] [fn_counter=1] [ex=18] [counter=2] [z = 1.11803398874989] [fn-z
= -2]
fail:
	printk(KERN_WARNING "%s: failed!\n", __FUNCTION__);
	goto out;
}


Error --->
static int wl3501_close(struct net_device *dev)
{
	struct wl3501_card *this = dev->priv;
	int rc = -ENODEV;
---------------------------------------------------------
[BUG]
/home/kash/linux/2.6.3/linux-2.6.3/drivers/net/smc-ultra32.c:301:ultra32_close:ERROR:INTERFACE_A_B:
Not calling ei_close. See ultra32_open:297 [COUNTER=ei_open-ei_close]
[fit=9] [fit_fn=2] [fn_ex=0] [fn_counter=1] [ex=13] [counter=3] [z =
0.125] [fn-z = -2]
	outb(0xff, dev->base_addr + EN0_ERWCNT);
	ei_open(dev);
	return 0;
}


Error --->
static int ultra32_close(struct net_device *dev)
{
	int ioaddr = dev->base_addr - ULTRA32_NIC_OFFSET; /* CMDREG */

---------------------------------------------------------
[BUG] becker@scyld.com
/home/kash/linux/2.6.3/linux-2.6.3/drivers/net/smc-ultra.c:511:ultra_close_card:ERROR:INTERFACE_A_B:
Not calling ei_close. See ultra_open:386 [COUNTER=ei_open-ei_close]
[fit=9] [fit_fn=3] [fn_ex=0] [fn_counter=1] [ex=13] [counter=3] [z =
0.125] [fn-z = -2]
	/* An extra odd byte is OK here as well. */
	outsw(ioaddr + IOPD, buf, (count+1)>>1);
}

static int

Error --->
ultra_close_card(struct net_device *dev)
{
	int ioaddr = dev->base_addr - ULTRA_NIC_OFFSET; /* CMDREG */

---------------------------------------------------------
[BUG] weisd3458@uni.edu does the steps of ei_close but in wrong order and
without holding locks
/home/kash/linux/2.6.3/linux-2.6.3/drivers/net/smc-mca.c:432:ultramca_close_card:ERROR:INTERFACE_A_B:
Not calling ei_close. See ultramca_open:365 [COUNTER=ei_open-ei_close]
[fit=9] [fit_fn=1] [fn_ex=0] [fn_counter=1] [ex=13] [counter=3] [z =
0.125] [fn-z = -2]
	unsigned long shmem = dev->mem_start + ((start_page - START_PG) << 8);

	isa_memcpy_toio(shmem, buf, count);
}


Error --->
static int ultramca_close_card(struct net_device *dev)
{
	int ioaddr = dev->base_addr - ULTRA_NIC_OFFSET; /* ASIC addr */

---------------------------------------------------------
[BUG] (becker@scyld.com, thorn@daimi.aau.dk, hiro@sanpo.t.u-tokyo.ac.jp,
gw4pts@gw4pts.ampr.org, 100136.3530@compuserve.com, gniibe@mri.co.jp,
zimerman@mailandnews.com) parport_unregister_device called in
plip_cleanup_module.  Looks like if we call, plip_attach, plip_detach,
plip_attach, the first nl->pardev is lost
/home/kash/linux/2.6.3/linux-2.6.3/drivers/net/plip.c:1331:plip_detach:ERROR:INTERFACE_A_B:
Not calling parport_unregister_device. See plip_attach:1292
[COUNTER=parport_register_device-parport_unregister_device] [fit=10]
[fit_fn=1] [fn_ex=0] [fn_counter=1] [ex=3] [counter=1] [z = -0.25] [fn-z =
-2]
	return;
}

/* plip_detach() is called (by the parport code) when a port is
 * no longer available to use. */

Error --->
static void plip_detach (struct parport *port)
{
	/* Nothing to do */
}

[PATCH] sis900 fix (Was: [CHECKER] Resource leaks in driver shutdown functions)

[Daniele Venzano]

Thank you for the spotting, the sis900 dirver was really missing a call
to netif_device_detach in sis900_suspend.

Attached is a trivial patch that fixes the issue.

The sis900 driver is currently unmaintained (the MAINTAINERS address
bounces), but I'm willing to take the work, since I know somewhat the
code and I wrote the power management functions.

I no one stands up, I'll send a patch to MAINTAINERS later on.

Bye.

On Mon, May 03, 2004 at 11:46:01AM -0700, Ken Ashcraft wrote:
> The resource allocation/freeing functions in question below are:
> netif_start_queue/netif_stop_queue
> 
> If you are CC'd on this email, it is because I think you are the
> maintainer for some of the code below.  Search for your email address
> below to find it.
> 
> 1	|	/drivers/net/sis900.c
> ---------------------------------------------------------
...
> [BUG] webvenza@libero.it
> /home/kash/linux/2.6.3/linux-2.6.3/drivers/net/sis900.c:2191:sis900_suspend:ERROR:INTERFACE_A_B:
> Not calling netif_device_detach. See sis900_resume:2229
> [COUNTER=netif_device_attach-netif_device_detach] [fit=4] [fit_fn=2]
> [fn_ex=0] [fn_counter=1] [ex=18] [counter=2] [z = 1.11803398874989] [fn-z
> = -2]
> 	pci_set_drvdata(pci_dev, NULL);
> }
> 
> #ifdef CONFIG_PM
> 
> 
> Error --->
> static int sis900_suspend(struct pci_dev *pci_dev, u32 state)
> {
> 	struct net_device *net_dev = pci_get_drvdata(pci_dev);
> 	struct sis900_private *sis_priv = net_dev->priv;

-- 
-----------------------------
Daniele Venzano
Web: http://teg.homeunix.org

Re: [PATCH] sis900 fix (Was: [CHECKER] Resource leaks in driver shutdown functions)

[Randy.Dunlap]

On Tue, 4 May 2004 10:43:26 +0200 Daniele Venzano wrote:

| Thank you for the spotting, the sis900 dirver was really missing a call
| to netif_device_detach in sis900_suspend.
| 
| Attached is a trivial patch that fixes the issue.

Just how trivial is the patch?

-ENOATTACHMENT

| The sis900 driver is currently unmaintained (the MAINTAINERS address
| bounces), but I'm willing to take the work, since I know somewhat the
| code and I wrote the power management functions.
| 
| I no one stands up, I'll send a patch to MAINTAINERS later on.
| 
| Bye.
| 
| On Mon, May 03, 2004 at 11:46:01AM -0700, Ken Ashcraft wrote:
| > The resource allocation/freeing functions in question below are:
| > netif_start_queue/netif_stop_queue
| > 
| > If you are CC'd on this email, it is because I think you are the
| > maintainer for some of the code below.  Search for your email address
| > below to find it.
| > 
| > 1	|	/drivers/net/sis900.c
| > ---------------------------------------------------------
| ...
| > [BUG] webvenza@libero.it
| > /home/kash/linux/2.6.3/linux-2.6.3/drivers/net/sis900.c:2191:sis900_suspend:ERROR:INTERFACE_A_B:
| > Not calling netif_device_detach. See sis900_resume:2229
| > [COUNTER=netif_device_attach-netif_device_detach] [fit=4] [fit_fn=2]
| > [fn_ex=0] [fn_counter=1] [ex=18] [counter=2] [z = 1.11803398874989] [fn-z
| > = -2]
| > 	pci_set_drvdata(pci_dev, NULL);
| > }
| > 
| > #ifdef CONFIG_PM
| > 
| > 
| > Error --->
| > static int sis900_suspend(struct pci_dev *pci_dev, u32 state)
| > {
| > 	struct net_device *net_dev = pci_get_drvdata(pci_dev);
| > 	struct sis900_private *sis_priv = net_dev->priv;
| 
| -- 
| -----------------------------
| Daniele Venzano
| Web: http://teg.homeunix.org


--
~Randy
(Again.  Sometimes I think ln -s /usr/src/linux/.config .signature) -- akpm

Re: [PATCH] sis900 fix (Was: [CHECKER] Resource leaks in driver shutdown functions)

[Daniele Venzano]

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

On Tue, May 04, 2004 at 08:28:49AM -0700, Randy.Dunlap wrote:
> On Tue, 4 May 2004 10:43:26 +0200 Daniele Venzano wrote:
> 
> | Thank you for the spotting, the sis900 dirver was really missing a call
> | to netif_device_detach in sis900_suspend.
> | 
> | Attached is a trivial patch that fixes the issue.
> 
> Just how trivial is the patch?
A one liner, against 2.6.5.

> -ENOATTACHMENT
Sorry, now is attached and is available at this url:
http://teg.homeunix.org/kernel_patches.html

The patch is tested, compiles and run. Could not test that very code
path because of other unrelated errors during suspend, that I was having
before this change.
But it's really a bug, there is a call to netif_device_attach inside
sis900_resume, but there is no corresponding netif_device_detach in
sis900_suspend.

Hope this clarifies.

-- 
-----------------------------
Daniele Venzano
Web: http://teg.homeunix.org


[-- Attachment #2: sis900-detach.diff --]
[-- Type: text/plain, Size: 354 bytes --]

--- linux-2.6.5/drivers/net/sis900.c	2004-04-05 17:33:30.000000000 +0200
+++ linux-2.6.5-exp/drivers/net/sis900.c	2004-05-04 10:15:06.000000000 +0200
@@ -2195,6 +2195,7 @@
 		return 0;
 
 	netif_stop_queue(net_dev);
+	netif_device_detach(net_dev);
 
 	/* Stop the chip's Tx and Rx Status Machine */
 	outl(RxDIS | TxDIS | inl(ioaddr + cr), ioaddr + cr);

Re: [PATCH] sis900 fix (Was: [CHECKER] Resource leaks in driver shutdown functions)

[Rob Landley]

On Tuesday 04 May 2004 03:43, Daniele Venzano wrote:
> Thank you for the spotting, the sis900 dirver was really missing a call
> to netif_device_detach in sis900_suspend.
>
> Attached is a trivial patch that fixes the issue.
>
> The sis900 driver is currently unmaintained (the MAINTAINERS address
> bounces), but I'm willing to take the work, since I know somewhat the
> code and I wrote the power management functions.
>
> I no one stands up, I'll send a patch to MAINTAINERS later on.
>
> Bye.

Does this fix the problem where you unplug the cat 5 cable from an SiS900 and 
then plug it back in (toggling the MII tranciever link detect status and all 
that), and the device goes positively mental until you reboot the system?  
(Packets randomly dropped or delayed for up to 15 seconds, and arriving out 
of sequence with horrible impacts on performance?)

I tried pursuing this when I first noticed it circa 2.4.4, but as you say, the 
driver is unmaintained and I haven't got specs (or any clue about) the 
chipset...

Rob

Re: [PATCH] sis900 fix (Was: [CHECKER] Resource leaks in driver shutdown functions)

[Daniele Venzano]

On Thu, May 06, 2004 at 12:23:40PM -0500, Rob Landley wrote:
> Does this fix the problem where you unplug the cat 5 cable from an SiS900 and 
> then plug it back in (toggling the MII tranciever link detect status and all 
> that), and the device goes positively mental until you reboot the system?  
> (Packets randomly dropped or delayed for up to 15 seconds, and arriving out 
> of sequence with horrible impacts on performance?)
> 
> I tried pursuing this when I first noticed it circa 2.4.4, but as you say,
> the driver is unmaintained and I haven't got specs (or any clue about) the 
> chipset...
I was not aware of this problem, the driver is slow to recognize the
link status of the interface and often needs sending some packets before
switching to link status on. But on my sis900 (on a laptop) I never
observed a behavior similar to yours.

The patch I submitted is a small fix in the power management code, so
it's very unlikely that it fixes anything in the link detection,
please also note that the patch is for kernels 2.6.x.

The driver in 2.6 has some small differencies that probably are fixes
never backported, is the driver in 2.4 also broken for other people ?
Are you using the 2.6 or 2.4 driver ?

Thanks for the feedback.
-- 
-----------------------------
Daniele Venzano
Web: http://teg.homeunix.org

Re: [PATCH] sis900 fix (Was: [CHECKER] Resource leaks in driver shutdown functions)

[Rob Landley]

On Friday 07 May 2004 10:06, Daniele Venzano wrote:
> On Thu, May 06, 2004 at 12:23:40PM -0500, Rob Landley wrote:
> > Does this fix the problem where you unplug the cat 5 cable from an SiS900
> > and then plug it back in (toggling the MII tranciever link detect status
> > and all that), and the device goes positively mental until you reboot the
> > system? (Packets randomly dropped or delayed for up to 15 seconds, and
> > arriving out of sequence with horrible impacts on performance?)
> >
> > I tried pursuing this when I first noticed it circa 2.4.4, but as you
> > say, the driver is unmaintained and I haven't got specs (or any clue
> > about) the chipset...
>
> I was not aware of this problem, the driver is slow to recognize the
> link status of the interface and often needs sending some packets before
> switching to link status on. But on my sis900 (on a laptop) I never
> observed a behavior similar to yours.
>
> The patch I submitted is a small fix in the power management code, so
> it's very unlikely that it fixes anything in the link detection,
> please also note that the patch is for kernels 2.6.x.

I've been using 2.6 exclusively since 2.6.0-test3.  Booted back into 2.4 a 
couple times for debugging purposes in 2003, but I haven't even had a 2.4 
kernel on my laptop since new year's.  (Well, source tarball...)

> The driver in 2.6 has some small differencies that probably are fixes
> never backported, is the driver in 2.4 also broken for other people ?
> Are you using the 2.6 or 2.4 driver ?

The problems were all with servers using the 2.4 driver.  I've got a machine 
in pieces upstairs with an SIS900 controller built into the motherboard, I 
can assemble that and give it a whack at some unspecified point in the 
future.  Also the machine my website and email are on has an SIS900 
on-motherboard controller connecting it to its DSL line, but that's in 
Pennsylvania and a bit difficult to debug at present.

I'll see if the problem persists under 2.6.6.

> Thanks for the feedback.

Thanks for actually taking a toothbrush to the sis900 driver.

Rob