Date: Wed, 24 May 2006 17:38:01 +0200 (CEST) From: Ulrich Spoerlein <uspoerlein@gmail.com> To: FreeBSD-gnats-submit@FreeBSD.org Subject: kern/98015: [PATCH] bfe(4): double free in error handling path. Message-ID: <200605241538.k4OFc1O4001715@roadrunner.q.local> Resent-Message-ID: <200605271600.k4RG0jRp046976@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>Number: 98015
>Category: kern
>Synopsis: [PATCH] bfe(4): double free in error handling path.
>Confidential: no
>Severity: non-critical
>Priority: low
>Responsible: freebsd-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: sw-bug
>Submitter-Id: current-users
>Arrival-Date: Sat May 27 16:00:44 GMT 2006
>Closed-Date:
>Last-Modified:
>Originator: Ulrich Spoerlein
>Release: FreeBSD 6.1-STABLE i386
>Organization:
>Environment:
>Description:
When bfe_attach() fails, bfe_release_resources() is sometimes called twice.
>How-To-Repeat:
Change one if (foo()) to if (foo() || 1) and watch it panic.
>Fix:
Please note, that I really don't know what I'm doing here! I looked at various
other drivers in the tree, and they are all similiar yet slightly different. I'm
pretty sure, there are more faulty drivers due to copy'n'paste.
Since bfe_release_resource() is now only called from bfe_detach(), these two
could/should be merged. This would bring bfe(4) more in line with other NIC
drivers. There are also some style(9) "violations"
NB: Found by: Coverity CID 476
Then, there was a lock order reversal on kldunload. It seems it has vanished,
though I'm really not sure right now.
--- syscall (444, FreeBSD ELF32, kldunloadf), eip = 0x480b45cb, esp = 0xbfbfe89c, ebp = 0xbfbfed08 ---
lock order reversal: (sleepable after non-sleepable)
1st 0xc5020114 bfe0 (network driver) @ /usr/src/sys/modules/bfe/../../dev/bfe/if_bfe.c:446
2nd 0xc08b3b40 ACPI root bus (ACPI root bus) @ /usr/src/sys/modules/acpi/acpi/../../../dev/acpica/acpi.c:1075
KDB: stack backtrace:
kdb_backtrace(0,ffffffff,c072b2c0,c072bc20,c06f2d8c) at kdb_backtrace+0x29
witness_checkorder(c08b3b40,9,c08ac97b,433) at witness_checkorder+0x578
_sx_xlock(c08b3b40,c08ac97b,433,c4b4cc00,c4dc1640) at _sx_xlock+0x50
acpi_release_resource(c4b4cc00,c4c28500,1,0,c4dc1640) at acpi_release_resource+0x23
bus_generic_release_resource(c4b4c700,c4c28500,1,0,c4dc1640) at bus_generic_release_resource+0x64
resource_list_release(c4c4cd04,c4c04980,c4c28500,1,0) at resource_list_release+0x6e
bus_generic_rl_release_resource(c4c04980,c4c28500,1,0,c4dc1640) at bus_generic_rl_release_resource+0x5e
bus_generic_release_resource(c4c04600,c4c28500,1,0,c4dc1640) at bus_generic_release_resource+0x64
resource_list_release(c4c4cd04,c4c28580,c4c28500,1,0) at resource_list_release+0xfb
bus_generic_rl_release_resource(c4c28580,c4c28500,1,0,c4dc1640) at bus_generic_rl_release_resource+0x5e
bus_release_resource(c4c28500,1,0,c4dc1640,c501e000) at bus_release_resource+0x61
bfe_release_resources(c4dc9c00,c4c28500,c4c28500,c4ef4800,ed0cdc30) at bfe_release_resources+0x142
bfe_detach(c4c28500) at bfe_detach+0x58
device_detach(c4c28500) at device_detach+0x70
devclass_delete_driver(c4b0a240,c501d6f0,c4dc1900,c4f6d700,0) at devclass_delete_driver+0x8c
driver_module_handler(c4dc1900,1,c501d6dc,c4dc1900,ed0cdcb0) at driver_module_handler+0xa5
module_unload(c4dc1900,0,c071c500,c06b0e72,1fb) at module_unload+0x37
linker_file_unload(c4f6d700,0,0,c4db1900,ed0cdcdc) at linker_file_unload+0x72
kern_kldunload(c4db1900,1d,0,ed0cdd30,c0678e17) at kern_kldunload+0x7c
kldunloadf(c4db1900,ed0cdd04,2,0,292) at kldunloadf+0x1e
syscall(3b,3b,3b,1d,bfbfee37) at syscall+0x22f
Xint0x80_syscall() at Xint0x80_syscall+0x1f
--- syscall (444, FreeBSD ELF32, kldunloadf), eip = 0x480b45cb, esp = 0xbfbfe89c, ebp = 0xbfbfed08 ---
--- if_bfe.patch begins here ---
--- if_bfe.c.orig Tue May 23 19:49:54 2006
+++ if_bfe.c Wed May 24 17:16:32 2006
@@ -369,7 +369,6 @@
if (bfe_dma_alloc(dev)) {
printf("bfe%d: failed to allocate DMA resources\n",
sc->bfe_unit);
- bfe_release_resources(sc);
error = ENXIO;
goto fail;
}
@@ -424,13 +423,14 @@
bfe_intr, sc, &sc->bfe_intrhand);
if (error) {
- bfe_release_resources(sc);
printf("bfe%d: couldn't set up irq\n", unit);
+ ether_ifdetach(ifp);
goto fail;
}
fail:
if (error)
- bfe_release_resources(sc);
+ bfe_detach(dev);
+
return (error);
}
@@ -443,24 +443,17 @@
sc = device_get_softc(dev);
KASSERT(mtx_initialized(&sc->bfe_mtx), ("bfe mutex not initialized"));
- BFE_LOCK(sc);
-
ifp = sc->bfe_ifp;
if (device_is_attached(dev)) {
+ BFE_LOCK(sc);
bfe_stop(sc);
+ bfe_chip_reset(sc);
+ BFE_UNLOCK(sc);
ether_ifdetach(ifp);
}
- bfe_chip_reset(sc);
-
- bus_generic_detach(dev);
- if(sc->bfe_miibus != NULL)
- device_delete_child(dev, sc->bfe_miibus);
-
bfe_release_resources(sc);
- BFE_UNLOCK(sc);
- mtx_destroy(&sc->bfe_mtx);
return (0);
}
@@ -922,6 +915,11 @@
dev = sc->bfe_dev;
+ if (sc->bfe_miibus != NULL)
+ device_delete_child(dev, sc->bfe_miibus);
+
+ bus_generic_detach(dev);
+
if (sc->bfe_vpd_prodname != NULL)
free(sc->bfe_vpd_prodname, M_DEVBUF);
@@ -971,6 +969,8 @@
if(sc->bfe_parent_tag != NULL)
bus_dma_tag_destroy(sc->bfe_parent_tag);
+
+ mtx_destroy(&sc->bfe_mtx);
return;
}
--- if_bfe.patch ends here ---
>Release-Note:
>Audit-Trail:
>Unformatted:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200605241538.k4OFc1O4001715>