How to hard lock FreeBSD-5.1 generic with sl

[Peter Pentchev]

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	roam at ringlet.net    roam at sbnd.net    roam at FreeBSD.org
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.
-------------- next part --------------
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);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-net/attachments/20031231/42df306d/attachment.bin