On Tue, Dec 30, 2003 at 04:28:00PM +0200, Peter Pentchev wrote: > On Tue, Dec 30, 2003 at 06:12:53AM -0800, Kris Kennaway wrote: > > On Mon, Dec 29, 2003 at 06:02:45PM -0800, Chris McKenzie wrote: > > > On three machines (PII 450, P3 450, Pentium laptop 200) with FreeBSD-5.1 > > > generic (and specific builds) I am able to completely hard lock the system > > > by doing the following > > > > > > # ifconfig ppp0 create > > > # ifconfig sl0 create > > > > > > Heh . . . that shouldn't happen. > > > > Does the problem persist with 5.2? > > I just tested in on a 5.2-CURRENT as of today, and yes, the system > locked up solid - no ddb, no anything. I'll try to do some more testing > as time permits.
[cc'd to -net for a pre-commit review / discussion] OK, I think I've found the problem. The if_clone_attach() routine in src/sys/net/if.c blindly adds the new cloned interface to the if_cloners list without checking if it is already on the list. This, understandably, leads to problems when trying to attach an interface that already exists - such as a ppp interface. The if_ppp code adds itself to the if_cloners list at the module loading stage. Thus, the very first invocation of ifconfig ppp0 create adds the ppp_cloner structure to the list *again* - and creates a loop on the list. Any attempts to traverse the list later lead to lock-ups. Attached is a patch that does two things: first, only adds the interface to the list if it is not already there (the second and third chunks, at lines 812 and 827 of if.c), and second, adds a if_check_cloners_loop() routine to traverse the if_cloners list and panic if a loop is found. The if_check_cloners_loop() invocations could be protected by INVARIANTS, KASSERT, or WITNESS, but it sure helps find such problems :) Chris, could you try this patch and see if it helps in your situation? And.. happy New Year, everyone! (albeit a little early :) G'luck, Peter -- Peter Pentchev [EMAIL PROTECTED] [EMAIL PROTECTED] [EMAIL PROTECTED] PGP key: http://people.FreeBSD.org/~roam/roam.key.asc Key fingerprint FDBA FD79 C26F 3C51 C95E DF9E ED18 B68D 1619 4553 I am not the subject of this sentence.
Index: src/sys/net/if.c
===================================================================
RCS file: /home/ncvs/src/sys/net/if.c,v
retrieving revision 1.174
diff -u -r1.174 if.c
--- src/sys/net/if.c 26 Dec 2003 18:09:35 -0000 1.174
+++ src/sys/net/if.c 31 Dec 2003 15:15:25 -0000
@@ -762,6 +762,32 @@
}
/*
+ * Check the if_cloners list for loops.
+ */
+static void
+if_check_cloners_loop(void)
+{
+ struct if_clone *ifc, *ifcn, *ifct;
+
+ for (ifc = LIST_FIRST(&if_cloners); ifc != NULL; ) {
+ ifcn = LIST_NEXT(ifc, ifc_list);
+ if (ifcn == NULL)
+ return;
+ if (ifcn == ifc)
+ panic(
+ "cloners loop to self for %p / %s",
+ ifc, ifc->ifc_name);
+ for (ifct = LIST_FIRST(&if_cloners); ifct != ifc;
+ ifct = LIST_NEXT(ifct, ifc_list))
+ if (ifct == ifcn)
+ panic(
+ "cloners loop from %p / %s to %p / %s",
+ ifc, ifc->ifc_name, ifct, ifct->ifc_name);
+ ifc = ifcn;
+ }
+}
+
+/*
* Look up a network interface cloner.
*/
static struct if_clone *
@@ -771,6 +797,7 @@
const char *cp;
int i;
+ if_check_cloners_loop();
for (ifc = LIST_FIRST(&if_cloners); ifc != NULL;) {
for (cp = name, i = 0; i < ifc->ifc_namelen; i++, cp++) {
if (ifc->ifc_name[i] != *cp)
@@ -812,6 +839,8 @@
int err;
int len, maxclone;
int unit;
+ int found;
+ struct if_clone *ift;
KASSERT(ifc->ifc_minifs - 1 <= ifc->ifc_maxunit,
("%s: %s requested more units then allowed (%d > %d)",
@@ -827,8 +856,19 @@
ifc->ifc_units = malloc(len, M_CLONE, M_WAITOK | M_ZERO);
ifc->ifc_bmlen = len;
- LIST_INSERT_HEAD(&if_cloners, ifc, ifc_list);
- if_cloners_count++;
+ if_check_cloners_loop();
+ found = 0;
+ LIST_FOREACH(ift, &if_cloners, ifc_list) {
+ if (ift == ifc) {
+ found = 1;
+ break;
+ }
+ }
+ if (!found) {
+ LIST_INSERT_HEAD(&if_cloners, ifc, ifc_list);
+ if_cloners_count++;
+ if_check_cloners_loop();
+ }
for (unit = 0; unit < ifc->ifc_minifs; unit++) {
err = (*ifc->ifc_create)(ifc, unit);
@@ -840,7 +880,9 @@
bytoff = unit >> 3;
bitoff = unit - (bytoff << 3);
ifc->ifc_units[bytoff] |= (1 << bitoff);
+ if_check_cloners_loop();
}
+ if_check_cloners_loop();
}
/*
@@ -853,6 +895,7 @@
LIST_REMOVE(ifc, ifc_list);
free(ifc->ifc_units, M_CLONE);
if_cloners_count--;
+ if_check_cloners_loop();
}
/*
@@ -877,6 +920,7 @@
count = (if_cloners_count < ifcr->ifcr_count) ?
if_cloners_count : ifcr->ifcr_count;
+ if_check_cloners_loop();
for (ifc = LIST_FIRST(&if_cloners); ifc != NULL && count != 0;
ifc = LIST_NEXT(ifc, ifc_list), count--, dst += IFNAMSIZ) {
strlcpy(outbuf, ifc->ifc_name, IFNAMSIZ);
pgp00000.pgp
Description: PGP signature
