kernel/2910: lkm(4) has internal issues

>Number: 2910
>Category: kernel
>Synopsis: lkm(4) has internal issues
>Confidential: no
IFOST http://www.ifost.org.au
>Environment:

	System      : OpenBSD 3.1
	Architecture: OpenBSD.i386
	Machine     : i386

	lkm(4) has an internal variable 'nlkms' containing the number of
	currently loaded modules, when a module is loaded or unloaded, it is
	incremented or decremented respectively. a freshly loaded module will
	be given a module id equal to the current value of nlkms. 

	the problem comes when a module is removed, and the module removed is
	not the last module loaded (ie, not loaded/unloaded in LIFO order).

	if i load module a, b and c, module a has id 0, b has id 1, and c has
	id 2. nlkms will be equal to 3. if i then modunload(8) module b, nlkms
	is decremented to be 2, and module b will be removed from lkm(4)'s
	list of modules. 

	the problem is mainly here (kern/kern_lkm.c:lkmlookup()) 

			....
                for (p = TAILQ_FIRST(&lkmods); p != NULL && i--;
                     p = TAILQ_NEXT(p, list))
                        ;
			...

	lkmmods is the list of loaded modules, i is the id of the module we
	want. if b has been removed there are 2 entries in the list (a id 0,
	and c id 2). modstat will look for modules by id sequentially,
	starting at 0. the first run (i == 0) goes as expected, but in the
	second run i == 1, the first iteration will return module a, the
	second iteration finds module c, but as i is now equal to 0, the loop
	is exited and module c's information is returned to modstat. 

	so modstat prints out information for module c, displaying it as
	having a module id of 2. this is correct, but lkm(4) will not let it
	be unloaded due to this piece of code (same function as above).

		 	...
		} else if (i >= nlkms) {
			*error = EINVAL;
			return NULL;
		}
			...

	since its id (2) is equal to nlkms (2). You can still remove it with 
	modstat using an id of 1, as this is < nlkms and in the lookup code 
Do you need help?
In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software:
Learn more about Pantek's Technical Support offerings
Place an order for 24/7/365 Technical Support here
Learn more about Pantek, who we are, and what we do
Chat with a Live Operator
Related openbsd.org Links
advocacy
announce
bugs
ipv6
misc
ports
ports-changes
security-announce
smp
source-changes
tech
Request more information about Pantek
	above it will be returned as the 2nd item of the list.

>How-To-Repeat:

	load three modules, remove the module you loaded second, run modstat
	and try and unload the module you loaded last with the id reported by
	modstat.
	

>Fix:

        this diff does 4 things to fix the problem:         

1. makes sure the first un-used module id is given to a new module, and keeps the tailq sorted by module id, lowest first to highest.
2. moves lkm_init() to kern/init_main.c
3. gets rid of the nlkms variable.
4. makes the lookup function easier to understand.

	it also contains a fix by eric jackson to stop a kernel panic reported
	on tech@openbsd.org 

--- kern/kern_lkm.c.old	Mon Jul 29 15:03:07 2002
+++ kern/kern_lkm.c	Fri Aug  2 11:31:08 2002

-static TAILQ_HEAD(, lkm_table) lkmods;	/* table of loaded modules */
+static TAILQ_HEAD(lkmods, lkm_table) lkmods;	/* table of loaded modules */
 static struct lkm_table	*curp;		/* global for in-progress ops */
-static size_t nlkms = 0;		/* number of loaded lkms */
 

 static struct lkm_table *lkmalloc(void);  static void lkmfree(struct lkm_table *); @@ -131,9 +130,6 @@

 	}
 	lkm_v |= LKM_ALLOC;
 
-	if (nlkms == 0)
-		lkminit();	/* XXX */
-
 	return (0);		/* pseudo-device open */

@@ -145,13 +141,28 @@
 static struct lkm_table *
 lkmalloc()
 {

-	struct lkm_table *ret = NULL;
+	struct lkm_table *p, *ret = NULL;
+	int id = 0;
 
 	MALLOC(ret, struct lkm_table *, sizeof(*ret), M_DEVBUF, M_WAITOK);
 	ret->refcnt = ret->depcnt = 0;
-	ret->id = nlkms++;
 	ret->sym_id = -1;
-	TAILQ_INSERT_TAIL(&lkmods, ret, list);
+	/* 
+	 * walk the list finding the first free id. as long as the list is
+	 * kept sorted this is not too ineffcient, which is why we insert in
+	 * order below.
+ 	 */
+	TAILQ_FOREACH(p, &lkmods, list) {
+		if (id == p->id)
+			id++;
+		else
+			break;
+	}
+	ret->id = id;
+	if (p == NULL) /* either first or last entry */
+		TAILQ_INSERT_TAIL(&lkmods, ret, list);
+	else 
+		TAILQ_INSERT_BEFORE(p, ret, list);
 
 	return ret;

 	TAILQ_REMOVE(&lkmods, p, list);
 	free(p, M_DEVBUF);
-	nlkms--;

 struct lkm_table *
@@ -185,6 +196,18 @@
 {

 	struct lkm_table *p = NULL;
 	char istr[MAXLKMNAME];
+	
+	/* 
+ 	 * p being NULL here implies the list is empty, so any lookup is
+ 	 * invalid (name based or otherwise). Since the list of modules is
Do you need more help?
In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software:
Learn more about Pantek's Technical Support offerings
Place an order for 24/7/365 Technical Support here
Learn more about Pantek, who we are, and what we do
Chat with a Live Operator
Related openbsd.org Links
advocacy
announce
bugs
ipv6
misc
ports
ports-changes
security-announce
smp
source-changes
tech
Request more information about Pantek
+ 	 * kept sorted by id, lowest to highest, the id of the last entry 
+ 	 * will be the highest in use. 
+	 */ 
+	p = TAILQ_LAST(&lkmods, lkmods);
+	if (p == NULL || i > p->id) {
+		*error = EINVAL;
+		return NULL;
+	}
 
 	if (i < 0) {		/* unload by name */
 		/*
@@ -200,14 +223,11 @@
 			if (!strcmp(istr, p->private.lkm_any->lkm_name))
 				break;
 		}
-	} else if (i >= nlkms) {
-		*error = EINVAL;
-		return NULL;
-	} else
-		for (p = TAILQ_FIRST(&lkmods); p != NULL && i--;
-		     p = TAILQ_NEXT(p, list))
-			;
-
+	} else 
+		TAILQ_FOREACH(p, &lkmods, list) 
+			if (i == p->id)
+				break;
+		
 	if (p == NULL)
 		*error = ENOENT;
 
@@ -264,7 +284,6 @@
 		 * a premature exit of "modload".
 		 */
 		lkmunreserve();
-		lkmfree(curp);
 	}
 	lkm_v &= ~LKM_ALLOC;
 	wakeup((caddr_t)&lkm_v);	/* thundering herd "problem" here */

 	case LMUNRESRV:
 		lkmunreserve();
-		if (curp)
-			lkmfree(curp);
 		break;
 
 	case LMREADY:
--- kern/init_main.c.old	Tue Jul 30 14:32:29 2002
+++ kern/init_main.c	Fri Aug  2 11:30:25 2002

+#ifdef LKM
+#include 
+#endif
+
 const char	copyright[] =

 	/* Initialize System V style message queues. */
 	msginit();
+#endif
+
+#ifdef LKM
+	/* Initialize Loadable kernel modules */
+	lkminit();

 	/* Attach pseudo-devices. */
--- sys/lkm.h.old	Fri Aug  2 11:27:46 2002
+++ sys/lkm.h	Fri Aug  2 11:28:20 2002
@@ -359,4 +359,5 @@
 	int	ver;		/* OUT: lkm compile version */

+void lkminit(void);
 #endif /* !_SYS_LKM_H_ */

Can we help you?

In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software: Learn more about Pantek's Technical Support offerings Place an order for 24/7/365 Technical Support here Learn more about Pantek, who we are, and what we do Chat with a Live Operator Related openbsd.org Links advocacy announce bugs ipv6 misc ports ports-changes security-announce smp source-changes tech Request more information about Pantek

>Release-Note:
Received on Thu Nov 7 15:48:44 2002