Date: Fri, 28 Apr 2000 04:30:11 -0700 (PDT) From: peter.edwards@ireland.com To: freebsd-gnats-submit@FreeBSD.org Subject: kern/18270: [PATCH] kldunload "vn" doesn't clean up enough, and causes panics. Message-ID: <200004281130.EAA21678@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>Number: 18270
>Category: kern
>Synopsis: [PATCH] kldunload "vn" doesn't clean up enough, and causes panics.
>Confidential: no
>Severity: serious
>Priority: medium
>Responsible: freebsd-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: sw-bug
>Submitter-Id: current-users
>Arrival-Date: Fri Apr 28 04:40:01 PDT 2000
>Closed-Date:
>Last-Modified:
>Originator: Peter Edwards
>Release: 4.0-STABLE/5.0-CURRENT
>Organization:
>Environment:
FreeBSD rocklobster 4.0-STABLE FreeBSD 4.0-STABLE #1: Thu Apr 27 09:32:28 IST 2000 petere@rocklobster:/usr/src/sys/compile/MAR27 i386
>Description:
Some reports on current@freebsd.org
indicated that after "kldunload"ing the vn device, further use of
/dev/vn* caused panics.
I saw a few problems, and attach a patch that addresses them
and appears to fix the crashes on my -stable box.
1: There is no cdesw_remove() when the module is unloaded.
2: Any dev_t's that make it to the driver get associated with a vn
softc. After the driver is unloaded and reloaded, these pointers
are still set in the dev_t's, but refer to the old, free()d softcs.
3: There are no calls to destroy_dev to match the calls to make_dev.
The included patch is complicated slightly because more than one dev_t
can be associated with a vn_softc, and for each softc, only one call to make_dev
is made. The patch keeps track of the "aliases" for the vn_softc by
chaining such aliases through the si_drv2 field in the dev_t.
I reproduced the problem on 4.0-STABLE, and after the patch, several
"kldunload vn.ko/vnconfig" operations leave my machine in one piece.
The problem probably exists in -current, and the patch applies cleanly
(modulo line offset changes) to the -current vn.c also
I'm not the most experienced FreeBSD Kernel Hacker, so a solid review
and comments would be appreciated. I really think there is some ugliness
in the way the vn device sets up the softc's and devices. It should
probably do a make_dev for each different dev_t that refers to a unit
when the unit is first opened, and probably also a make_dev() for a
configurable number of vn's at load time, to get them to appear under
devfs. If someone can confirm that this is an accurate analysis, I'll
spend some time fixing it up, if no one else has time.
>How-To-Repeat:
>Fix:
I've attached the patch as compressed uuencoded, as well as plaintext,
to get around cut-n-paste whitespace corruption.
*** vn.c.old Wed Apr 26 16:23:03 2000
--- vn.c Fri Apr 28 11:56:41 2000
***************
*** 139,144 ****
--- 139,148 ----
int sc_maxactive; /* max # of active requests */
struct buf sc_tab; /* transfer queue */
u_long sc_options; /* options */
+ dev_t sc_devlist; /* list of devices which refer to
+ the same vn unit. The last
+ one on the list is the one
+ created by make_dev */
SLIST_ENTRY(vn_softc) sc_list;
};
***************
*** 179,200 ****
unit = dkunit(dev);
vn = dev->si_drv1;
if (!vn) {
SLIST_FOREACH(vn, &vn_list, sc_list) {
if (vn->sc_unit == unit) {
dev->si_drv1 = vn;
break;
}
}
}
if (!vn) {
vn = malloc(sizeof *vn, M_DEVBUF, M_WAITOK);
if (!vn)
return (NULL);
bzero(vn, sizeof *vn);
vn->sc_unit = unit;
dev->si_drv1 = vn;
! make_dev(&vn_cdevsw, 0,
UID_ROOT, GID_OPERATOR, 0640, "vn%d", unit);
SLIST_INSERT_HEAD(&vn_list, vn, sc_list);
}
return (vn);
--- 183,221 ----
unit = dkunit(dev);
vn = dev->si_drv1;
if (!vn) {
+ /* See if we have already a vn softc for this disk unit. */
SLIST_FOREACH(vn, &vn_list, sc_list) {
if (vn->sc_unit == unit) {
+ /* Just add to the alias list. */
+ dev->si_drv2 = vn->sc_devlist;
+ vn->sc_devlist = dev;
dev->si_drv1 = vn;
break;
}
}
}
if (!vn) {
+ /*
+ * No previous uses of this vn unit: construct its softc and
+ * "canonical" device.
+ */
+ dev_t canonical_dev;
vn = malloc(sizeof *vn, M_DEVBUF, M_WAITOK);
if (!vn)
return (NULL);
bzero(vn, sizeof *vn);
vn->sc_unit = unit;
dev->si_drv1 = vn;
! canonical_dev = make_dev(&vn_cdevsw, 0,
UID_ROOT, GID_OPERATOR, 0640, "vn%d", unit);
+ canonical_dev->si_drv1 = vn;
+ canonical_dev->si_drv2 = 0;
+ vn->sc_devlist = canonical_dev;
+ if (canonical_dev != dev) {
+ /* Add the alias we are opening to the alias list */
+ dev->si_drv2 = vn->sc_devlist;
+ vn->sc_devlist = dev;
+ }
SLIST_INSERT_HEAD(&vn_list, vn, sc_list);
}
return (vn);
***************
*** 763,776 ****
--- 784,812 ----
/* fall through */
case MOD_SHUTDOWN:
for (;;) {
+ dev_t dev, next;
vn = SLIST_FIRST(&vn_list);
if (!vn)
break;
SLIST_REMOVE_HEAD(&vn_list, sc_list);
if (vn->sc_flags & VNF_INITED)
vnclear(vn);
+
+ /*
+ * Cleanup all the dev_t's that refer to this vn
+ * unit, and destroy_dev the canonical device
+ * created with make_dev
+ */
+ for (dev = vn->sc_devlist; dev; dev = next) {
+ next = dev->si_drv2;
+ dev->si_drv1 = dev->si_drv2 = 0;
+
+ if (next == 0)
+ destroy_dev(dev);
+ }
free(vn, M_DEVBUF);
}
+ cdevsw_remove(&vn_cdevsw);
break;
default:
break;
UUencoded:begin 644 patch.Z
M'YV0*@*"L./&Q1@7;]B027"E#!D00>#(`2'#!H@8-G3(F*$#Q@R*,$(J:$%R
M8,$Q"8S(20-1(D4<%V/HJ)&11@R0(@/JW,E3A0*=%V?D8!&#!@T0.T>6C"&4
M*`V8)$DJ``$B01HW=!(D`#%GS)<V8?"$&4,GC9TR.Q*\4`$"+!X0(T"\,0-B
M;-FS(.24B5.GS!PZ<ZJJ>#&UZE\Y=<B"$%/'S-:N7^B$$9-V+0@Z<L*XF6.F
MS$2^9?I633"X<((Z7]B\<7-&*U>O;^"473VG,MO8LS>/)DUX154R9>Q$=@T9
MN!TV:?[:!H'\KURZQM.,\0OB#AKI:/*6Z3R1SAL%OK6*IWH931FN8=J<)PBB
MCILT=%R`H&*>>9B_X'>[IKKZ_.KRYS5'!PC)`2B7&V7D)]Y^((RA5QAT.+18
M'FV%L4897QBG56E4)3`%$TE,0<4713A!A119H$#0%W/,1<<8*;R66G)T[%!8
M'S92]5-///H$5`PW#"5#2$CI9)I[\('0`PADK($D'2@8ET*.5;&WI'$M^#!'
M&AG*84<,5%I%%PHA$!3C'J9Y"**(7QCQA!1%!#$$$BJZP0(()JPHX)V0"7AF
MFF*"4&>6D#VIY))/_MFA>%AJR2497MZT)$%ABB?&@VM4FD`?:7+:H:=5I3%F
MF6XH.IJ5%;*AVA@H;*E'&7,A1="=37Q!1!%6"%&%$;1^<44025#QQ!)3IBFJ
MH*2F`*A>=-0AAQN".E$%$TP4NZ@8K\KQ1IU\IO%JK"J862E!A'IE**+OU9AF
MHUMV^:62)MD8PFA@79AA<"CDZ<878Q@WQQUWPG!GFN2!4$421'PAQ1-/4''G
M$0A_\00414@1A+!2!&P##0*#(`)!)9`APIV)5OIAB",FX<04%8^(1)Q$Y*LG
MC7?.*J.?88*:`+/.0EMGL5%=A,,,+,@@PTU1M7!DNO`V^624P5E;);17!D?H
MHY&&>2R99H*`9GAJL35%&><=>\=Y:(2!5QAL/$@&A6&8Q)6+8X!@QAO=71<8
M&<FMT5ZZ\G$XVLELN@FGG'3:K.^,?_'IE9]>`[HUN5J:RW0/Z,+W)]A:6:9$
M'<Z%0<9#WAG(=AKW,4=CX+WIQR[6=L@`+^7%!2>@C9PG0+M7Q@G8='":)O"Z
MNY+&"V@"EY9AH:8Z;VJ:SELG&SG8:RFX%5M.O`&"1,&E\0;H[<U!7:QTZ"WW
MDSHT2!MFB0T(7V`MFO%B76Z083U2'H^AV6K2L2T"D]V;C@ON5QJP&2<R#=K?
M>_3'AGO985Q42]6J6N4M6-$E7'8"0:UNE:M=]>I7P1J6U+02/3,MJPS->E:T
MIE6M2F'+,]NRF:LL**M20;!<7SC7W^!3J>%!ZEV3<H.\1J,_-_"/@0Z$5[TP
M9!R9[:M?P?E7P`:VJ((=+&$+:]C#(C:QBEWL31KCV)T^YH:0C6R'="@6V(IX
M1+8Y\&K$FYT0K<?&!;K1AUZ2W9)@@+M3N0&'O:,1O.K8OP8:IX\D'!,AD6@<
M$(2@:G;8W&XL$X31F0XYJ3M;7?0B%SB4X3VLN8SVRG>>TZ7.=P7<#1YC)T=`
MVHY&B-3*[ASH.TC&LGF$2]G*6O:%EP4A9HO;DTD<Q[@TYLPT/%/ASVS4(Q[M
MB"TWL`'1;A#-(@5$*2T`P0UP0`,6X"`&LDM:FBQC!K:QH3S:JL,9LB.X!.A/
M?!I\0L*F@(0J4($(3[B"$W20IKM-!`4[V($D&14<!!KG3@C"@[H6I;L(YK)-
M29""B)Q8S!$F$EDF9"CR,*6IA\*I"4^P0A%Z"3.*"K-/-+)HH`95N2^8@0UA
M.$-@3``"*SC!"%]06;"*0`1E:90@8V"#\N2PS/Q8+VQ'Q=\0A*J9.L"A+JHR
MT`'I<(+`E`]"VN&.*,M3(((DE2U/NI-F'@*<P[PA#TDD90*-:,=S1F<Z7VW0
M@R+TD#O`)SM+=.!76R<>?PJJD4%TY7%@"<`'%A9>"4WC]/23V-_9`8X_E$$L
MM;+*XJU2CR#@HU%SM[7&8BZSRLJ=\/R"F;,Z$&J1G&SS$F`&O92!6QJT%:YT
G902+<FJ-_KK#%_32AC><A:)0M,._+)J\Y9D&..6L`QOHP,]K<50!
>Release-Note:
>Audit-Trail:
>Unformatted:
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-bugs" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200004281130.EAA21678>